Skip to content

test(covenantsigner): add missing signer defensive-property coverage#4178

Open
piotr-roslaniec wants to merge 3 commits into
feat/psbt-covenant-final-project-prfrom
test/covenant-signer-defensive-coverage
Open

test(covenantsigner): add missing signer defensive-property coverage#4178
piotr-roslaniec wants to merge 3 commits into
feat/psbt-covenant-final-project-prfrom
test/covenant-signer-defensive-coverage

Conversation

@piotr-roslaniec

Copy link
Copy Markdown
Collaborator

What

Adds five regression tests closing genuinely-missing defensive-property coverage for the
covenant signer. Stacked on feat/psbt-covenant-final-project-pr (#3882); tests only, no
production changes.

Why

A coverage review of the signer's defensive properties (auth, body cap, timeouts, replay,
reorg guard, approval-certificate validation) started from a ~27-item wishlist but found that
~22 already exist in the suite. Rather than duplicate them, this PR adds only the handful
that were actually absent, plus one boundary complement.

Tests added

pkg/tbtc/signer_approval_certificate_test.go

  • TestVerifySignerApprovalCertificateRejectsUnsupportedVersion — the certificateVersion == 1
    hard gate (signer_approval_certificate.go:230) had no test; rejects both 0 and 2.
  • TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm — the
    tecdsa-secp256k1 algorithm gate (:233) had no test.

pkg/tbtc/covenant_signer_test.go

  • TestEnsureActiveOutpointFinalityPropagatesProviderError — the finality guard's error path
    (covenant_signer.go:614) was untested; existing tests only cover known confirmation counts
    above/below the threshold. Asserts fail-closed when confirmations cannot be determined.

pkg/covenantsigner/server_test.go

  • TestServerAcceptsBodyExactlyAtLimit — complements the existing maxRequestBodyBytes+1
    rejection in TestServerBoundaryErrorMatrix by pinning the inclusive boundary (a body of
    exactly maxRequestBodyBytes must pass the size guard).

pkg/covenantsigner/store_lock_test.go

  • TestNewServiceReleasesLockOnInitFailure — asserts NewService releases the store file lock
    when a later init step fails (service.go:135-142 defer), so a signer that failed to start
    once can restart against the same data dir. Only the store-level Close() release was tested.

Dropped from the original list as already covered: the non-loopback-without-token startup
guard (already asserted inside TestInitializeRejectsInvalidOrUnavailablePort), Poll expiry
checks, confirmation thresholds/boundaries, dedup, lock contention, digest/signer-set/high-S/DER
cert rejections, trust-root pinning, quote expiry, oversized-body, and healthz-exempt.

Verification

  • go test ./pkg/tbtc/ ./pkg/covenantsigner/ — pass.
  • go test -race ./pkg/covenantsigner/ — pass.
  • gofmt -l clean, go vet clean.
  • Note: go test -race ./pkg/tbtc/ trips a pre-existing data race in
    TestSigningExecutor_SignBatch (reproduces on this branch's base tip af236a25a with none of
    these changes; unrelated to the covenant signer). Flagging, not fixing here.

Open questions for @lrsaturnino

Two items surfaced during the review that need an owner decision; they gate two further tests
not included here:

  1. Signer-approval expiry binding. The three tip commits (e572ea3a8 / a882cf142 /
    ccbc4c3ac, "cryptographically bind + enforce signer-approval expiry") were reverted.
    SignerApprovalCertificate.EndBlock is still checked at Poll (service.go:275-290), but the
    ECDSA signature covers approvalDigest only — EndBlock is not inside the signed digest, so
    its value is caller-settable. Is the binding intended to re-land, or is Poll-time check-only
    the accepted design? (Determines whether we add a TestEndBlockIsBoundToSignature.)

  2. chainId == 0 guard on the EIP-712 leg. On the follow-up feat/covenant-eip712-eth-leg
    (feat(covenantsigner): wallet-signed EIP-712 approvals + cooperative REDEEM/RENEW #4169), EIP712ChainID is folded into the EIP-712 domain separator, but nothing rejects
    EIP712ChainID == 0 at startup — a misconfig fails soft (no real eth_signTypedData_v4
    signature is ever produced under chainId 0, so verification just never matches). Intentional,
    or should we add a startup guard + test?

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e59d909-84d0-424c-aaaf-9b38f0ba50e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/covenant-signer-defensive-coverage

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The version and signature-algorithm gates in verifySignerApprovalCertificate
return before the expected signer-set hash is examined, so the hash computed
via expectedSignerSetHashForWallet was never evaluated on those paths. Pass an
arbitrary non-empty placeholder instead and document that a reordering of the
gate after the hash comparison would now surface as a mismatch, catching the
regression. Removes the helper, whose only two call sites are gone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant