fix(covenantsigner): apply audit findings from #3935 rebased onto current base#3940
Merged
lrsaturnino merged 1 commit intofeat/psbt-covenant-final-project-prfrom Apr 16, 2026
Conversation
Implements 15 fixes from the consolidated audit of PR #3933 (covenant signer + fault isolation). These address findings from 5 review passes (initial triage, 2 multi-agent lens reviews, PR reviewer feedback, validation review). **P1 - High** - **A2**: Use `canonicaljson.Marshal` for handoff payload hash (cross-language determinism) - **A3/A23**: Improve wallet registry error messages, elevate log to Error, add WalletChainData godoc - **A4**: Extract Submit critical section into `createOrDedup` helper (5 unlock points -> defer) - **A5**: Deterministic tiebreaker when both timestamps unparseable (lexicographic RequestID) - **A6**: Restrict healthz auth bypass to GET method only - **A22**: Poison route keys from skipped jobs to preserve dedupe semantics **P2 - Medium** - **A7**: Document AuthToken CLI flag /proc visibility risk - **A12/A27**: Cancel service context on init failure + add SIGINT/SIGTERM signal handling - **A15**: Document advisory flock limitations and storage requirements - **A16**: Add aggregate load summary with skip count - **A24**: Remove superseded job from byRequestID on dedup replacement - **A25**: Rename misleading test after resilient loading change - **A29**: Use `errors.Is()` for errJobNotFound comparison --- **A1: Default `requireApprovalTrustRoots` to true in production** - **File:** `pkg/covenantsigner/server.go:70-75` - **Risk:** When `signerApprovalVerifier` is nil, the service accepts requests without cryptographic signer approval verification. The `requireApprovalTrustRoots` config flag exists as mitigation but defaults to false. - **Action:** Default `requireApprovalTrustRoots` to true in production configs, or fail startup when port is non-zero and no verifier is available. - **Effort:** Low **A8: Bitcoin regtest integration tests for witness scripts** - **File:** `pkg/tbtc/covenant_signer.go` (design) - **Risk:** Go unit tests cannot enforce `cleanstack`, signature encoding quirks, or opcode limits. A script passing unit tests could be rejected by the Bitcoin network, locking funds. - **Action:** Introduce `btcd` or Bitcoin Core regtest integration tests that compile witness scripts, sign transactions, and broadcast on a local regtest node. - **Effort:** High **A9: Split monolithic validation.go (1922 lines)** - **File:** `pkg/covenantsigner/validation.go` - **Issue:** Mixes input parsing, crypto verification, normalization, and commitment computation in a single file. - **Action:** Split into focused files: `validation_quote.go`, `validation_approval.go`, `validation_template.go`. - **Effort:** High **A10: Deduplicate UTXO resolution logic** - **File:** `pkg/tbtc/covenant_signer.go:512-628` - **Issue:** `resolveSelfV1ActiveUtxo` and `resolveQcV1ActiveUtxo` are near-identical (~60 lines each). - **Action:** Extract shared `resolveActiveUtxo(request, witnessScript, templateName)` helper. - **Effort:** Medium **A11: Deduplicate trust root normalization** - **File:** `pkg/covenantsigner/validation.go:628-724` - **Issue:** `normalizeDepositorTrustRoots` and `normalizeCustodianTrustRoots` are structurally identical. - **Action:** Unify via generics or shared inner function. - **Effort:** Low **A13: strictUnmarshal trailing token rejection** - **File:** `pkg/covenantsigner/validation.go:71-75` - **Issue:** Inconsistent with server-level `decodeJSON` which rejects trailing JSON tokens. - **Action:** Add trailing-token rejection or document single-document context constraint. - **Effort:** Low **A14: Document Engine interface contract** - **File:** `pkg/covenantsigner/engine.go` - **Issue:** No documentation of what nil Transition means (OnSubmit: default to Pending; OnPoll: no-op). - **Action:** Add godoc specifying the contract and allowed return states. - **Effort:** Low **A26: Rate limiting on submit endpoint** - **File:** `pkg/covenantsigner/server.go:345` - **Issue:** Authenticated callers can spawn unbounded concurrent 5-minute signing operations. Mitigated by auth and routeRequestID dedup but no per-client throttle exists. - **Action:** Add token-bucket rate limiting per auth token (e.g., `golang.org/x/time/rate`, ~5 req/min). - **Effort:** Medium **A28: Replace reflect.DeepEqual for Handoff comparison** - **File:** `pkg/covenantsigner/service.go:201` - **Issue:** `sameJobRevision` uses `reflect.DeepEqual` for `map[string]any` Handoff comparison. Correct but potentially slow at scale. - **Action:** Define a concrete `HandoffData` struct with an `Equal` method. Acceptable as-is for single-digit RPM throughput. - **Effort:** Low **A30: Document qcV1SignerHandoff schema** - **File:** `pkg/tbtc/covenant_signer.go:35` - **Issue:** The `Kind` constant serves as a version identifier but the downstream contract for handoff artifacts is implicit. - **Action:** Add doc comment noting this struct defines the downstream API schema. - **Effort:** Low **A3 (type-level): Add `HasRegistryData` to WalletChainData** - **File:** `pkg/tbtc/chain.go:418` - **Issue:** Zero `[32]byte` for MembersIDsHash means "unavailable" but this is implicit. Downstream guards work but future code could silently use the zero value. - **Action:** Add `HasRegistryData bool` field or use `*[32]byte` for MembersIDsHash to make unavailability explicit at the type level. - **Effort:** Medium (touches many callers) --- `TestSubmitHandlerPreservesContextValues` fails on the base branch (`fix/review-findings`). Confirmed not introduced by these changes -- the test expects request context values to propagate through the service context detachment, which is not the current design. - [x] `go build ./pkg/covenantsigner/...` and `go build ./pkg/tbtc/...` compile cleanly - [x] All covenantsigner tests pass (except pre-existing `TestSubmitHandlerPreservesContextValues`) - [x] All relevant tbtc tests pass (GetWallet fault isolation, signer approval, payload hash) - [x] Pinned hash test (`TestComputeQcV1SignerHandoffPayloadHash_DeterministicKeyOrdering`) still passes after canonicaljson switch - [ ] CI green
piotr-roslaniec
approved these changes
Apr 16, 2026
lrsaturnino
merged commit 245901f
into
feat/psbt-covenant-final-project-pr
17 checks passed
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
2 participants
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.
Problem
PR #3935 was merged into
fix/review-findings, an intermediate base branch that was never integrated intofeat/psbt-covenant-final-project-pr. This leftfeat/psbt-covenant-final-project-prwithout the audit-finding fixes that #3935 implemented (createOrDedup mutex extraction, healthz auth bypass restriction, canonical JSON for handoff payload, error message improvements, gosec hardening, and more).Solution
This PR cherry-picks the squash commit of #3935 onto the current state of
feat/psbt-covenant-final-project-pr, which already contains PR #3933 (Maclane's fault-isolation work plus his subsequent fixes for the review comments raised on that PR). Three files required manual conflict resolution where #3935 and #3933 made overlapping changes addressing the same concerns. Inpkg/tbtc/signer_approval_certificate.go, theErrMissingWalletIDandErrMissingMembersIDsHashsentinel errors from #3935 were dropped because Maclane'sensureWalletRegistryDataAvailablefunction already covers the same case and is in active use; keeping both would have left the sentinels as dead code. Inpkg/covenantsigner/server_test.go, the test rewrite from #3935 (which depended on aserviceCtxparameter tonewHandler) was reverted in favor of the existing middleware-based test, which already exercises thecontext.WithoutCancel(r.Context())detachment path. Inpkg/covenantsigner/store_test.go, the test rename collision was resolved by taking #3935's name (TestStoreLoadResolvesInvalidUpdatedAtForDuplicateRouteKeys).Three additional cleanups followed from the merge mechanics: the godoc on
WalletChainData.MembersIDsHashwas updated to referenceensureWalletRegistryDataAvailableinstead of the dropped sentinels, an orphanedcancelService()call was removed fromserver.go(itsserviceCtxinfrastructure did not auto-merge), and a duplicatedelete(s.byRequestID, existingID)was deduplicated instore.go's load path.Tests
All tests across
pkg/covenantsigner,pkg/tbtc, andpkg/chain/ethereumpass. A behavioral diff against the base branch shows identical build, vet, and file-listing outputs, with alloklines matching except for timing variation. The two adapter tests added by Maclane (TestMakeWalletChainData...) and the two registry-unavailable tests for the issue-and-verify signer approval paths are preserved and pass.