Reservations epic -> dev tracking by piotr-roslaniec · Pull Request #4282 · threshold-network/keep-core

piotr-roslaniec · 2026-09-03T05:46:04Z

Reservations epic -> dev tracking

This PR aggregates the UTXO reservations epic (reservations-epic) and tracks its promotion to dev.

M1 status (2026-10-29)

Read this before the diff: the branch is feature-complete for M1 and waiting on the freeze and the external audit, not on a merge.

The fixes are public. Every reservation fix in this milestone is on this branch, and the contract-side counterpart is on tbtc-v2 #1116 at d1bf5bc281. There is no private review line and no patch held back for publication: the work that used to be described that way landed here on 2026-10-28, and the diff below contains it.
The external audit has not started. The quote request went out on 2026-10-29 and no auditor has replied yet. What exists today is an internal audit of this branch, published as a record in docs/security/pr-4282-audit/ (with the re-run under rerun/); it is not an external review and it is not an audit verdict.
The next step is the M1 code freeze at the head named below, targeted at 2026-11-06 at the earliest. The external audit reviews the frozen heads (this PR's head and tbtc-v2 Increase group size to 64 #1116's head). The Sepolia deployment and the testnet drill run inside the audit window, in parallel, because there is no time for them before it starts; they are not the next step and they are not a precondition for the freeze.
The merge into dev comes after the audit, once its findings are fixed and re-reviewed. This PR stays open and draft until then.

Diff caveat

The previous public head 8139ef3c0 was 175 commits ahead / 231 commits behind dev (cf9be7030), which is why the three-dot diff used to show only the epic's unique commits. The current head 4ced1c8398 carries dev: it is 299 commits ahead / 0 commits behind dev (measured 2026-10-29), and git rev-list --count 4ced1c8398..dev prints 0.

Landing the review line (2026-10-28)

The epic carries the reservation work that was reviewed off this branch, landed on reservations-epic on 2026-10-28. Reservations are a feature in development, so that work is public: this branch is where it lives and where it is reviewed. The branches it came from are archived and are not a place for new reservation work.

Head: 4ced1c8398 test(reservations): cover four uncovered reservation review mutations (#4367). The reservation line sits on top of the previous public head 8139ef3c0, 355 commits ahead of it (283 non-merge, 72 merge), and it contains dev (cf9be7030).
Reservation line proper, per the review line's classification: 52 reservation commits, 3 plain commits, 4 rebuild fixups (flattened-import migration, per-item error aggregate type, wallet-transaction fee and unpack and API adaptations, coordination-test scoping) and 4 follow-ups (two docs naming the public tbtc-v2 source of the vendored validator build, a refactor giving the reservation round error its own name, and the CI pin to tbtc-v2).
Six commits cleared the CI failures the first push showed: e3bdf2590 gofmt in the reservation test files, e76208212 enables corepack so the tbtc-v2 compile step runs the declared yarn 4, 535b9a9d0 restores an integer guard the merge dropped and bounds the reservation conversions, 48d94ed50 skips the vendored third_party modules in the gosec run, 8d14cbf55 repins TBTC_V2_REF to tbtc-v2 4c98e6b82, and f03cc61dc re-includes scripts/check-reservation-flags.sh in the build image's docker context (the test image inherits .dockerignore, which excludes scripts/) and makes the missing-script case fail loudly instead of reading as a clean exit 0.
36 commits landed after f03cc61dc (26 non-merge, 10 merge). They are review follow-ups, not new scope: reservation floors the client paths had skipped, stranding of a Live wallet's expired reservation, the Closing wallet's closing period held before stranding, the proof-loop liveness gauge, a startup warning when the network's activation order is broken, the published internal audit record, and the mutation-survivor test coverage that closes the remaining review findings.
Files changed against dev: 360 files, +137,455 / -776. Most of the additions are the published audit evidence under docs/security/ (128 files); the code sits mostly under pkg/maintainer (49), pkg/tbtcpg (46), pkg/tbtc (46) and pkg/chain (40).
Two stops were resolved by hand: the regenerated ReservationRouter bindings (import block) and one SPV proof-loop hunk. Two further stops replayed from the rerere cache, and the rest applied cleanly.
Local evidence at the previous head: go build ./..., go vet on pkg/tbtc, pkg/tbtcpg, pkg/maintainer/spv and pkg/chain/ethereum, go test ./pkg/tbtcpg/... ./pkg/maintainer/spv/..., ./pkg/tbtc -run 'Reserv|Activation', ./pkg/chain/ethereum -run 'Reserv|Validator' and ./pkg/clientinfo all pass. TBTC_V2_REF pins tbtc-v2 4c98e6b82, and make -C pkg/chain/ethereum/tbtc/gen verify-vendored-fallback plus testdata/walletproposalvalidator/verify.sh exit 0 against that build with corepack active. CI status is in the Status section.

What the reservation line brings:

Reservation keeper monitoring (D28). Watchdog gauges for reservation proofs, alerts when an action passes its deadline, wallet discovery for those alerts, a failed gauge read failing the observation pass, gauges registered when only reservation proofs run, and a proof-loop liveness gauge. Docs describe the gauges and their escalation.
Reservation deadlines on chain time. Reservation actions are judged against the chain head rather than the host clock, and the SPV side alerts on actions past their deadline.
SPV and proof loops. A reservation proof pass keeps going past a per-item error instead of stopping on the first one, crashed reservation watchers restart with backoff, and a Live wallet's expired reservation is stranded rather than left open.
Parameter floors. The client applies the contract floors the paths used to skip: the re-anchor floor rejects a negative anchor value before the comparison, and the re-anchor target amount is checked for headroom before the request is sent.
Re-anchor off a retiring wallet. The client sends the source wallet member proof with a re-anchor request and falls back to the least-loaded Live wallet when every target is full; the router bindings are regenerated for the new request.
Hand-off capacity bindings. The testnet build is bound to the hand-off capacity contracts and to the release-to-pool entry, with a test that a deposit released to the pool is swept.
Sweeping and stranding. A wallet is requeued for a stranding recheck when the close handler fails, and the stale-deposit poll keeps its cursor and evaluates its budget on chain time.
Tests. Acceptance then re-anchor on one reservation across two windows, the DepositSweep lead and reservation checklist paths, keeper observer boundaries and error paths, keeper mutation-survivor tests, the reservation action read, validation, assembly and broadcast error paths, and the chained lifecycle test.
Docs and style. Where reservation cap reverts can come from, the published internal audit record with its executed evidence, gofmt and helper cleanups, and a refactor that describes the reservation fixes by behaviour instead of by tracker IDs.

Cross-repo M1 review (2026-09-28)

A review of this tracker and tbtc-v2 #1116 against the M1 specification raised 60 candidates; two validation passes confirmed 48 (3 P0, 6 P1, 27 P2, 12 P3) and dropped 7. Remediation is split across:

fix(reservations): close cross-repo M1 review findings #4343 - wallet coordination, SPV/watchers, ABI drift enforcement, validator harness and fee-debt metrics (merged 2026-09-30).
tbtc-v2 #1161 - validator snapshot, canonical ABI selectors, activation scripts and runbook (merged 2026-09-30).
docs/reservations-spec @ 3c5603fa5 - reconciled M1 requirements, architecture, readiness and delivery status.

Neither fix PR performs the dev reconciliation step.

Spec PR: keep-core #4366 docs(spec): reservations spec v5 for the M1 spec freeze, branch carry/reservations-spec-public-v5 at 506eabe560, targeting reservations-epic. It is open and in draft; it carries the spec, its approval follow-ups and its changelog. The spec freeze is 2026-11-05 and this branch's code freeze is 2026-11-06 at the earliest, so the spec PR lands before this one merges.

Merge stack (#4274 chain) - COMPLETE

feat(tbtc): wire reservation executors and watchers #4274 m1/keep-core-client -> reservations-epic - feat(tbtc): wire reservation executors and watchers (merged)
fix(spv): re-verify reservation action generation before SPV proof submission #4276 m1/reservation-readiness-fixes -> reservations-epic - fix(spv): re-verify reservation action generation before SPV proof submission (merged)
test(tbtc): add reservation proposal marshaling coverage #4277 m1/reservation-protobuf-marshaling -> reservations-epic - test(tbtc): add reservation proposal marshaling coverage (merged)
fix(tbtc): remove frequency gate on reservation checklist actions #4278 m1/reservation-coordination-checklist -> reservations-epic - fix(tbtc): remove frequency gate on reservation checklist actions (merged)
test(tbtc): multi-signer simulated integration test for reservation coordination #4279 m1/reservation-multisigner-integration-test -> m1/reservation-coordination-checklist - test(tbtc): multi-signer simulated integration test for reservation coordination (merged into fix(tbtc): remove frequency gate on reservation checklist actions #4278's branch prior to fix(tbtc): remove frequency gate on reservation checklist actions #4278 landing; content is in reservations-epic)
test(reservations): M2 test-coverage backfill (7 of 8 items) #4280 m1/reservation-test-coverage-backfill -> reservations-epic - test(reservations): M2 test-coverage backfill (7 of 8 items) (merged)

The entire #4274 stack is now merged into reservations-epic.

Other PRs targeting `reservations-epic`

draft: UTXO reservation wallet-side foundations (previous end-to-end attempt — see #4282 for M1) #4238 feat/utxo-reservation-wallet-support -> reservations-epic - superseded by the accepted reservations-epic / Reservations epic -> dev tracking #4282 implementation line; do not reconcile or merge this earlier parallel attempt.

Cumulative diff note

The epic also carries shared pkg/net/local release/handler changes inherited from its merge stack, including #4284's release-boundary drain fix. They are intentional network/test-stability work, not reservation protocol logic. The review-line merge adds the client-side reservation work listed above.

How to use this PR

New reservations work should target reservations-epic (or stack onto an open reservations PR).
This PR stays open as the running reservations-epic -> dev gate.
Next step: freeze at 4ced1c8398 for the M1 code freeze (2026-11-06 at the earliest), hand that head to the external audit, fix and re-review the findings, then merge this PR into dev. The Sepolia deployment and the testnet drill happen inside the audit window and do not block the freeze.

Status

The #4274 stack is complete and the review-line landing plus its review follow-ups are on the branch at 4ced1c8398. Cross-repo review remediation has landed in #4343 and tbtc-v2 #1161. #4238 is superseded, not a remaining merge decision. What is left, in order: the M1 code freeze, the external audit on the frozen head, the audit fixes and re-review, then the merge. GitHub reports mergeable: MERGEABLE and mergeStateStatus: CLEAN at this head; the PR is still a draft.

CI at 4ced1c8398, as read on 2026-10-29 from the PR checks, is green on all five workflows: Solidity ECDSA, Solidity ECDSA docs, Solidity Random Beacon, Solidity Random Beacon docs, and Client. The Client workflow passes all 18 jobs, including client-format, client-vet, client-lint, client-scan, client-build-image, client-build-artifacts, client-go-tests (1), client-go-tests (2), client-build-test-publish, client-integration-test, client-bench, client-gate, electrum-lifecycle, both vendor byte-identity jobs and reservation-router-vendored-fallback-verify.

One note on an earlier run: the first attempt of 40731829443 at f03cc61dc failed client-build-artifacts at "Build Docker Runtime Image", where go mod tidy && go mod download stopped on a transient proxy error (github.com/ipfs/go-ipfs-util@v0.0.3: read "https://proxy.golang.org/...": stream error: stream ID 89; INTERNAL_ERROR; received from peer). Rerunning the failed jobs at the same commit passed, and the whole workflow was green there before the follow-up commits landed.

coderabbitai · 2026-09-03T05:46:13Z

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

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

…nt review of #4282) (#4283) ## Summary Remediation for the 37 confirmed findings from a multi-agent review of PR #4282 (`dev` <- `reservations-epic`, i.e. the accumulated content of #4274+#4276+#4277). 37 raised -> 37 confirmed -> 0 dropped after arbitration and validation. - **P1 (6 of 7 fully fixed, 1 partially fixed):** deposit-sweep reservation-vault exclusion, reservation look-back underflow + target-wallet check, reservation acceptance `eth_getLogs` bounds + nonce reconciliation + caps, SPV proof-loop retry-eviction data loss (symptom fixed, structural root cause deferred - see below), stale-deposit timeout memoization, below-dust re-anchor trigger removal (M-27, resolved via tbtc-v2 source after user escalation). - **P2/P3 (22 of 30 fixed, 8 explicitly deferred):** see "Deferred" below. Full-repo `go build`, `go vet`, and `go test ./...` all pass with these fixes applied (verified after every commit and once more at closeout). ## Deferred (1 P1 architectural root-cause + 7 P2/P3 symptoms/hygiene) An arbiter-recommended structural fix for M-16 (remove the SPV proof loop's persistent-cursor design entirely in favor of the stateless bounded-rescan pattern every sibling proof type already uses) was attempted together with the M-7 nonce-aware timeout fix and an M-14 dead-code removal. That combined change broke three existing tests and was reverted rather than debugged under time pressure. Only a narrower, independently-safe subset landed: a surgical patch for M-3 (non-lossy cursor rewind) plus unrelated memoization/metrics/test fixes. **M-16's own P1 rating is only partially addressed** - the persistent-cursor design itself, and the M-7/M-14 symptoms it also breeds, remain unremoved. 1. **M-16 (P1)** `pkg/maintainer/spv/reservation_proof_loop.go:227-246` - `reservationProofScanState`'s persistent cursor is the structural root cause of M-3 (fixed surgically) and M-7 (below). Removing it in favor of the stateless bounded-rescan pattern is what broke 3 tests on first attempt and remains unimplemented. 2. **M-7 (P2)** `reservation_action_timeout_watch.go:260-281` - `CheckReservationActionTimeouts` deletes `pendingActions` entries on 3 of 4 non-notifying outcomes without asserting the tracked `requestNonce` against the freshly-derived one; same root cause as M-3. 3. **P2** `reservation_action_timeout_watch.go:370` + `reservation_wiring.go:38-49` - the timeout watcher's `WalletMembersResolver` only resolves wallets the local operator co-signs; an offline/disabled/colluding wallet's own operators get zero independent timeout coverage. 4. **P2** dead-code cluster in `reservation_proof_loop.go` / `reservation_proof_loop_test.go` - `findReservationAcceptanceTransaction`, `findReservationReanchorTransaction`, and their wrapper helpers have zero production callers; 14 tests exercise the unused wrapper instead of the `isMatching*` predicates actually called in production. 5. **P2** `reservation_proof_loop.go:612,~817` - two tautological guards are algebraically always-false, masking that the real enforced constraint is only `0 < fee <= TxMaxFee`. 6. **P2** `reservation_wiring.go:237-320` `startStaleDepositPoll` - the entire loop body runs untested inside a goroutine; existing tests assert only that the goroutine starts. 7. **P3** `reservation_action_timeout_watch.go:18-20` - unused "backward-compatibility alias" constant, zero references. 8. **P3** `reservation_proof_loop.go:644` - duplicated, truncated comment fragment left by a merge. ## Known conflicts with other open PRs in this stack - read before merging This branched from `reservations-epic` at `bb3dcb398`. Three other efforts are in flight against overlapping code and were **not** reconciled here, since they belong to PRs this one doesn't own: ### 1. `pkg/tbtc/coordination.go` vs #4278 (hard conflict, not cosmetic) #4278 ("remove frequency gate on reservation checklist actions") drops `&& windowIndex%frequencyWindows == 0` from the reservation-actions checklist gate (custody-critical, should run every window like `ActionRedemption`) but its diff still references the old single `ReservationsActivationBlock` constant. This PR's `602d0ef11` independently rewrote that same `if` into `reservationsActivationBlock(ce.ethereumNetwork)`, a per-network table lookup (`ethereum.Mainnet: 26500000`, everything else defaults to 0). **A conflict resolution that naively favors this PR's side of that hunk silently reinstates the frequency gate #4278 deliberately removed.** Combined resolution (verified against both intents): ```go // Reservation actions (acceptance, re-anchor) are custody-critical like // Redemption and are checked on every coordination window once the // activation block is reached, not frequency-gated like the // throughput-driven DepositSweep/MovedFundsSweep/MovingFunds actions // above: a delayed reservation acceptance or re-anchor risks the // on-chain ReservationActionTimeout backstop firing before the wallet // subsystem gets a chance to act. The activation block is a per-network // table (reservationsActivationBlock), not a single global constant, but // it is still config-independent and globally observable from chain // height alone -- which is what keeps leader and follower checklists in // agreement without relying on local config. if coordinationBlock >= reservationsActivationBlock(ce.ethereumNetwork) { actions = append(actions, ActionReservationAnchor) actions = append(actions, ActionReservationReanchor) } ``` ### 2. `pkg/tbtc/marshaling.go` vs #4278 (duplicate, this PR's version wins) #4278 independently adds the same 4 missing `Marshal`/`Unmarshal` doc comments this PR's `7cbb8cc2f` adds, but comment-only and with a capitalization bug (lowercases the exported type name, e.g. `"...converts the reservationAnchorProposal..."`). This PR's version is a superset: correctly capitalized comments plus the actual nil-guard/zero-hash-rejection logic #4278 doesn't have. On merge, take this PR's 4 lines, drop #4278's. ### 3. `pkg/tbtcpg/reservation_acceptance_test.go` vs #4280 (whole-file conflict + one real design decision) #4280 ("M2 test-coverage backfill") independently rewrote large parts of the same shared test harness this PR's `726f05ed7` touched - the same `reservationAcceptanceLocalChain` type, constructor, and ~14 shared methods, plus `scenarioReservationAcceptanceChain`/`registerReservedDeposits`/ `expectedAnchorsEqual`. This is a heavy line-level conflict across the whole file, not just redundant test names. Specifics: - `TestReservationAcceptanceTask_AmountCapBoundaries` (this PR, cap boundaries only) is a strict subset of #4280's `TestReservationAcceptanceTask_BoundaryChecks` (adds `MaxReservationsPerWallet`, net-of-fee `ReservationMinAmount`, `ActiveReservationsCount`). Left in place rather than deleted preemptively - #4280 is still open and two-deep-stacked (on #4278, also open) and could stall or be reworked; delete this PR's version only in the merge that actually lands #4280. - This PR's `TestReservationAcceptanceTask_VaultNotConfigured_ZeroAddress` (finding: dead vault-not-configured guard) has no equivalent on #4280's side - a "just take #4280's file" resolution silently drops it. - **Real design decision, not just a merge conflict:** #4280's `TestReservationAcceptanceTask_Stateless_PastEventsError` exercises `PastReservationAcceptanceRequestedEvents` returning an error and asserts fail-closed skip-on-error. This PR's `hasPendingAction` (from `726f05ed7`) no longer calls `PastReservationAcceptanceRequestedEvents` at all - it uses a different, generation-scoped pending-action check instead. Ported onto this PR's code as-is, that test would either pass vacuously or fail for an unrelated reason. **This PR intentionally left `PastReservationAcceptanceRequestedEvents` on the `tbtcpg.Chain` interface (`chain.go:263`) and the test double's `acceptanceEvents`/`acceptanceEventsErr` fields in place, undeleted, even though they now have zero production callers** - removing them here would have foreclosed reconciling #4280's test against whichever pending-action mechanism is ultimately kept. Whoever merges this PR and #4280 needs to pick one mechanism and either delete the losing side's interface method/test or keep both if there's a reason for two independent checks. ## Testing - `go build ./...`, `go vet ./...`: clean. - `go test ./...`: full repo suite, 0 failures (verified at closeout after every commit landed).

…m steps COPY ./ci-shims/tbtc-artifacts in the Dockerfile sourced a directory excluded by .gitignore and created only by client.yml's shim step; release.yml builds the identical Dockerfile with no equivalent step, so a fresh-checkout release build hard-failed on the missing COPY source. Un-ignore the directory and track a placeholder so it always exists regardless of which workflow builds the image. Also delete the client.yml steps gated on github.base_ref == 'reservations-epic': PR #4282's base is dev, so the gate never fires for this PR, and once the epic lands on dev it can never fire again. The gen/Makefile fallback rules already supply the same artifact surface and remain the only reachable mechanism.

#4282 40 confirmed findings fixed (2 P0, 7 P1, 24 P2, 7 P3); 1 P2 finding (in-kind fee reserve watch) intentionally left unfixed - its only two remediations are building unscoped new functionality or editing a separate spec repo, both out of scope here. P0: - wire notifyReservationAcceptanceTimedOut end to end (vendored ABI, Go bindings, chain interfaces, ethereum adapter, watcher dispatch by ActionType) - acceptance-type timeout notifications could never succeed on-chain before this - gate deposit-sweep's reserved-deposit filter on the reservation vault address instead of calling isReservedDeposit unconditionally on every deposit pre-activation, which risked halting sweeping network-wide during the binary-upgrade-before-Bridge-upgrade window P1: - action-timeout watcher no longer requires wallet-member resolution to notify a Reanchor timeout, so a dead/closed wallet's stuck action is now permissionlessly resolvable - drop the load-failure eviction that permanently dropped a tracked action from timeout coverage after 3 RPC hiccups - remove the mainnet reservations-activation-block placeholder pending a real rollout height - correct GetReservation/GetReservationAction doc comments (absence is Unknown state, not an error) - propagate chain-read errors instead of masking them as no-op - memoize per-pass proof-invariant chain reads shared across proof-loop transactions - add the missing reservations-remaining below-dust regression subtest P2/P3: gauge visibility, nonce-bound preload validation, notifiedAt semantics, address-comparison case sensitivity, wallet-vault fee-floor check, dual-flag config validation, duplicate/stale comments and tests, and assorted simplifications. Also reverts an in-flight hard-fail config-validation check for the Tbtc/Spv reservations dual-flag pairing back to a warning: start's config categories never load the Maintainer section, so the check would have hard-failed startup for any legitimate split deployment running the SPV maintainer as a separate process. Self-verification follow-up (same review pass, iterated against external review of the fixes themselves): re-checked every "fixed" claim above against the actual diff and against the reasoning behind each fix, and made three further corrections: - deposit-sweep follower-side (ValidateDepositSweepProposal) never actually got the reservation-vault gate described above - only the leader side (deposit_sweep task) did, so a follower could still sign a sweep a leader had already correctly rejected. Implemented the matching gate on the follower path: fetch ReservationParameters with the same bounded retry as the leader (fewer attempts only makes a wrong "reservations aren't live" guess more likely, never less, so there is no safety argument for the follower retrying less than the leader), then hard-reject on any IsReservedDeposit error or a confirmed reservation for any deposit the cheap vault-match prefilter flags as a candidate - unlike the neighboring fee soft-check just below, which stays deliberately log-only because a merely underpriced sweep is not irreversible the way sweeping a reservation is. Added the regression test proving detection (TestValidateDepositSweepProposal_RejectsReservedDeposit) and its counterpart proving the fail-open path still sweeps normally when ReservationParameters is unavailable (TestValidateDepositSweepProposal_SweepsWhenReservationParametersUnavailable, mirroring the leader's existing test of the same shape). - the P1 loadFailures-eviction removal above initially grew a backoff mechanism to avoid re-logging every poll tick during a persistent failure, but the backoff suppressed the downstream timeout-notify check for the same tick it suppressed the retry - a single transient RPC error could have delayed a genuinely-overdue Bridge notification by up to 10 minutes, worse than the log spam it was meant to prevent (a real outage fails every tracked action identically regardless of backoff, so there was nothing for a per-entry backoff to usefully save). Removed it; kept the simpler P1 fix alone - retry every tick, evict only on an observed non-Pending state. - reservation_wiring.go's walletMembersResolver parameter was threaded through WireReservationWatchers but never read by anything downstream of the P1 fix that removed its only consumer; deleted the dead parameter from the function, its test, and the cmd/start.go call site (which used to feed it the WalletMembersResolver tbtc.Initialize returned), and corrected the call site's comment (the paired-flag validation is a warning, not a hard error, per the note above). Finished the resulting clean cutover: with cmd/start.go (Initialize's only caller anywhere in the repo) no longer reading it, tbtc.Initialize now returns only error, and the WalletMembersResolver interface plus node.ResolveWalletMembers (its one implementation) are deleted with it.

mergeability

…4282 Comment-only audit and fix pass across all comments this PR added or modified: drift against current code, self-containment (drop references to other repos / markdown docs that don't exist in keep-core), redundant text, and inconsistent voice. No behavioral change - gofmt/build/vet clean, diff confirmed comment-only against d1697f5.