fix(tbtc): reservation review remediation (37 findings from multi-agent review of #4282)

[piotr-roslaniec]

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):

// 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 test(reservations): M2 test-coverage backfill (7 of 8 items) #4280's TestReservationAcceptanceTask_BoundaryChecks
  (adds MaxReservationsPerWallet, net-of-fee ReservationMinAmount,
  ActiveReservationsCount). Left in place rather than deleted preemptively -
  test(reservations): M2 test-coverage backfill (7 of 8 items) #4280 is still open and two-deep-stacked (on fix(tbtc): remove frequency gate on reservation checklist actions #4278, also open) and could
  stall or be reworked; delete this PR's version only in the merge that
  actually lands test(reservations): M2 test-coverage backfill (7 of 8 items) #4280.
• This PR's TestReservationAcceptanceTask_VaultNotConfigured_ZeroAddress
  (finding: dead vault-not-configured guard) has no equivalent on test(reservations): M2 test-coverage backfill (7 of 8 items) #4280's
  side - a "just take test(reservations): M2 test-coverage backfill (7 of 8 items) #4280's file" resolution silently drops it.
• Real design decision, not just a merge conflict: test(reservations): M2 test-coverage backfill (7 of 8 items) #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 test(reservations): M2 test-coverage backfill (7 of 8 items) #4280's test against whichever pending-action
  mechanism is ultimately kept. Whoever merges this PR and test(reservations): M2 test-coverage backfill (7 of 8 items) #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).

[coderabbitai]

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: Team

Run ID: 728e0686-0081-4f3d-a9af-ecf61e448f7b

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}