fix: implement deep-audit findings for covenant signer#3938
Merged
piotr-roslaniec merged 40 commits intoMay 23, 2026
Merged
Conversation
piotr-roslaniec
force-pushed
the
fix/deep-audit-findings
branch
from
3d55760 to
1b68176
Compare
lrsaturnino
reviewed
Apr 17, 2026
Member
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
Flagging a compile-breaking reference found during a deep review of this PR — see the inline comment on pkg/covenantsigner/store.go:358. Not requesting changes, just bringing it to attention.
|
|
||
| s.byRequestID[job.RequestID] = cloned | ||
| s.byRouteKey[key] = job.RequestID | ||
| delete(s.poisonedRoutes, key) |
Member
There was a problem hiding this comment.
s.poisonedRoutes isn't a declared field on Store — the struct definition (lines 18–25 of this file) declares only handle, mutex, lockFile, byRequestID, and byRouteKey, and no errPoisonedRouteKey sentinel or poison-tracking infrastructure exists anywhere in the PR. Repo-wide git grep -n 'poisonedRoutes\|errPoisonedRouteKey' on origin/fix/deep-audit-findings returns only this one line.
Empirically from the head ref:
go build ./pkg/covenantsigner/...→ exits 1 withpkg/covenantsigner/store.go:358:11: s.poisonedRoutes undefined (type *Store has no field or method poisonedRoutes)go build ./pkg/tbtc/...andgo build ./cmd/...transitively fail (both importpkg/covenantsigner)go test ./pkg/covenantsigner/...→ build-failedgo vet ./pkg/covenantsigner/...→ same undefined-field error
On the base ref (origin/fix/review-findings) all of the above succeed.
Commit f54d4e4d4 (the A34 fix) is a single-line insertion of this delete(...) call with no accompanying struct-field declaration or producer-side poison-setting code — looks like a rebase may have dropped the foundation commit.
Flagging only, not a change request.
lrsaturnino
reviewed
Apr 17, 2026
Member
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
Just a double-check: Is this PR intended to merge with fix/review-findings or main?
Collaborator
Author
|
@lrsaturnino It should target |
piotr-roslaniec
added a commit
that referenced
this pull request
Apr 17, 2026
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.
piotr-roslaniec
force-pushed
the
fix/deep-audit-findings
branch
from
1b68176 to
45428c9
Compare
lrsaturnino
approved these changes
Apr 23, 2026
piotr-roslaniec
changed the base branch from
main
to
feat/psbt-covenant-final-project-pr
When the wallet registry is unavailable during fault-isolated GetWallet calls, signer approval verification now returns actionable errors that distinguish "registry unavailable" from "genuinely zero values". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ancellation Replace unbounded context.WithoutCancel in submitHandler with a service-level context that is cancelled on process shutdown and has a 5-minute timeout. This prevents threshold signing goroutines from hanging indefinitely and ensures clean cancellation on container stop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a guard that fails the CI step if the -run regex silently matches zero tests, which would otherwise pass without actually testing anything. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… context The test injected a value into the HTTP request context and expected it to be visible in the engine, but the submit handler deliberately derives its context from the service context (not the request context) to survive HTTP disconnects. Fix the test to inject the value into the service context, which matches the actual design contract.
…me key A malformed file processed before its valid sibling would poison the route key, then the valid job would load into byRouteKey but remain inaccessible via GetByRouteRequest because the poison check ran first. Clear the poison entry when a valid job is successfully indexed for that route key.
…ersisted job files The poison-route mechanism (A22) attempted graceful degradation by skipping corrupt files and marking their route keys as poisoned. This had two fundamental issues: (1) an ordering-dependent bug where a valid job iterated before its malformed sibling left the route permanently poisoned despite a valid job being loaded, and (2) when the corrupt file's route key was unrecoverable (binary garbage), no poison was set and dedupe protection was silently lost. For a pre-mainnet Bitcoin signing service, fail-closed is the safer default: ambiguous persisted state is a safety fault, not a recoverable nuisance. A single corrupt file should block startup rather than risk duplicate signing jobs. Changes: - Store.load() now returns a hard error on unreadable or malformed persisted job files instead of skipping them - Removed poisonedRoutes map, errPoisonedRouteKey sentinel, poisonRouteFromPartialJob, SkippedJobFiles, and poison check in GetByRouteRequest - Updated tests to expect hard failure on corrupt files - Added superseded-job-removal assertions to dedup tests
…e operations Add #nosec annotations for three findings in store.go: - G304 on os.OpenFile: lockPath is built from operator config (dataDir) plus constants (jobsDirectory, lockFileName), not user input - G104 on lockFile.Close(): best-effort cleanup after failed flock; the lock error is what gets returned - G104 on store.Close(): best-effort lock release after failed load; the load error is what gets returned
…claration gosec flags the `go func()` declaration line, not the `context.Background()` call inside the body. Move the annotation to the correct line so the client-scan CI job passes.
… route key replace
…igrationDestination
…ots KeyID whitespace
…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.
piotr-roslaniec
force-pushed
the
fix/deep-audit-findings
branch
from
54dae9b to
6af268d
Compare
piotr-roslaniec
merged commit 558f4bc
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.
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
DataDirfromstorage.Dirwhen 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.context.Background()withsignal.NotifyContext(SIGINT, SIGTERM)incmd/start.go. The shutdown goroutine (HTTP drain, cancel signing ops, release file lock) was dead code -- it blocked onctx.Done()which never fired.byRequestIDentries on route-key replacement inPut(), and add route-key holder verification inloadPollJob. 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
Put(). Poison was self-reinforcing at runtime becauseSubmithitGetByRouteRequest(which returnserrPoisonedRouteKey) before reachingPut().migrationPlanQuoteTrustRootsKeyID at startup, matching the pattern for depositor/custodian roots. Config with accidental whitespace in KeyID caused immediate request rejection.NewStoresucceeds, using a deferred cleanup on named error return.P3 Fixes
destination.ReservationIDinnormalizeMigrationDestination, matchingnormalizeMigrationPlanQuote. Asymmetric trim caused spurious rejection for whitespace-padded IDs.Audit methodology
Test plan
go build ./cmd/... ./pkg/covenantsigner/... ./pkg/tbtc/...compiles cleanlygo test -race ./pkg/covenantsigner/...passes