test(covenantsigner): add missing signer defensive-property coverage#4178
Open
piotr-roslaniec wants to merge 3 commits into
Open
test(covenantsigner): add missing signer defensive-property coverage#4178piotr-roslaniec wants to merge 3 commits into
piotr-roslaniec wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, noproduction 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.goTestVerifySignerApprovalCertificateRejectsUnsupportedVersion— thecertificateVersion == 1hard gate (
signer_approval_certificate.go:230) had no test; rejects both0and2.TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm— thetecdsa-secp256k1algorithm gate (:233) had no test.pkg/tbtc/covenant_signer_test.goTestEnsureActiveOutpointFinalityPropagatesProviderError— the finality guard's error path(
covenant_signer.go:614) was untested; existing tests only cover known confirmation countsabove/below the threshold. Asserts fail-closed when confirmations cannot be determined.
pkg/covenantsigner/server_test.goTestServerAcceptsBodyExactlyAtLimit— complements the existingmaxRequestBodyBytes+1rejection in
TestServerBoundaryErrorMatrixby pinning the inclusive boundary (a body ofexactly
maxRequestBodyBytesmust pass the size guard).pkg/covenantsigner/store_lock_test.goTestNewServiceReleasesLockOnInitFailure— assertsNewServicereleases the store file lockwhen a later init step fails (
service.go:135-142defer), so a signer that failed to startonce 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 expirychecks, 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 -lclean,go vetclean.go test -race ./pkg/tbtc/trips a pre-existing data race inTestSigningExecutor_SignBatch(reproduces on this branch's base tipaf236a25awith none ofthese 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:
Signer-approval expiry binding. The three tip commits (
e572ea3a8/a882cf142/ccbc4c3ac, "cryptographically bind + enforce signer-approval expiry") were reverted.SignerApprovalCertificate.EndBlockis still checked at Poll (service.go:275-290), but theECDSA signature covers
approvalDigestonly —EndBlockis not inside the signed digest, soits 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.)chainId == 0guard on the EIP-712 leg. On the follow-upfeat/covenant-eip712-eth-leg(feat(covenantsigner): wallet-signed EIP-712 approvals + cooperative REDEEM/RENEW #4169),
EIP712ChainIDis folded into the EIP-712 domain separator, but nothing rejectsEIP712ChainID == 0at startup — a misconfig fails soft (no realeth_signTypedData_v4signature is ever produced under chainId 0, so verification just never matches). Intentional,
or should we add a startup guard + test?