feat(tbtc): covenant signer final project branch#3882
Open
mswilkison wants to merge 140 commits into
Open
Conversation
…rmalization via generic helper
… a signer approval verifier
…and qc_v1 UTXO resolution
…lock acquisition paths
Adds the poisonedRoutes map field that was referenced in A34 fix (commit f54d4e4) but never declared in the struct definition. Without this field, the store fails to compile: s.poisonedRoutes undefined (type *Store has no field or method poisonedRoutes) Fixes compile error reported in PR #3938 review comment.
P0-1: Add signer approval expiration check via EndBlock field.
validationOptions gains currentBlock field; at policyIndependentDigest
true, checks EndBlock expiration and falls through to re-verification.
P0-2: Persist poisoned route markers to disk for recovery across restarts.
store.go adds poisonedDirectory, MarkPoisoned writes marker files,
and load() restores state on startup.
P1: Add concurrency limit for in-flight submit/poll operations.
Service gains maxInFlight field and semaphore channel; slot acquisition
added in both Submit() and Poll() paths.
P2: Check EndBlock expiration in loadPollJob before digest comparison.
Uses currentBlockProvider to verify approval has not expired.
P3: Add poll rate limiting alongside existing submit rate limit.
pollLimiter uses golang.org/x/time/rate at 60/min; handlers updated
to check Allow() before processing.
Engine: Add CurrentBlockHeightProvider as separate optional interface;
passiveEngine stub added; WithCurrentBlockProvider refactored to
auto-detect interface from Engine; nil guard added in Poll().
Prior to this change, WithCurrentBlockProvider silently returned 0 when the underlying provider returned an error. This caused signer approval expiration checks to pass incorrectly during RPC failures, treating expired approvals as valid. After this change, the provider returns (uint64, error). Both call sites (loadPollJob and Poll) now propagate errors instead of suppressing them.
Remove redundant inner if block in loadPollJob's expiration check. The outer && already guarantees job.Request.SignerApproval != nil, making the inner nil check on EndBlock unreachable after the short-circuit. Also adds a doc comment to WithMaxInFlight explaining that n <= 0 disables the concurrency limit entirely.
Remove the MarkPoisoned function, poisonedRoutes map, and related poisonedDirectory constant. The feature was added in a prior commit but is never called from any code path, leaving the scaffolding unused.
…BlockErr field Extends scriptedEngine to support the SignerApprovalVerifier interface and inject currentBlockHeight provider errors for comprehensive testing of error propagation paths. - Add signerApprovalVerifier field to scriptedEngine - Add currentBlockErr field to scriptedEngine - Add VerifySignerApproval method delegating to inner verifier - VerifySignerApproval returns nil when inner verifier is nil (no-op)
When a signer approval certificate is expired and no verifier is present in the validation options (Poll flow), return the expiration error directly instead of falling through to the re-verification path that requires a signerApprovalVerifier. This ensures TestServicePollRejectsExpiredCertificate correctly returns 'signer approval certificate has expired' instead of 'request.signerApproval cannot be verified by this signer deployment'.
Use explicit engine variable and WithCurrentBlockProvider in tests that need to verify block-height-dependent validation, for clearer block height control.
…fier interface scriptedEngine implicitly implemented SignerApprovalVerifier via its signerApprovalVerifier field and VerifySignerApproval method. This caused NewService to auto-wire a signerApprovalVerifier on every test service, triggering the validation guard that requires SignerApproval in the request whenever a verifier is configured. Tests using baseRequest (no SignerApproval) then failed before calling the engine, hanging goroutine synchronization channels indefinitely. Remove VerifySignerApproval and the signerApprovalVerifier field from scriptedEngine. Tests that need a signer approval verifier now pass it explicitly via WithSignerApprovalVerifier, making the intent clear.
golang.org/x/time was marked indirect in go.mod even though pkg/covenantsigner/server.go imports golang.org/x/time/rate directly for submit-endpoint rate limiting. Running go mod tidy removes the stale // indirect annotation so the dependency graph matches the actual import surface.
…-project-pr Two pieces dropped when skipping conflicting intermediate commits: - cancelService() on listener bind error in covenantsigner.NewServer: prevents service-context goroutines from leaking when the HTTP listener fails to bind during startup. Originally added in the now-skipped ef45c15 (which also added a signal handler later removed by 73c8863). - TestVerifySignerApprovalCertificateRejectsHighSSignature in signer_approval_certificate_test.go: defense-in-depth coverage proving the verifier rejects mathematically-equivalent high-S signatures (S' = N - S) that would otherwise enable signature malleability. Originally added by 54dae9b.
## Summary Implements 8 fixes from the deep audit of the covenant signer feature (20 agent passes across 5 analysis areas + cross-provider verification with Gemini and GPT models). ### P1 Fixes - **A31**: Auto-derive `DataDir` from `storage.Dir` when unset and signer port is non-zero. Without this, the exclusive file lock was silently disabled, allowing two processes to corrupt the job store concurrently. - **A32**: Replace `context.Background()` with `signal.NotifyContext(SIGINT, SIGTERM)` in `cmd/start.go`. The shutdown goroutine (HTTP drain, cancel signing ops, release file lock) was dead code -- it blocked on `ctx.Done()` which never fired. - **A33**: Unconditionally evict stale `byRequestID` entries on route-key replacement in `Put()`, and add route-key holder verification in `loadPollJob`. Previously, a failed file delete left an orphan that a stale Poll could use to seize the route key from a newer job. ### P2 Fixes - **A34**: Clear poisoned route keys on successful `Put()`. Poison was self-reinforcing at runtime because `Submit` hit `GetByRouteRequest` (which returns `errPoisonedRouteKey`) before reaching `Put()`. - **A36**: Normalize `migrationPlanQuoteTrustRoots` KeyID at startup, matching the pattern for depositor/custodian roots. Config with accidental whitespace in KeyID caused immediate request rejection. - **A38**: Release file lock if trust root normalization fails after `NewStore` succeeds, using a deferred cleanup on named error return. ### P3 Fixes - **A35**: Trim whitespace from `destination.ReservationID` in `normalizeMigrationDestination`, matching `normalizeMigrationPlanQuote`. Asymmetric trim caused spurious rejection for whitespace-padded IDs. - **A37**: Enforce low-S normalization on signer approval certificate threshold signature. Without this, a certificate holder could submit the high-S variant to create a duplicate job (doubled resource consumption, no fund loss). ### Audit methodology - 20 agent runs (4 passes x 5 areas: coverage, contract verification, threat model, property testing, integration seams) - Cross-provider verification: Gemini 3 Pro Preview (security), Gemini 2.5 Pro (integration), GPT-5.2 (state layer) - Multi-agent consolidated review for final severity calibration - All 7 Bitcoin P2WSH spending paths verified correct via script execution traces - No P0 (fund-loss) vulnerabilities found ## Test plan - [x] `go build ./cmd/... ./pkg/covenantsigner/... ./pkg/tbtc/...` compiles cleanly - [x] `go test -race ./pkg/covenantsigner/...` passes - [x] Relevant tbtc tests pass (GetWallet, signer approval, payload hash) - [ ] CI green
…val expiry
Signer-approval certificates carried an EndBlock that production never enforced
and that the threshold signature never covered, so a caller could omit or
replace it without invalidating the certificate. Production also installed no
block-height provider, so expiry was compared against a zero block.
Introduce certificate v2, which binds the expiration block into the signature:
- The tECDSA signature now signs a domain-separated digest
SHA256("covenant-signer-approval-certificate-v2:" || approvalDigest ||
uint64_be(endBlock)) instead of the bare approval digest. Verification
reconstructs this digest, so altering either the approval digest or the
expiration block fails verification.
- EndBlock is required and is the inclusive last valid host-chain (Ethereum)
block; a certificate is expired only once currentBlock > EndBlock. Certificate
v1 and a missing EndBlock now fail closed.
- Issuance takes an explicit future expiration block chosen before signing and
refuses to issue if signing completes after it.
Wire the production host-chain block provider: covenantSignerEngine implements
CurrentBlockHeight against node.chain (the Ethereum anchoring chain), and
NewService auto-detects it so correctness no longer depends on callers passing
an option.
Make enforcement unconditional and fail closed:
- Submit resolves the current host-chain height up front, rejects an expired
certificate before signature verification and before any engine work, and
fails closed when the provider is unavailable or errors.
- Poll validates the resubmitted request and rechecks the stored job both before
and after OnPoll, catching expiry reached while signing is in flight.
- Server startup rejects an engine that verifies approvals but cannot provide a
block height.
This redefines EndBlock from a signing-completion block to an explicitly chosen
future expiry, and intentionally provides no v1 migration path: v1 fails closed.
External producers/consumers must adopt certificate v2, the exact digest
encoding, and the Ethereum height domain before enforcement is deployed. A
cross-language signing-digest vector is added to the contract testdata.
…Submit signing and release store lock on init failure Submit fetched the host-chain height once up front and enforced certificate expiry only during that initial validation. Because OnSubmit performs synchronous threshold signing that can span multiple blocks, a certificate valid at submit time (currentBlock == EndBlock) could expire while signing was in flight, yet the resulting artifact was still persisted and returned. Recheck certificate timeliness in Submit at two points, mirroring Poll's handling of OnPoll: immediately before OnSubmit (after verifier work and any in-flight-slot wait), so engine signing never starts under an authorization that expired during validation; and again after OnSubmit, before applying, persisting, or returning its result. Both rechecks reuse ensureStoredCertificateTimely and fail closed on a missing or errored block-height provider. Also release the store's advisory file lock when covenant-signer initialization fails after the service was created. The startup rejection for an engine that verifies approvals without providing a block height -- and every other post-NewService error path -- returned without closing the store, leaking the lock and blocking a corrected retry in the same process. Add tests covering expiry crossed during the verifier and during OnSubmit, Poll acceptance strictly before EndBlock, and a successful Initialize retry proving the store lock is released after a rejected startup.
… persisting Submit result Submit's post-OnSubmit timeliness recheck ran before s.mutex.Lock(), leaving a window in which the host chain could advance past the certificate's EndBlock while this submit blocked acquiring the mutex (for example behind a concurrent Poll holding it during its own block-height RPC). On acquiring the lock Submit would find its snapshot unchanged and persist and return the ready artifact using a timeliness check that had gone stale, honoring an authorization that had since expired. Keep the pre-lock recheck as a fast-path early exit, and add an authoritative recheck under s.mutex immediately before applying and persisting the transition so the check is atomic with the Put. This mirrors Poll, whose post-OnPoll recheck already runs under the mutex via loadPollJob. The in-lock recheck re-fetches the height and fails closed on a missing or errored provider.
|
Important Review skippedToo many files! This PR contains 52 files, which is 2 over the limit of 50. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (52)
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 |
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
3 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.
Summary
This PR introduces the covenant signer — a Go HTTP service that signs Bitcoin PSBTs (Partially Signed Bitcoin Transactions) using each operator's existing tECDSA wallet share from DKG. It is consumed by the off-chain covenant tooling (in
tbtc-v2-ac) when a covenant migration or recovery operation needs an operator-side signature on a constructed PSBT.Repo footprint: ~9.4K LoC of Go in
pkg/covenantsigner/plus integration glue acrosspkg/tbtc/,pkg/chain/ethereum/,pkg/bitcoin/,cmd/, andconfig/. 48 files changed, +13,574 / −30.Audit positioning
This PR is the secondary review target for the Trail of Bits PSBT covenant audit. The primary review covers the on-chain Solidity contracts in
tlabs-xyz/tbtc-v2-ac. The current SOW miscalls this PR an "off-chain attester / watchdog service" — that wording is incorrect:ATTESTER_ROLEand noWATCHER_ROLEon-chain.WatchdogEnforcer.sol(the actual permissionless watchdog) is already merged ontbtc-v2-ac/dev. It lives intbtc-v2-ac, not here.ac-watchdog-service(Node.js) lives intlabs-xyz/ac-watchdog-service, not here.Where to start reading
Recommended file order:
pkg/covenantsigner/doc.go— package overviewpkg/covenantsigner/DEPLOYMENT.md— deployment topology, dedup scopepkg/covenantsigner/server.go— HTTP, bearer-token auth, timeoutspkg/covenantsigner/service.go— Submit/Poll, idempotency, request digestpkg/covenantsigner/validation.go— request validation, fuzz targetpkg/covenantsigner/types.go— request/response shapes,expiresAtpkg/covenantsigner/store.go— in-memory + on-disk persistencepkg/covenantsigner/engine.go— engine interfacepkg/tbtc/covenant_signer.go— concrete engine: BTC confirmations, PSBT signing, tECDSA share lookuppkg/internal/canonicaljson/marshal.go— canonical JSONTest vectors live at
pkg/covenantsigner/testdata/(covenant_recovery_approval_vectors_v1.json,migration_plan_quote_signing_vectors_v1.json).Defensive properties (cheat-card)
routeRequestIDper request: same ID + same body returns the prior result; same ID + different body is rejected; expired quote (expiresAt) is rejectedNewEncryptedProtectedPersistenceinpkg/storage/storage.goOut-of-scope by design
This service signs PSBTs and returns them in HTTP responses. It does not submit Ethereum transactions: grep for
nonce,SendTransaction, orTransactionManagerin this PR returns nothing. Nonce safety, transaction replay, and RPC-outage handling are concerns of the off-chainac-watchdog-service(a separate canonical repo) — not this PR.Cross-repo coupling
This PR binds against contract ABIs from
tlabs-xyz/tbtc-v2-acviapkg/chain/ethereum/tbtc.go. After the on-chain Solidity stack lands (PR #263 → #280 → #281 / #282 →dev→main), the Go bindings here will need regeneration against the merged ABIs. The current bindings tracktbtc-v2-ac/feat/ac-031-psbt-covenant-importas of the most recent refresh.CI surface
gosecenabled —staticcheckenabled — unit + integration tests run on every PR.govulncheck— not yet wired in; planned addition.pkg/covenantsigner/validation.go); light given the input surface (HTTP JSON, PSBT, witness scripts).Dependency replacements (deliberate, pinned)
tss-lib→ Threshold-Network fork.keep-common→tlabs-xyzfork.Both forks are intentional; auditors are encouraged to review the forks commit-by-commit alongside this PR.
Known limitations / liveness risks
Status
mainon 2026-05-10 (clean rebase: 93 commits replayed, 15 cherry-pick-equivalents auto-skipped, 0 conflicts).