fix(spv): re-verify reservation action generation before SPV proof submission by piotr-roslaniec · Pull Request #4276 · threshold-network/keep-core

piotr-roslaniec · 2026-09-01T13:59:16Z

Summary

Adds a submission-time re-check of a reservation action generation's state
in the reservation SPV proof loop (reservation_proof_loop.go), plus its
test coverage.

proveReservationAcceptanceActions/proveReservationReanchorActions check
that an action generation is Pending once near the top of their loop,
then run a Bitcoin transaction-history scan before reaching the submit
call - a window in which the generation could settle, time out, or be
superseded. submitReservationActionProof (reservation_reanchor_proof.go)
already re-fetches the action and rejects on type/state/target-wallet
mismatch immediately before SubmitReservationProof, so no incorrect or
misdirected proof can be submitted either way. What was missing was a way
to tell that expected, benign outcome apart from a genuine failure in the
logs and metrics: previously both fell through to submitReservationActionProof's
existing checks and surfaced identically. verifyReservationActionStillProvable
adds an earlier, dedicated check whose only observable difference is
operational: an expected staleness skip is now Warn-logged with the
actual observed action type/state (not just the expected one) and is
deliberately not counted as a failed submission attempt, while a genuine
chain-read error still propagates and a genuine logic error caught later
still increments the existing failure metrics.

Testing

TestVerifyReservationActionStillProvable (table-driven): happy path,
stale action generation, wrong action type, mismatched target wallet,
genuine chain-read error, and an absent/zero-value action generation.
Loop-level regression cases added to TestProveReservationAcceptanceActions/
TestProveReservationReanchorActions confirming the guard's skip path
is wired through the real loop functions (zero submissions for a stale
action generation).
go test -race ./pkg/maintainer/spv/... ./pkg/tbtc/...

Known CI issue (not caused by this PR)

client-build-test-publish fails at make generate (_address/ReservationRouter
target): @keep-network/tbtc-v2@development on npm does not publish a
ReservationRouter.json artifact for the ReservationRouter contract that
pkg/chain/ethereum/tbtc/gen/Makefile already declares as a required
contract. Confirmed present on the base branch (m1/keep-core-client)
itself, independent of any change in this PR. Tracked in
threshold-network/keep-core#4281.

No workaround is applied in this PR - three were evaluated and rejected:

Job-level continue-on-error on client-build-test-publish: doesn't
change the job's own reported check conclusion (still shows failed), and
unblocks client-integration-test into a new failure of its own (missing
Docker image artifact) that was previously a clean skip. Net worse.
Step-level continue-on-error on the failing build step: would make
the job report green, but that step also builds the image the same job's
go test ./... step runs against - skipping past it means the full
client Go test suite silently never runs while CI reports success, for
any future build breakage, not just this known npm gap. Too high a
blast radius for a temporary workaround.
Excluding ReservationRouter from codegen (required_contracts in
pkg/chain/ethereum/tbtc/gen/Makefile): pkg/chain/ethereum/tbtc.go
actively constructs and uses tbtccontract.ReservationRouter in
production code (reservationRouterBinding, RequestReservationReanchor),
so the generated bindings are load-bearing, not unused scaffolding.
ReservationRouter's address file specifically is unused (its binding
is constructed against the Bridge address, not its own - see gen.go's
//go:embed list, which never embeds a ReservationRouter address), so
excluding just that piece is safe, but the ABI/contract bindings still
need real ABI JSON that the npm package doesn't ship at all, and the
shared clean: target in pkg/chain/ethereum/common/gen/Makefile
(used by every contract package: ecdsa, threshold, random-beacon, tbtc)
unconditionally wipes abi/*, contract/*, and cmd/* before rebuild,
which would delete the already-correct, currently-committed
ReservationRouter bindings with nothing left to regenerate them. A
real fix means reworking that shared clean step to be selective
per-contract - out of scope for this PR, affects codegen for every
contract package in the repo, and needs its own review.
Excluding ReservationRouter from codegen, restoring committed
bindings from git afterward: verified empirically infeasible, not just
theoretically. .dockerignore excludes **/gen/**/*.go (only gen.go
and cmd/cmd.go are kept) and excludes .git (.* at the top) from
the Docker build context - the already-committed ReservationRouter
bindings never reach the image in the first place, and there is no git
history inside it to check out from. Reproduced the exact CI failure
locally (make get_artifacts && cd pkg/chain/ethereum/tbtc/gen && make environment=development) to confirm clean deletes them before the
missing-artifact error halts the build.
Vendoring a reconstructed ReservationRouter.json sourced from the
ABI already embedded in the committed abi/ReservationRouter.go
(ReservationRouterMetaData.ABI): also verified infeasible by round-trip
test. That embedded string has already lost every struct internalType
qualifier ("struct BitcoinTx.Info" became "structBitcoinTx.Info")
during the original abigen run, so regenerating from it produces
different, wrong type names (Struct0/Struct2/Struct4 instead of
BitcoinTxInfo/BitcoinTxProof/ReservationReservationRequest) that
would not match what pkg/chain/ethereum/tbtc.go expects. Confirmed via
a real abigen run and diff against the committed file - not
byte-identical. Hand-authoring the missing internalType values back in
would mean fabricating artifact content that can only legitimately come
from an upstream solc compile of the real ReservationRouter.sol.

Update: a fifth approach - a development-only vendored fallback for
just the wholly-missing ReservationRouter.json - turned out to be
genuinely viable and is landed (pkg/chain/ethereum/tbtc/gen/Makefile +
ReservationRouter.fallback-artifact.json). Its ABI is byte-for-byte
verified: re-derived from the already-committed ReservationRouterMetaData.ABI
with the internalType prefix space abigen's metadata packer strips put
back (a deterministic, checkable normalization, not fabricated data), then
round-tripped through the same abigen + keep-common generator invocation
CI uses and diffed byte-identical against the committed
abi/ReservationRouter.go, contract/ReservationRouter.go, and
cmd/ReservationRouter.go. Confirmed on CI: make generate now completes
successfully for all four contract packages (tbtc, threshold, beacon,
ecdsa), ~40s further than before.

That unmasked a second, larger external gap at the actual go build
step: Bridge and WalletProposalValidator's live npm ABI has zero
reservation-related methods (ValidateReservationAnchorProposal,
ValidateReservationReanchorProposal, IsReservedDeposit all missing),
while the committed Go bindings for those two existing contracts already
expect them. Same root cause as ReservationRouter (tbtc-v2's npm
development tag hasn't caught up with this reservation feature's
Solidity changes) but a different, riskier shape to fix: this is merging
specific missing methods into a live-fetched ABI shared with dozens of
unrelated methods on those two contracts, not swapping out one wholly
self-contained missing artifact. Not attempted - no clean byte-identical
verification path the way ReservationRouter had, and touches two
contracts with much larger surface area. client-build-test-publish stays
red on this second gap.

Resolved. Fixed with a second vendored-fallback patch
(pkg/chain/ethereum/tbtc/gen/Makefile + Bridge.reservation-methods-fallback.json

WalletProposalValidator.reservation-methods-fallback.json): the fetched
npm artifact is patched in place (development only, only when the methods
are actually missing) by merging in vendored method fragments extracted
from the committed BridgeMetaData.ABI/WalletProposalValidatorMetaData.ABI
(same internalType prefix-space restoration as ReservationRouter's
fallback). Verified with a full clean end-to-end run - fresh npm fetch,
fresh make generate for all four contract packages, go build ./...,
go vet ./..., and the full go test ./... suite (1887 tests, 89
packages) - before landing.

CI is fully green: 12/12 checks pass, including client-build-test-publish
and client-integration-test (previously always skipped on this PR,
now running and passing for the first time). Both external gaps tracked
in #4281 are worked around in this PR; no
remaining CI issue caused by or blocking this PR.

Replace the placeholder getter/no-op submitter for tbtc.ActionReservationReanchor with a real implementation: - getUnprovenReservationReanchorTransactions discovers unproven re-anchor Bitcoin transactions by walking ReservationReanchorRequested events, skipping settled/timed-out action generations, and matching candidate transactions against the still-registered anchor outpoint via ReservationByAnchorUtxo. - reservationReanchorTransactionProofSubmitter re-derives the (reservationKey, requestNonce) pair the generic proof-loop signature cannot carry, then submits via the existing SubmitReservationReanchorProof path. Extends the spv.Chain interface with PastReservationReanchorRequestedEvents and ReservationByAnchorUtxo (already present on TbtcChain); adds matching localChain test fakes. Reservation acceptance proof submission remains a documented placeholder pending its own watcher integration - out of scope here. Covers the discovery precision paths (shape mismatch, anchor mismatch, settled-action skip) and the submitter's nonce/key derivation with new unit tests.

Replace the placeholder Run() (guard checks only, no loop) with a real background poller: - WatchWallet registers a wallet public key hash for polling; dedupes registrations under a mutex-protected set. - Run(ctx) now blocks, checking every watched wallet immediately and then every poll interval, until ctx is done. Each iteration walks each watched wallet's reservations (WalletReservations) and calls the existing CheckReservationActionTimeouts per reservation. A failure checking one wallet is logged and does not abort the iteration or stop the loop. - startActionTimeoutRun (reservation_wiring.go) now threads ctx into Run(ctx) instead of discarding it; context.Canceled is treated as the expected shutdown path, not a failure to log. Run's signature changes from Run() to Run(ctx context.Context); the only call site (startActionTimeoutRun) is updated. Wallet discovery (who calls WatchWallet with which wallets) remains a separate, pre-existing gap shared by all three reservation watchers - see the 'PR H placeholder' comments on subscribeReservationWalletClosed and subscribeReservationActionTimedOut - and is out of scope here. Covers WatchWallet dedup, all three Run precondition guards, and an end-to-end test that Run notifies a timed-out action on its first (immediate) iteration and returns promptly on ctx cancellation.

coderabbitai · 2026-09-01T13:59:24Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.}

…e proof submission submitDiscoveredReservationReanchorProof previously re-derived the submission nonce by reading the reservation's current RequestNonce. That field tracks the reservation's live action generation, which can have moved on since the discovered transaction was built - e.g. the original re-anchor action times out and a new, unrelated action generation becomes current while the SPV maintainer is still waiting out requiredConfirmations on the old transaction. Submitting the live nonce in that case pairs a stale, unrelated transaction with the wrong action generation. Fix: before submitting, fetch the action generation at the reservation's current nonce and require it to still be a Pending Reanchor action targeting the exact wallet the discovered transaction actually pays. Any mismatch is reported as an error so the proof loop treats the transaction as not-yet-submittable instead of silently misattributing the proof. Adds two regression tests: one for the action-no-longer-pending case, one for the pending-but-different-target-wallet case.

The prior comment framed the race as spanning proveTransactions waiting out requiredConfirmations. In fact the getter and submitter run back-to-back within the same proveTransactions call for a given transaction (spv.go:229 getter, :286 submitter); under-confirmed transactions are skipped and re-discovered on the next tick, not held. The staleness window is the narrow same-call gap between the getter's per-event Pending check and the submitter call, not a multi-block confirmation wait. The fix itself (verify the current action generation before submitting) is unchanged and still correct - only the severity/likelihood framing in the comment was wrong.

… nonce An error returned from transactionProofSubmitter propagates out of proveTransactions (spv.go:292-293), aborting the entire proving round for every other in-flight transaction across every proof type that tick, then restarting the whole SPV maintainer after the backoff. That is disproportionate for the two mismatch branches added in the prior commit (stale/superseded action generation, mismatched target wallet): both are an expected, if rare, outcome of a narrow same-tick race, not an infrastructure failure. Both branches now log a warning and return nil instead of an error, so proveTransactions treats the transaction as handled and moves on to the next one - it will simply not be rediscovered on the next tick since its action generation is no longer Pending. Flips both regression tests to assert a nil error and that the submission hook was not called, matching the corrected behavior.

Both skip branches previously claimed the transaction 'will simply not be rediscovered' on later ticks. That relied on an unverified assumption - that the Bridge allows at most one Pending action per reservation at a time - which is asserted nowhere in this Go client and could not be confirmed against the on-chain source for this action-generation model (not available in the local tbtc-v2 checkout). If that assumption is false, getUnprovenReservation- ReanchorTransactions' per-event Pending check would keep returning the same transaction and both branches would log the same warning every tick. Replaced with the honest, verifiable termination condition: this outpoint stops matching once the reservation's current generation lands its own correct re-anchor proof, at which point the existing 'no reservation is anchored at the spent outpoint' branch takes over instead. No behavior change - comment accuracy only.

Adopts the human-authored reservation watcher/proof-submission design that landed directly on m1/keep-core-client (commits 20cb160, 1de2631, 753d717, cb8ad1a) in parallel with this branch's own Row1/Row2 work, superseding it: their dedicated reservation-proof loop covers both acceptance and re-anchor proof submission (this branch's adapter only covered re-anchor), and their timeout-watcher rewrite replaces the admitted Run() placeholder the same way this branch's did. Conflict resolution: - pkg/maintainer/spv/{chain,chain_test,reservation_action_timeout_watch, reservation_action_timeout_watch_test,reservation_wiring,spv}.go: took the human's version wholesale (superseding design). - pkg/maintainer/spv/reservation_reanchor_proof*.go: took the human's version. Their commit only added a 9-line bounds-check fix on top of the pre-existing core proof-assembly primitive (SubmitReservation- ReanchorProof et al., already part of PR #4274's original scope, not this branch's contribution) - this branch's own addition was the 325-line generic-loop discovery adapter, now dead since the human's design discovers via events instead. Both PR H's original acceptance- proof file and the new dedicated loop call directly into the unmodified primitive, so it could not be deleted outright. Ported forward: added verifyReservationActionStillProvable to reservation_proof_loop.go plus dedicated tests - a submission-time re-check of the action generation's Pending state and target wallet, mirroring the safety margin this branch's now-dropped adapter had (submitDiscoveredReservationReanchorProof's stale-nonce/mismatched- wallet guard) that the human's design does not have on its own. Wired into both proveReservationAcceptanceActions and proveReservationReanchorActions.

Adopts the human's incremental-scan reservation proof loop redesign (persistent per-pass event cursor, walletEvent dedup wrapper pattern, map-based pending-action tracking replacing full-window rescans every poll) and re-ports the immediate-pre-submit staleness guard (verifyReservationActionStillProvable) on top of it, unchanged from the prior merge's design. Guard closes the window between the loop's top-of-pass Pending check and the actual SPV proof submission, after a Bitcoin transaction-history scan and proof assembly in between. Concatenated both sides' test files (theirs: scan-range arithmetic, transaction-finding, transaction-proving, full integration tests for both action loops; ours: five guard-specific unit tests) - zero overlapping coverage. Fixed one test-fixture gap surfaced by the guard: TestProveReservationReanchorActions's event fixture never set TargetWalletPublicKeyHash (dead field before the guard existed); populated it to match the fixture's registered action. Full repo: 49 packages, zero FAIL.

Lets callers log the actual observed action type/state instead of a bare uint8 via %v.

…rage verifyReservationActionStillProvable's doc comment overclaimed that it closes a submission-time race; submitReservationActionProof's own pre-existing re-fetch+check (right before SubmitReservationProof) is what actually prevents an incorrect or misdirected submission. Rewrite the comment to state the guard's real purpose: distinguishing an expected, benign skip (Warn-logged, deliberately not counted as a failed submission attempt) from a genuine error. Fix the skip Warnf to report the actually observed action type/state (via the new String() methods) instead of only the caller-supplied expected type formatted with bare %v. Fix a false-success log: a guard skip inside the submit closure returned nil, so proveReservationTransaction always logged 'successfully submitted proof' even when nothing was submitted. Add a sentinel error (errReservationActionNoLongerProvable) so the skip path is distinguishable from both a real submission and a real failure. Extract the two submit closures into named submitReservationAcceptanceActionProof / submitReservationReanchorActionProof functions so the re-anchor closure's wallet-field selection (event.TargetWalletPublicKeyHash, not SourceWalletPublicKeyHash) can be unit-tested directly, without going through Bitcoin transaction discovery (this package's local chain test double can only discover a transaction via the source wallet's outputs, which forces source and target to coincide in any end-to-end test and so can't catch a field swap between them). Test changes: - Fix TestVerifyReservationActionStillProvable_WrongActionType's fixture leaving TargetWalletPublicKeyHash at its zero value, which let the wallet-mismatch branch mask the action-type branch under test. - Consolidate the five near-duplicate TestVerifyReservationActionStillProvable_* tests into one table-driven TestVerifyReservationActionStillProvable, add a case for a genuine chain-read error (via the new localChain.getReservationActionErr injection field, instead of relying on 'no action installed' as an implicit error trigger) and a case for an absent/zero-value action (matching what the real chain adapter actually returns for a never-set entry). - Remove stale/inaccurate doc comments (dangling references to a 'prior design' and 'generic-loop adapter' that don't exist in this repo; a claim that a propagated error would abort the whole proving pass, when call sites always log-and-continue per event). - Add loop-level regression cases to TestProveReservationAcceptanceActions / TestProveReservationReanchorActions asserting zero submissions when the action is no longer pending at submission time. - Add TestSubmitReservationReanchorActionProof_UsesTargetWallet, which calls the extracted function directly (bypassing discovery) with a genuinely distinct source/target wallet, to catch a regression that swaps the two fields at the call site.

…ervation-readiness-fixes # Conflicts: # pkg/maintainer/spv/reservation_proof_loop.go # pkg/maintainer/spv/reservation_proof_loop_test.go

…4281)" This reverts commit 0a427b6.

…dev package @keep-network/tbtc-v2@development does not publish ReservationRouter.json, so make generate fails outright (#4281). Add a development-only fallback that supplies a vendored copy of the ABI when the real artifact is missing, verified byte-identical to the currently committed bindings by round-tripping through the same abigen + keep-common generator invocation. Non-development builds are unaffected and still hard-fail on a missing artifact.

…th missing reservation methods @keep-network/tbtc-v2@development publishes Bridge.json and WalletProposalValidator.json, but both are stale relative to this reservation feature: missing isReservedDeposit (Bridge) and validateReservationAnchorProposal/validateReservationReanchorProposal (WalletProposalValidator), which the committed Go bindings already call. Patch the fetched artifact in place (development only, only when actually missing) with vendored method fragments extracted from the committed MetaData.ABI, verified by a full clean end-to-end run: fresh npm fetch, fresh make generate, go build/vet/test across the whole repo all pass (1887 tests, 89 packages).

piotr-roslaniec added 2 commits September 1, 2026 13:46

piotr-roslaniec added 4 commits September 1, 2026 14:10

This was referenced Sep 1, 2026

test(tbtc): add reservation proposal marshaling coverage #4277

Draft

fix(tbtc): wire reservation actions into the coordination checklist #4278

Draft

test(tbtc): multi-signer simulated integration test for reservation coordination #4279

Draft

piotr-roslaniec added 2 commits September 2, 2026 06:06

piotr-roslaniec changed the title ~~feat(spv): wire reservation re-anchor proof discovery and action-timeout poll loop~~ fix(spv): re-verify reservation action generation before SPV proof submission Sep 2, 2026

piotr-roslaniec added 5 commits September 2, 2026 13:04

feat(tbtc): add String() methods for ReservationActionType/State

9f09fb3

Lets callers log the actual observed action type/state instead of a bare uint8 via %v.

Merge remote-tracking branch 'origin/m1/keep-core-client' into m1/res…

f5c5e35

…ervation-readiness-fixes # Conflicts: # pkg/maintainer/spv/reservation_proof_loop.go # pkg/maintainer/spv/reservation_proof_loop_test.go

ci(client): tolerate known tbtc-v2 npm ReservationRouter gap (#4281)

0a427b6

Revert "ci(client): tolerate known tbtc-v2 npm ReservationRouter gap (#…

7210a7c

…4281)" This reverts commit 0a427b6.

piotr-roslaniec mentioned this pull request Sep 2, 2026

Client CI build fails: tbtc-v2 development npm package missing ReservationRouter.json #4281

Open

piotr-roslaniec added 2 commits September 2, 2026 16:23

Navigation Menu

Uh oh!

fix(spv): re-verify reservation action generation before SPV proof submission - #4276

fix(spv): re-verify reservation action generation before SPV proof submission#4276
piotr-roslaniec wants to merge 15 commits into
m1/keep-core-clientfrom
m1/reservation-readiness-fixes

piotr-roslaniec commented Sep 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 1, 2026

Draft PR not reviewed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

piotr-roslaniec commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Known CI issue (not caused by this PR)

Uh oh!

coderabbitai Bot commented Sep 1, 2026

Draft PR not reviewed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

piotr-roslaniec commented Sep 1, 2026 •

edited

Loading