fix(reservations): address validated review findings on #4343 by piotr-roslaniec · Pull Request #4346 · threshold-network/keep-core

piotr-roslaniec · 2026-09-30T09:20:46Z

fix(reservations): address validated review findings on #4343

Context

#4343 merged into reservations-epic before its multi-lens review was acted on. That review raised 55 findings: 45 were confirmed (listed as 42) and 10 were dropped. This PR fixes the confirmed ones on top of the merge commit 76aaecdd3, whose code is identical to #4343's head.

It also corrects several claims in the #4343 description that did not hold. They are listed under "Corrections to #4343" below.

Changes

Re-anchor (`pkg/tbtcpg/reservation_reanchor.go`)

Re-anchor out of Closing source wallets, on both the request and the resume path. Solidity allows MovingFunds and Closing sources. Before, a wallet that entered Closing while it still held reservations could never finish closing. The below-dust notification stays MovingFunds-only.
At most one on-chain request per pass. The fee is estimated and the transaction assembled against the live reservationTxMaxFee before the request is sent. Any failure after a request is sent ends the pass. The mined-wait stops early if another requester's action takes the nonce.
Removed the in-memory in-flight map, the GetReservationReanchorRequestReceipt method and its receipt enum. RequestReservationReanchor returns error again. Every round decides from re-read chain state. A duplicate request after a restart or a leader change can still revert on-chain, which costs gas but is safe.
Skip reservations in re-anchor cooldown (tbtc.Reservation.ReanchorCooldownUntil is now converted), and skip reservations whose anchor is at or below txMaxFee + minAmount. Both cases revert on-chain.
The target search runs once per pass. Wallets that are full on count are excluded for the rest of the pass, and the smallest anchor that fits nowhere is remembered.
TestReservationRequestTimeoutSafetyMarginMatchesValidator pins the Go copy of the 7,200-second margin. The EVM harness pins the on-chain value.

Acceptance (`pkg/tbtcpg/reservation_acceptance.go`, `pkg/tbtc`)

Candidates now come from ReservationAcceptanceRequested events for the wallet. The scan covers the on-chain action timeout plus one day, in 10,000-block chunks.
Only confirmed Pending Acceptances targeting the wallet count toward the 50-candidate budget, and that check runs before any Electrum call. Before, never-requested or fake reserved reveals could use up the budget and starve real requests.
Requests made more than 30 days after the reveal are now proposed and signed:
- The proposer scans backward from the request block, bounded by the snapshotted term plus 7 days.
- The signer estimates the reveal block from RevealedAt and widens the window once if needed.
- Both use the new chunked helper tbtc.FindDepositRevealedEvent.
A fee-estimate or proposal error now skips that candidate instead of aborting the window.
Vault fee gauges are published once per Run and reuse the vault address that was already read. The gauge is renamed to reservation_vault_fee_reserve_tbtc_base_units, because its value is in 1e18 base units.

SPV maintainers (`pkg/maintainer/spv`)

The proof loop, the stale-deposit watcher and the action-timeout watcher share one first-scan rule:
- Start at the reservation activation block.
- Skip the startup scan on networks without an activation entry.
- Scan in 10,000-block chunks.
The stranding startup scan is chunked. It keeps its 30-day start, because wallets registered before activation can hold reservations, and it no longer falls back to scanning from genesis.
Progress is saved per chunk, so a failed chunk keeps the earlier work. A scan error no longer skips proving or checking items already tracked. Scan cursors stay 12 blocks behind the tip to cover shallow reorgs.
A TimedOut acceptance generation is evicted once its reservation is no longer Unknown. Before, it was retried for up to termSeconds after a newer generation settled.
Each reservation gets at most one proof per pass: Pending first, then TimedOut by descending nonce. This applies to both acceptance and re-anchor.
TimedOut re-anchor generations whose reservation cannot settle are evicted. Stranded ones are kept, because late settlement is allowed. Generations with nothing to prove back off up to 32 passes. The reservation is read once per generation per pass.
Stale-deposit deadlines are stored in the tracked entry. The unused snapshotRefundDeadline fallback is removed.
cmd/maintainer.go copies the Ethereum network into the SPV config through a tested helper.

CI, harness and tooling

TBTC_V2_REF is pinned to tbtc-v2 eec999aa, the merge of tbtc-v2#1161 into reservations-upgrade. The harness WalletProposalValidator.json was re-vendored from that same commit. The bindings and the four ABI fallbacks are unchanged and verified against it.
New CI check: the harness validator must match the compiled one. The ABI is compared exactly, and the creation and deployed bytecode are compared with the CBOR metadata stripped. This check fails on the old 9f8f5ef1 validator, which has the same ABI but different logic.
New harness cases against the real bytecode:
- timeout margin at +7200 (rejected) and +7201 (accepted);
- an anchor from a MovingFunds wallet;
- re-anchor target count at the cap and at cap + 1;
- an anchor fee above the maximum;
- an anchor naming the wrong wallet.
verify-vendored-fallback is now a single macro. A checkabi failure is reported as a tool failure, not as ABI drift. checkabi no longer double-spaces an internalType that already contains a space.
regenerate.sh requires TBTC_V2_BUILD_DIR and compiles by relative path, so regenerating is reproducible across machines.

Tests and fakes

The LocalChain RequestReservationReanchor fake now mirrors Reservation.sol (source and target state, cooldown, anchor floor, caps where zero blocks all, reserved target capacity). It writes only the action fields Solidity writes.
All acceptance Run tests now go through the strict validator fake. That fake now also checks the funding tx hash and the deposit script. Fixtures the contracts would reject were fixed. Misleading "cap" scenarios were removed.
The SPV fakes return zero records for absent keys, as the real mappings do, and honour EndBlock.
The conversion tests now cover TermSeconds, MinAmount and ReanchorCooldownUntil with non-zero values.
Each new regression test was checked by reverting its production line, confirming the test fails, and restoring the line.

Docs

Removed the duplicated tbtc.go header.
Completed the lists of fields the conversions deliberately skip.
Documented that TermSeconds and MinAmount are set only for acceptance generations.
Rewrote comments that described removed behaviour or cited internal review IDs.

Behaviour change to note

Only Developer has a reservation activation entry today. On Mainnet, Sepolia and other networks, none of the three loops scans history at startup; they process new blocks only. For the action-timeout watcher this replaces the previous 30-day lookback. It matches the other loops, and reservations are not enabled on those networks until an entry is added. When an entry is added, restart recovery covers everything since activation.

Corrections to #4343

"Pending receipts suppress duplicate requests": the adapter never returned Pending, and entries expired after 6 blocks while rounds are 900 blocks apart. The mechanism is removed; see Re-anchor.
"Accept Live and MovingFunds source wallets": this was true only for the acceptance wallet. Re-anchor sources were MovingFunds only; Closing is now also accepted.
"Watcher recovery scans at the activation block, in bounded chunks; unknown networks skip startup scans": the action-timeout watcher did neither. It does now.
"Preserve the re-anchor miner fee in domain conversions": no conversion existed. The unused MinerFee field is removed.
"Tightened LocalChain validator fakes": most acceptance Run tests bypassed them. They no longer do.
"Action snapshots" mutation-checked: the conversion test compared zero with zero. It now uses non-zero values.
The undeclared dependency on tbtc-v2#1161 is resolved by the new pin.

Not changed

Re-anchor requests stay inside leader proposal generation. Moving them into a maintainer loop was raised as a follow-up design option, not a defect.
The test capacity ledger does not model capacity held by pending acceptances.
StubBridge.json is not checked in CI; it is a test stub, not an upstream contract.

Verification

gofmt on every changed Go file: clean.
go build ./...: pass.
go vet on ./pkg/tbtcpg/... ./pkg/tbtc/... ./pkg/chain/ethereum/... ./pkg/maintainer/... ./cmd/... ./pkg/clientinfo/...: pass.
go test -count=1 on the same packages: pass.
go test -race on pkg/maintainer/spv and pkg/tbtcpg: pass.
The drift check and the harness check, run with bash -e against a local eec999aa build: pass.

The eight copy-pasted mktemp/jq/diff blocks become calls to a single verify_abi macro. The local side is written to a temp file and chained with && instead of being piped into jq, so a checkabi build failure now stops make with checkabi's own error instead of being reported as ABI drift. Explanatory comments that sat inside the recipe (and were echoed by make) move above the target.

An internalType that already reads "struct X" became "struct X". Also terminate the unknown-contract error with a newline.

Remove the duplicated tbtc.go file header, list DissolutionDelay among the intentionally unconverted action fields, document that TermSeconds and MinAmount are acceptance-only snapshots, drop the never-populated ReservationReanchoredEvent.MinerFee field, and move the deposit refund safety margin constant next to the test fake that uses it.

…t it The drift job compiled tbtc-v2 9f8f5ef1 while the harness validator was vendored from e635e229, whose validateReservationAnchorProposal logic differs under an identical ABI, so no check could notice. - Pin TBTC_V2_REF to eec999aa (merge of tbtc-v2#1161 into reservations-upgrade); the four reservation artifacts and the Go bindings still match it. - Re-vendor WalletProposalValidator.json from an eec999aa build (code unchanged apart from the metadata hash) and record that commit. - Add verify.sh and a job step comparing the vendored validator to the compiled one: ABI exactly, creation and deployed bytecode with solc's CBOR metadata removed, since the job's unfrozen yarn install can move the metadata hash. - regenerate.sh now requires TBTC_V2_BUILD_DIR instead of defaulting to a local sibling checkout.

… lookup The in-memory in-flight map did not suppress duplicate requests across rounds: rounds are far apart, the leader rotates, restarts clear it, and the adapter never reports a mempool transaction as pending. Safety comes from re-reading chain state, since Reservation.sol only accepts a request against an Active reservation. Remove the map and its helpers, GetReservationReanchorRequestReceipt with its receipt enum, and restore RequestReservationReanchor to return only an error. Run now dispatches on chain state alone: a mined request shows up as an ActionPending reservation and takes the resume path.

…alidator harness Run the real WalletProposalValidator bytecode at the boundaries only it can check: an action timing out exactly REQUEST_TIMEOUT_SAFETY_MARGIN from now is rejected and one second later is accepted (anchor and re-anchor), a MovingFunds wallet can anchor, a re-anchor target at its reservation cap is accepted and one above it is rejected, and an anchor fee above the action's max fee or naming a wallet other than the action's target is rejected. Times use the harness block timestamp so the boundaries are exact. Also state the real reason the simulated backend is avoided: github.com/fjl/memsize's linkname to runtime.stopTheWorld is rejected by the Go 1.23+ linker.

… behind the tip All three reservation loops now share one scan-range policy: the first scan starts at the reservation activation block (the action-timeout watcher no longer uses a 30-day lookback), networks without an activation entry skip the first scan, and every scan runs through fetchPastEventsInChunks and stops a few blocks behind the tip. The chunk helper hands each chunk to a callback so callers keep progress and advance their cursor chunk by chunk; a scan error no longer skips the proving or checking of already-tracked items, and the proof loop runs the re-anchor round even when the acceptance round fails. The stranding startup registration scan is chunked as well.

The LocalChain RequestReservationReanchor fake now enforces every requestReservationReanchor precondition in Solidity's order and with its revert reasons (cooldown, MovingFunds or Closing source, Live target distinct from the source, signing window, anchor floor, count cap where zero blocks all, amount cap), reserves the target's count and amount on request, and writes exactly the action fields Solidity writes. WalletReservationsCount and WalletReservationsAmount read a ledger made of the wallet's custodied reservations plus the capacity pending re-anchor requests reserved on it; new timeout and settlement helpers update it the way the contract does. Fixtures now set an action timeout and keep anchors above ReservationTxMaxFee + ReservationMinAmount, as a real request requires.

Reservation.sol accepts a permissionless re-anchor request from a MovingFunds or Closing source, and a wallet can reach Closing while it still holds reservations. Run only acted on MovingFunds wallets, so a re-anchor missed during MovingFunds, including the resume of an already authorized generation, stayed stuck and blocked wallet closing. Accept Closing on both paths; the below-dust notification stays MovingFunds only.

…producible metadata

…ay window

Estimate the fee and assemble the re-anchor transaction against the live ReservationTxMaxFee, which Solidity snapshots into the new action, before sending RequestReservationReanchor, so a fee spike no longer leaves an unusable request holding the target's capacity. Any failure after the request was sent now ends the pass instead of moving on to another reservation; only pre-submit capacity reverts retry the next target. The mined-wait stops as soon as the nonce advances to another requester's action, and the not-confirmed error now wraps its cause.

…ttleable generations A TimedOut acceptance generation is evicted once its reservation exists, since the Bridge then rejects every other acceptance proof. Each pass walks a reservation's generations Pending first, then TimedOut by descending nonce, and stops after the first submission attempt, so two generations matching the same Bitcoin transaction no longer broadcast a second, reverting proof. A TimedOut re-anchor generation is evicted when the reservation is no longer Active, ActionPending or Stranded, its reservation record is read once per pass and reused for proving, and a TimedOut generation with no matching or provable transaction is re-checked on a doubling backoff capped at 32 passes, ended early when the source wallet's transaction list changes.

…ld revert Run now skips an Active reservation while block time is before its reanchorCooldownUntil or when its anchor is not above ReservationTxMaxFee + ReservationMinAmount, the two request-time gates Reservation.sol applies to permissionless callers. Previously such reservations re-ran the target search and a reverting gas estimate on every window.

…ts pending entry pollTick always recorded the deadline from the reveal event at discovery, so the chain-read fallback that recovered it for an unmemoized deposit (with its own unchunked 30-day reveal scan and sentinel error) never ran in production. The deadline now lives in the tracked entry and is passed to CheckStaleReservedDeposit; the memo, the fallback helpers and their tests are removed.

Run searched for a target per reservation and per capacity retry, each time re-running the bounded registration scan, the unbounded fallback, and GetWallet plus capacity reads for every wallet, exactly when caps are saturated. A per-pass target search now reads registrations and wallet states at most once, lazily on the first reservation that needs a target, rules out count-capped wallets for the rest of the pass, remembers the smallest anchor that fits nowhere, and skips already-read wallets in the unbounded fallback.

… test The end-to-end typed-nil test returned once the forced panic fired, before the deferred recover had called recordReservationWatcherDeath. A no-op hook now runs after that call and the test waits on it.

…y margin Add a Run test where only the amount pre-check rules out a partly filled target, with the boundary where the target lands exactly on the cap and the zero-cap-is-unlimited case, each asserting which target received the single request. Pin the Go copy of the validator's 7200-second request timeout safety margin.

The copy of [ethereum] Network into the SPV maintainer config moves into maintainerConfig, so a test can pin it: without it the network is Unknown and every reservation catch-up scan is skipped.

Correct the EthereumNetwork, ethNetwork, lookback-constant, stale-deposit and anchor-hash comments: the reservation proof loop and the stale-deposit and action-timeout watchers all start their first scan at the activation block and skip it on networks without an entry, the 30-day lookback now bounds only the stranding registration scan, and the Go anchor hash differs from the Bridge's for a reservation without an anchor. Drop history wording from the touched comments.

The watcher-death metric test left its recovered goroutines reading reservationWatcherPanicRecovered while the typed-nil test replaced it, a data race under -race. Both tests now install the hook through one helper and wait for both recover paths before returning.

…uest

Candidates now come from the wallet's ReservationAcceptanceRequested events within the on-chain action timeout plus a margin, instead of DepositRevealed events from the last 30 days. Only generations the chain confirms are a Pending Acceptance for this wallet count toward the per-run budget, and that check runs before any reveal search or Bitcoin lookup, so never-requested reveals cannot starve real requests. The reveal is located by a chunked backward scan from the request block bounded by the generation's term, so requests made long after the reveal are still proposed. Run performs the per-window setup and gauge publication once, then tries candidates in timeout order; a fee-estimate or proposal failure skips that candidate instead of aborting the window. The pre-write error wrapper is removed. Tests delegate to the strict anchor validator, which now also checks the funding transaction hash and deposit locking script. Fixtures use real funding transactions, future refund locktimes, the 2-hour deposit minimum age and a 90-day term. Scenarios that only rejected for lack of a pending action are removed, the below-minimum scenario becomes the reachable boundary case, and a MovingFunds scenario is added.

…it reveal

…sts by invariant

…uest

coderabbitai · 2026-09-30T09:20:55Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 204810a0-b23d-4663-a6f6-40452beb3c48

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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

piotr-roslaniec added 30 commits September 30, 2026 08:13

fix(clientinfo): name vault fee reserve gauge in TBTC base units

009cf3d

test(spv): return zero records for absent reservation keys and honour…

2fde8d3

… EndBlock in event fakes

fix(tbtc): keep checkabi internalType unchanged when already spaced

001c43d

An internalType that already reads "struct X" became "struct X". Also terminate the unknown-contract error with a newline.

test(ethereum): cover action TermSeconds and MinAmount conversion

2517c1a

fix(chain): pass the known vault address to reservation vault fee reads

f391d9c

feat(reservations): expose the re-anchor cooldown on tbtc.Reservation

324cd92

fix(ethereum): compile the harness StubBridge by relative path for re…

cb72d01

…producible metadata

fix(tbtc): locate anchor deposit reveals from reveal time, not a 30-d…

f9cb95c

…ay window

test(spv): wait for the watcher recover path in the typed-nil metrics…

add36ce

… test The end-to-end typed-nil test returned once the forced panic fired, before the deferred recover had called recordReservationWatcherDeath. A no-op hook now runs after that call and the test waits on it.

test(cmd): cover the Ethereum network handed to the SPV maintainer

c575313

The copy of [ethereum] Network into the SPV maintainer config moves into maintainerConfig, so a test can pin it: without it the network is Unknown and every reservation catch-up scan is skipped.

docs(tbtcpg): note the cooldown in the re-anchor resume margin comment

07bbb04

Merge branch 'fix/m1-review-wp-a' into t3code/review-current-pull-req…

0aab992

…uest

Merge branch 'fix/m1-review-wp-c' into t3code/review-current-pull-req…

928caf7

…uest

piotr-roslaniec added 4 commits September 30, 2026 09:15

fix(tbtc): use the chain's block time when locating an anchor's depos…

0c1a261

…it reveal

test(tbtcpg): model budget fillers as settled generations and name te…

9d7fe18

…sts by invariant

Merge branch 'fix/m1-review-wp-b' into t3code/review-current-pull-req…

2ffa901

…uest

fix(tbtcpg): scan acceptance requests in bounded chunks

42bde46

piotr-roslaniec marked this pull request as ready for review September 30, 2026 09:51

piotr-roslaniec merged commit 49c4df1 into reservations-epic Sep 30, 2026
19 checks passed

piotr-roslaniec deleted the fix/m1-review-followups branch September 30, 2026 09:51

Navigation Menu

fix(reservations): address validated review findings on #4343 - #4346

fix(reservations): address validated review findings on #4343#4346
piotr-roslaniec merged 34 commits into
reservations-epicfrom
fix/m1-review-followups

piotr-roslaniec commented Sep 30, 2026

Uh oh!

coderabbitai Bot commented Sep 30, 2026 •

edited

Loading

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

piotr-roslaniec commented Sep 30, 2026

fix(reservations): address validated review findings on #4343

Context

Changes

Re-anchor (pkg/tbtcpg/reservation_reanchor.go)

Acceptance (pkg/tbtcpg/reservation_acceptance.go, pkg/tbtc)

SPV maintainers (pkg/maintainer/spv)

CI, harness and tooling

Tests and fakes

Docs

Behaviour change to note

Corrections to #4343

Not changed

Verification

Related

Uh oh!

coderabbitai Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Re-anchor (`pkg/tbtcpg/reservation_reanchor.go`)

Acceptance (`pkg/tbtcpg/reservation_acceptance.go`, `pkg/tbtc`)

SPV maintainers (`pkg/maintainer/spv`)

coderabbitai Bot commented Sep 30, 2026 •

edited

Loading