fix(reservations): close cross-repo M1 review findings - #4343
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 was referenced Sep 29, 2026
…ce anchor Address review findings in the Ethereum reservation adapter: - pkg/chain/ethereum/tbtc.go:915-925 - map only ethereum.NotFound or a nil receipt to NotFound; propagate every other TransactionReceipt error. - pkg/chain/ethereum/tbtc.go:918-919 - bound the receipt lookup with a 30s context, matching the baseChain header helpers. - pkg/chain/ethereum/tbtc.go:1767-1782 - drop the unreachable empty-string vault address branch. - pkg/tbtc/chain.go:612 - remove RequestReservationAcceptance from the reservation chain interfaces and the Ethereum implementation; only the depositor may request acceptance. - Expose the action's on-chain SourceAnchorUtxoHash snapshot for the SPV proof loop's TimedOut re-anchor eviction. - pkg/chain/ethereum/tbtc.go:915, :1795 - add adapter tests for receipt status mapping and vault fee-debt/reserve reads.
Address review findings in wallet coordination: - pkg/tbtcpg/reservation_reanchor.go:981 - forget in-flight re-anchor requests whose reservation left the source wallet. - pkg/tbtcpg/reservation_acceptance.go:427 - zero the vault fee gauges when the reservation vault is unconfigured. - pkg/tbtcpg/chain_test.go:1420 - anchor validator fake enforces wallet state, deposit, fee, minimum amount and refund margin preconditions. - pkg/tbtcpg/chain_test.go:1515 - re-anchor validator fake enforces source custody, source != target and the fee bound. - pkg/tbtcpg/chain_test.go:1728 - re-anchor request fake enforces the target amount cap; Run test covers moving to the next candidate.
…oofs Address review findings in the SPV maintainer and watchers: - pkg/maintainer/spv/reservation_event_scan.go:13-24 - use 10,000-block startup scan chunks; test chunk boundaries and error propagation. - pkg/maintainer/spv/reservation_proof_loop.go:625-690 - evict a TimedOut re-anchor generation only once the reservation's current anchor no longer matches the action's on-chain source anchor snapshot. - pkg/maintainer/spv/reservation_proof_loop.go:285 - keep scan state across inner-loop restarts so recovery resumes instead of rescanning. - pkg/maintainer/spv/reservation_event_scan.go:41 - drop the unused currentBlock parameter. - pkg/maintainer/spv/reservation_stale_deposit_watch.go:125, :325-330 - remove the parameters tick cache and the redundant IsReservedDeposit read. - pkg/maintainer/spv/reservation_stale_deposit_watch.go:36-39,84-87,245-253, :77-79,253-257, :729; reservation_wiring.go:438-446; reservation_action_timeout_watch.go:251 - correct watcher comments. - pkg/maintainer/spv/reservation_wiring_test.go:1079, :1183 - synchronize the recording metrics recorder and replace the sleep with a completion signal. - pkg/maintainer/spv/reservation_stale_deposit_watch_test.go:136 - drop review-history narration from test comments.
- .github/workflows/client.yml:240 - the ABI drift gate now also compares the ABI embedded in the committed generated bindings (extracted with go run ./checkabi) against the compiled tbtc-v2 artifacts, failing on mismatch.
- pkg/clientinfo/performance.go:242, :767 - state the TBTC reserve gauge publishes base units and is approximate above 2^53. - pkg/chain/ethereum/tbtc_validator_harness_test.go:8 - replace review-ID narration with evergreen intent.
piotr-roslaniec
marked this pull request as ready for review
September 30, 2026 08:13
piotr-roslaniec
added a commit
that referenced
this pull request
Sep 30, 2026
# 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. ## Related - Follow-up to #4343. Tracker: #4282. - tbtc-v2#1161 (merged into `reservations-upgrade` as `eec999aa`).
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
1 participant
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.
Context
A cross-repo review compared tbtc-v2 #1116 and keep-core #4282 against the M1 reservations specification. It raised 60 candidates; two independent validation passes confirmed 48 and dropped 7. This PR closes the keep-core code findings on top of
reservations-epic.The three P0s made the wallet-side flow nonfunctional or discarded contract-settleable proofs:
Changes
Wallet coordination
SPV maintainers and watchers
timeoutAt + termSeconds; keep TimedOut re-anchor proofs without an added Go-side expiry.ABI, chain and observability
termSecondsandminAmountsnapshots and the re-anchor miner fee in domain conversions.Verification
gofmton every changed/new Go source: clean.go build ./...: pass.go vet ./cmd/... ./pkg/chain/ethereum ./pkg/clientinfo ./pkg/maintainer/spv ./pkg/tbtc ./pkg/tbtcpg/...: pass.go test -count=1 ./cmd/... ./pkg/chain/ethereum ./pkg/clientinfo ./pkg/maintainer/spv ./pkg/tbtc ./pkg/tbtcpg/...: pass.make -C pkg/chain/ethereum/tbtc/gen verify-vendored-fallbackagainst freshly compiled tbtc-v2 artifacts: all four ABI comparisons pass.go test -count=1 ./...was attempted. Every changed package passed; the run failed only in unmodifiedpkg/tecdsa/dkgwhenTestTssFinalize_IncomingMessageCorrupted_WrongPayloadhit its internal TSS round-one timing guard. Re-running that test produced nondeterministic outcomes across identical source and an identical 87-package dependency closure, including a pass at the exact base commit. This PR does not suppress or modify that test.Targeted regression tests were mutation-checked: each failed when its corresponding production branch was temporarily removed, then passed after exact restoration. Covered paths include late TimedOut proofs, action snapshots, P2PKH matching, activation-block recovery, re-anchor receipt states, the timeout safety margin, and typed-nil metrics.
Scope notes
reservations-epicatf66f11240(includes fix(reservations): review-round fixes for reservation ABI, watcher plumbing, and lookback bounds #4324).dev; Reservations epic -> dev tracking #4282 remains conflicting until that separate integration step.pkg/chain/ethereum/tbtc_validator_harness_test.goat 981 lines;pkg/maintainer/spv/reservation_proof_loop_test.gogrows from 1,668 to 2,168 lines. This exception was explicitly chosen for this PR and should be paid down separately.go vet ./...warning inpkg/tecdsa/signing/protocol.gois outside this diff; changed reservation packages are vetted separately.Related