Skip to content

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

Merged
piotr-roslaniec merged 3 commits into
fix/tob-tbtcacext-remediationfrom
test/covenant-signer-defensive-coverage
Jul 31, 2026
Merged

test(covenantsigner): add missing signer defensive-property coverage#4178
piotr-roslaniec merged 3 commits into
fix/tob-tbtcacext-remediationfrom
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: 4f689f07-96d3-4eab-81b3-cc7d25cc3b09

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.

@lrsaturnino

lrsaturnino commented Jul 29, 2026

Copy link
Copy Markdown
Member

This is a clean, test-only PR — four _test.go files, +214/-0, no production code changed. All three claimed coverage gaps are genuine, and all five new tests exercise the code paths they claim.

Two non-blocking suggestions:

  • server_test.go:939 — the new boundary test pins the cap at exactly N bytes, but the existing oversized-body case is actually N+92 bytes, not N+1 as its comment implies, leaving a 91-byte band untested in the loosen-the-cap direction.
  • server_test.go:986 — the rejection assertion matches a message shared by two different decode-failure paths; tying it to the typed SignerSubmitInput struct (as the neighboring test already does via mustJSON) would make future schema drift fail at compile time instead of producing a misleading diagnostic.

@lrsaturnino lrsaturnino left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-only change (four _test.go files, +214/-0), no production code touched, no behavioral regressions. Two non-blocking suggestions noted in comments.

@lrsaturnino
lrsaturnino changed the base branch from feat/psbt-covenant-final-project-pr to fix/tob-tbtcacext-remediation July 30, 2026 16:26
@lrsaturnino
lrsaturnino force-pushed the test/covenant-signer-defensive-coverage branch from e575b0a to 6740c42 Compare July 30, 2026 16:26
@lrsaturnino
lrsaturnino self-requested a review July 30, 2026 16:47
@lrsaturnino
lrsaturnino force-pushed the fix/tob-tbtcacext-remediation branch from b5e1166 to d1a909a Compare July 30, 2026 21:24
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.
@lrsaturnino
lrsaturnino force-pushed the test/covenant-signer-defensive-coverage branch from 6740c42 to eb5aab5 Compare July 30, 2026 21:25

@lrsaturnino lrsaturnino left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@piotr-roslaniec
piotr-roslaniec merged commit d337848 into fix/tob-tbtcacext-remediation Jul 31, 2026
18 checks passed
@piotr-roslaniec
piotr-roslaniec deleted the test/covenant-signer-defensive-coverage branch July 31, 2026 18:23
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.

2 participants