fix(reservations): review-round fixes for reservation ABI, watcher plumbing, and lookback bounds - #4324
Open
piotr-roslaniec wants to merge 27 commits into
Open
fix(reservations): review-round fixes for reservation ABI, watcher plumbing, and lookback bounds#4324piotr-roslaniec wants to merge 27 commits into
piotr-roslaniec wants to merge 27 commits into
Conversation
Regenerated ReservationRouter/WalletProposalValidator/Bridge Go bindings from the real compiled Solidity ABI (tbtc-v2 PR #1116 build artifacts), replacing the self-validating vendored fallback. This surfaced and fixed 5 P0-severity defects where the vendored bindings diverged from the real on-chain interface: - ProofType constants were off-by-one against the zero-based on-chain enum (Acceptance=0, Reanchor=2, not 1/3). - SubmitReservationProof called a combined entry point that does not exist on the router; split into SubmitReservationAcceptanceProof and SubmitReservationReanchorProof matching the real router functions. - Reservation proposal ABI structs were missing requestNonce, which the real WalletProposalValidator contract requires. - WalletReservations called a router view function that does not exist; reimplemented by deriving the wallet's currently-custodied reservation keys from acceptance/reanchor request events, filtered by each reservation's current on-chain wallet. - NotifyReservationActionTimeout took an extra walletMembersIDs argument the real router does not accept. Updates the spv.Chain and tbtc.Chain interfaces and all implementations/ test doubles/call sites accordingly.
…iew round 2 P1 fixes (all findings from the second multi-agent review pass on keep-core PR #4282): - Wire reservation watchers from the maintainer process (spv.go), not just the client (cmd/start.go); add a hard-fail startup self-check when the paired LeaderDutiesEnabled flags disagree and every watcher's initial scan found zero activity. - Harden the vendored ABI fallback CI guard to diff full function signatures, not just names; regenerate the two vendored fallback artifacts (ReservationRouter.fallback-artifact.json, WalletProposalValidator.reservation-methods-fallback.json) from the real compiled ABI, since they had drifted the exact same way the P0 fixes already corrected; add a CI job that compiles tbtc-v2 and verifies no future drift. - Add bounded retry to the stranding watcher's chain reads so a transient RPC failure does not permanently miss a wallet's stranding notifications. - Fix the startup catch-up scan to retry (not silently assume Live) a wallet whose GetWallet call fails during startup. - Re-check stranding after a Reanchor-type action timeout restores a reservation to Active under its (possibly dead) source wallet. - Gate the reserved-deposit sweep filter on the real per-network activation block instead of chain-call success/failure. - Bound reservation-acceptance candidate selection's Electrum calls with a worker pool and a real timeout instead of unbounded serial calls. - Park stale-deposit-poll entries assigned to a Live wallet instead of re-checking the entire tracked set every tick. - Capture and log the wallet's on-chain termination cause (moving funds timeout / moved funds sweep timeout / fraud challenge defeat) alongside each stranding notification, inferred from the three pre-termination Bridge events; best-effort, never blocks notification on failure. - Bound the action-timeout watcher's per-tick RPC volume with a rotating cursor instead of scanning every tracked action every tick. P2 fixes bundled in the same pass (landed in the same files as the P1 work above, by the same reviewing/fixing pass): - Remove the dead CI artifact shim in Dockerfile/client.yml whose compile step never existed; document the real vendored-fallback mechanism instead. - Fix the reservation proof loop's RBF-replacement handling (candidate map was silently dropping earlier transactions) and add a per-wallet transaction cache to avoid refetching full history every pass. - Reorder the stale-deposit watcher's timeout derivation to check its own memo cache before paying a chain RPC. - Correct two misattributed TODO(test-coverage) comments in tbtc.go and broaden the follow-up note to cover every proposal validator in the package.
- Fail closed for ethereum.Unknown in ReservationsActivationBlock: only the local Developer network activates reservations immediately: every other network, including the unset/unrecognized zero value, now requires an explicit activation-block entry or never activates. - Fix proofInfoCache re-reading a stale cached chain tip against a fresh per-transaction confirmation count within the same proof pass; the tip is now re-fetched per transaction like the confirmation count is, while the genuinely pass-invariant fields keep their existing cache. - Add a deterministic per-operator stagger before each watcher's first notify attempt (action-timeout and stale-deposit paths), so multiple reservation-enabled operators no longer race identical permissionless notify calls on simultaneous first detection. - Consolidate 5 duplicated 30-day lookback constants, 2 duplicated scan-range helpers, and 2 differently-formatted event-key helpers into one canonical implementation each across the reservation proof loop, action-timeout watcher, and wiring layer. - Fold the stale-deposit watcher's previously wiring-owned ticker, cursor, and pending/parked state into the watcher itself as a self-contained Run(ctx, interval) loop, matching the action-timeout watcher's existing shape; removes ~300 lines of split state machine from the wiring layer. - Add a wiring test asserting the stale-deposit and action-timeout watchers actually drive real Bridge notifications on startup, not just that WireReservationWatchers returns without error.
…named flags tbtc.ReservationsConfig was reused verbatim as pkg/maintainer/spv's own Config.Reservations field, creating two identically-typed, identically-named LeaderDutiesEnabled flags (Tbtc.Reservations.LeaderDutiesEnabled and Maintainer.Spv.Reservations.LeaderDutiesEnabled) with different real meanings - one gates proposal generation/watcher wiring/metrics in the client process, the other gates SPV proof submission in the maintainer process. Replace both with plainly-named booleans on their own owning configs: Tbtc.ReservationsEnabled and Maintainer.Spv.ReservationProofsEnabled. Delete tbtc.ReservationsConfig entirely. Updates every call site, test fixture (toml/json/yaml), and config_test.go path, plus comments that referenced the old dotted paths.
- Remove the false shared-cadence claim between DefaultReservationStaleDepositPollInterval and DefaultReservationActionTimeoutPollInterval; document both as independently tuned (currently coincidentally equal), not coupled. - Escalate both reservation watchers' Run-loop exit to Fatalf, matching this function's established severity for equivalent-consequence wiring failures: Run only returns non-nil post-construction on an unreachable-in-practice misconfiguration, so reaching that branch means the watcher silently stopped monitoring reservations - worth crashing loudly over, not logging at the same level as a routine per-tick error.
Address advisory gaps caught after the main P0/P1/P2/P3 batch: - TbtcChain.WalletReservations: extracted the deduplication, custody filter, and sort into a standalone resolveCustodiedReservationKeys helper (mirroring this file's existing buildReservationAnchorProposalAbi pattern), so the logic can be unit-tested without simulated-backend infrastructure. Added a WalletReservationsCount short-circuit so wallets that have never custodied a reservation do not pay the full genesis-to-tip eth_getLogs scan on every wallet-close notification; added deterministic big.Int sort on the returned key slice (Go map iteration order is non-deterministic). - WalletReservations test coverage: added TestResolveCustodiedReservationKeys exercising dedup (acceptance + reanchor event union), nil-event guard, custody filter (re-anchored-away exclusion), lookup-error propagation, and deterministic order. The riskiest untested new logic in the P0 batch now has regression coverage. - P0 #3 anchor/reanchor proposal mapping test vacuity: the existing TestBuildReservationAnchorProposalAbi and TestBuildReservationReanchorProposalAbi both used RequestNonce: 0 in input and expected RequestNonce: 0 in output, so the new RequestNonce field mapping would have silently regressed to omitting the field without any test failure. Set RequestNonce to non-zero values (17, 23) in both tests so the new mapping is actually exercised. - Marshal parity: tightened both ReservationAnchorProposal.Marshal and ReservationReanchorProposal.Marshal to reject zero-value fees (was Sign() < 0 only, accepting Sign() == 0 since big.Int(0).Bytes() is empty) and zero-value ReservationKey. Moved the three affected table-driven subtests into a new TestReservationProposals_MarshalRejectsZeroValues asserting directly against Marshal (the original table uses marshalThroughProposal which t.Fatalf's on any Marshal error, so it cannot cover Marshal-side rejection). - Stranding watcher operator-address plumbing: added operatorAddress field and SetOperatorAddress setter to reservationStrandingWatcher, and called SetOperatorAddress in the wiring layer at the existing strandingWatcher construction site. Stranding is intentionally NOT staggered - a blocking delay here would silently suppress a one-shot OnWalletClosed event (the trigger never replays), which is strictly worse than a transient gas race; the field is retained only for observability and forward-compatibility, documented in the struct field's doc comment. - Watcher Run goroutine death: replaced Fatalf escalation with defer recover() + distinct Errorf. Run only returns non-nil on the interval misconfiguration guard at loop start (per-tick errors are logged and the loop continues), so Fatalf was both unreachable in practice and architecturally wrong - WireReservationWatchers is called from two callers (cmd/start.go's client process and spv.go's maintainer process) and the caller decides process-level fatality, not this shared helper.
…c sort, test coverage Address gaps caught after the post-review batch landed: - TbtcChain.WalletReservations: thread the wallet's own on-chain registration block (earliest, since wallets can re-register during recovery) as StartBlock on both PastReservationAcceptanceRequestedEvents and PastReservationReanchorRequestedEvents filter queries. A wallet cannot have reservation events before it existed, so this is an exact lower bound for the eth_getLogs scan - the previous genesis-to-tip fallback only fired when the registration lookup itself failed, so a transient RPC error degrades to the old behavior rather than skipping real reservations. - resolveCustodiedReservationKeys: sort the candidate keys BEFORE the per-key GetReservation lookup loop, so the order of chain calls is deterministic and the partial-failure (first lookup error propagated) path reproduces across runs instead of depending on Go map iteration order. - TestResolveCustodiedReservationKeys: rewrite as four t.Run subtests covering the actual failure modes the original P0 fix turns on: dedup (key present in both event lists appears exactly once), custody filter (reanchored-away excluded), reanchored-in (key only reachable via the reanchor event list), and sorted output regardless of event order. Plus a lookup-error subtest. - Mutation-tested the RequestNonce mapping: temporarily setting line 529's mapping to RequestNonce: 0 makes TestBuildReservationAnchorProposalAbi fail; restoring makes it pass. The non-zero RequestNonce values (17, 23) in the input and expected structs are now actually exercised. - Stranding watcher operatorAddress plumbing: REMOVED. The field and setter I added previously were dead code - no caller reads the field and the wiring-layer setter call was on a one-shot path where adding a blocking stagger would silently suppress notifications rather than race them (the OnWalletClosed event never replays). Recording the stranding-stagger item as deliberately deferred with the one-shot rationale. - Stale-deposit watcher recover wording: corrected 'reservation stranding notifications' to 'stale reserved deposit notifications' in both the panic-recover block and the post-Run error message so the log doesn't misdirect whoever reads it during an incident.
|
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 |
CI client-format check caught a trailing blank line left in deposit_sweep_test.go in both packages from an earlier commit this session.
…d-fallback-verify The job I added in 8ec859d compiles tbtc-v2 upstream Solidity contracts via 'yarn install --frozen-lockfile', which failed deterministically on every run (checked both pushes to this PR): a transitive dependency (umpirsky/country-list, pulled in via @celo/contractkit) resolves over the unencrypted git:// protocol (port 9418), which times out against GitHub Actions runners' network egress - unrelated to any Go/Solidity code in this change, but it is this job's own CI wiring and belongs in scope here. Fix: rewrite git://github.com/ to https://github.com/ for git's own URL resolution before yarn install, so dependency fetches use the same protocol every other checkout/fetch step in this job already uses successfully.
The https rewrite in the prior commit fixes the git:// timeout but changes how yarn resolves the country-list git dependency vs the exact hash frozen in tbtc-v2's own yarn.lock, which --frozen-lockfile then rejects as drift even though no real dependency version changed. This checkout is a scratch clone of the upstream repo used only to compile and diff the real ABI - never committed back - so an unpinned, job-local lockfile resolution is safe.
ReservationRouter.sol does not exist on tbtc-v2's main branch yet - verified via the GitHub API (bridge/ dir has no Reservation* files on main). PR threshold-network/tbtc-v2#1112, which adds it, merged into tbtc-v2's own 'reservations-upgrade' integration branch instead - mirrors this repo's own reservations-epic staging pattern. Pointing TBTC_V2_REF at main made this job fail unconditionally on every run, independent of any lockfile or network fix. Also replace the raw 'cp: cannot stat' failure with a clear ::error:: pointing at TBTC_V2_REF, so a future re-drift of this ref (e.g. once the reservations work lands on tbtc-v2 main and this should switch back) fails with an actionable message instead of a bare missing-file error.
…re-terminal state - misconfiguration self-check no longer conflates RPC-failure with genuine zero on-chain activity; each scan signal is now tri-state and the check runs async instead of blocking client/maintainer startup on reservation-history scan depth - stranding recheck after an action-timeout notify now waits for the notify tx to mine instead of reading pre-mined state, and dedupes per wallet per tick - WalletTerminationCause is skipped entirely when a closed wallet holds zero reservations - stale-deposit Notified resolution is no longer terminal on tx submission; it is confirmed on a later tick's on-chain re-check and retried if unconfirmed, so a reverted notify can no longer permanently lose a tracked deposit - isWalletLive and the per-tick ReservationParameters read now reuse the existing per-tick memo instead of re-fetching - removed SetOperatorAddress/SetStrandingWatcher compatibility setters in favor of required constructor parameters - deleted the reservationDefaultLookBackBlocks compatibility aliases in reservation_proof_loop.go and reservation_stale_deposit_watch.go - corrected ReservationProofsEnabled's doc comment and stale LeaderDutiesEnabled/SubmitReservationProof references - removed misleading no-state-mutation claims on the Unknown/Keep resolution doc comments
…servation filtering WalletReservations' registration-block establishment no longer costs an unbounded genesis-to-tip eth_getLogs; earliestWalletRegistrationBlock is now bounded by the same 30-day-plus-reorg-margin convention used elsewhere in the reservation feature. WalletTerminationCause's doc comment is corrected: it does not verify mutual exclusivity of the three pre-termination events and returns the first found in a fixed priority order, not the most recent; its call-frequency doc comment now matches its actual call sites instead of claiming "once per wallet close". FindDeposits hardcoded reservationsActive=false, so moving_funds.go's unswept-deposit guard always treated reserved deposits as unswept, permanently blocking moving-funds proposal generation for any wallet holding one once reservations go live. FindDeposits now takes an explicit reservationsActive parameter: moving_funds.go passes the proposal request's real value, and the listing-only maintainer CLI caller passes false explicitly.
…nding-tx fetch depositSweepAction.execute() derived reservationsActive from proposalProcessingStartBlock, a strictly later block than the coordinationBlock the leader uses in executeLeaderRoutine, breaking the invariant that both sides evaluate the activation predicate at the same height. The action now carries and uses the real coordinationBlock, threaded from processCoordinationResult through handleDepositSweepProposal rather than reversed from a fixed offset (the reversal was an unguarded uint64 subtraction that could underflow). The reservation-acceptance funding-tx worker pool eagerly fetched every phase-one candidate's Bitcoin transaction body/confirmations before the oldest-first eligibility loop could pick one and stop, turning ~2 RPCs into up to the full backlog size. It now schedules candidates lazily, oldest-first, and stops dispatching once an eligible candidate is found, with new coverage for the concurrency cap, per-candidate timeout, and ordering under out-of-order completion.
Marshal was tightened to reject non-positive big.Int values on AnchorTxFee, ReservationKey, and ReanchorTxFee, but Unmarshal did not enforce the same invariant on receipt, so a hand-crafted protobuf carrying a non-empty zero-value byte encoding for any of the three fields passed unchanged. The vendored-fallback-verify CI job's rationale comment claimed to diff against a "real, currently-deployed" contract ABI when it actually diffs against the ABI compiled from the pinned upstream tbtc-v2 ref; also removed a dangling reference to prior-review finding IDs whose source artifacts don't exist in this checkout.
…nderflow now is a caller-supplied, not-guaranteed-monotonic tick token. If now ever precedes a deposit's recorded notifiedAt (clock adjustment, or a non-monotonic tick), now-notifiedAt underflows to approximately 2^32 and the backoff check fails open, resubmitting NotifyStaleReservedDeposit immediately instead of waiting out actionTimeoutRenotifyInterval. now < notifiedAt is now treated the same as "interval not yet elapsed".
…t wiring docs - misconfiguration self-check now logs at Errorf instead of Fatalf/os.Exit, so a correctly-configured node on a quiet network no longer self-terminates - initial watcher-scan goroutines publish a scanResult even when their pass panics, so a panic can no longer permanently block the self-check goroutine on resultCh and leak it - corrected wiring doc comments describing the self-check's actual behavior and the deferred (not immediate) stranding recheck after an action timeout - simplified reservationOperatorStaggerOffset's internal hash reduction to plain uint64 modulo (signature unchanged) - added an integration test proving the real WireReservationWatchers wiring drains a deferred stranding recheck end to end
…flow - add the same now < notifiedAt underflow guard the stale-deposit watcher already has, preventing a backward clock step from causing a renotify storm on the action-timeout watcher - drainStrandingRechecks now re-queues a wallet on a transient GetWallet or stranding-check error instead of permanently dropping its recheck
…osits An IsReservedDeposit RPC error inside findDeposits now returns a hard error instead of silently dropping the deposit, matching pkg/tbtc/deposit_sweep.go's posture. Previously an RPC error could make a still-reserved deposit look unreserved to the moving-funds unswept-deposit check. Also corrects the reservationsActive doc comment to reference the coordination block, not the current block.
…path - reservationsActive doc now correctly describes evaluation at the coordination block, not the current/processing block - removed a stale coordinationBlock field-doc sentence contradicted by the code and its sibling comment - added a regression test for ValidateDepositSweepProposal's fail-closed behavior when reservationsActive is true and ReservationParameters errors
…ting - give cancellation priority in the funding-tx lookup dispatcher so an observed stop() is checked before a dispatch slot is acquired, narrowing (per the pipeline's own tolerance) the early-stop race - correct pipeline doc comments to describe the concurrency cap as strict and the total-RPC/early-stop savings as best-effort, not guaranteed - move the skipDepositKeys check ahead of the per-run candidate cap so already-rejected deposits no longer consume fresh candidate budget
earliestWalletRegistrationBlock now resolves and memoizes a wallet's actual earliest registration block via a bounded backward walk instead of a fixed ~30-day window that silently fell back to a genesis-to-tip scan for any older wallet - the dominant case for the stranding watcher's callers. WalletTerminationCause reuses this bound instead of always scanning from block 0.
- operatorAddress field doc no longer cites a nonexistent SetOperatorAddress setter; points to the constructor-parameter rationale instead - pollTick's Notified/Drop description corrected to match StaleDepositResolutionNotified's actual documented contract - dropped stale 'Finding 6' review-report references from test comments
walletTransactionsForProof now fetches transaction bodies for its own already-known hash list instead of calling GetTransactionsForPublicKeyHash, whose Electrum implementation re-fetches the same hash list internally - removing one of two redundant full wallet-history scans per cache miss. Added a proveReservationTransaction test covering a non-matching candidate ahead of an unconfirmed matching one, and dropped stale review-report 'Finding N'/'P0 #N' references from test comments.
currentBlockHeight never cached its result (by design, per its own doc comment - the Bitcoin tip can change between two calls in the same pass), so attaching it to proofInfoCache was misleading. It is now a package-level function; behavior is unchanged.
- coordination.go's activation-block table doc no longer claims ethereum.Unknown activates at block 0 or names the removed reservationsActivationBlock symbol - client.yml's vendored-fallback-verify job comment no longer claims it runs on every event/branch; it runs whenever the Client workflow runs
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.
Fixes surfaced during multi-agent review of the reservations stack (P0-P3), landed as one branch off
reservations-epic.Summary
NewReservationActionTimeoutWatcher, not aSetStrandingWatchersetter) so a reservation restored toActiveby an action-timeout notify is scheduled for a stranding recheck on the next poll tick (not immediately -- the notify transaction has only just been submitted, not mined).reservation-router-vendored-fallback-verify): compiles the real tbtc-v2 Solidity contracts from the pinned upstream ref and diffs the generated ABI against the vendored fallback, catching silent drift.WalletTerminationCause: infers which pre-termination event (MovingFundsTimedOut / MovedFundsSweepTimedOut / FraudChallengeDefeatTimedOut) preceded a wallet's termination.ReservationsConfiginto plainly-named flags (Tbtc.ReservationsEnabledin the client,Maintainer.Spv.ReservationProofsEnabledin the maintainer). Both processes independently wire the reservation watchers (stranding / stale-deposit / action-timeout) under their own flag; this is intentional - the watchers are permissionless, network-wide duties, not leader-election duties, so redundant wiring in a colocated deployment is functionally redundant (aNotify*call against an already-settled reservation is a chain-level no-op), though two independently running watchers can still race before settlement.WalletReservationsnow derives its result fromNewWalletRegistered/reservation events (an event-derivation switch away from the prior router view call) and bounds reservation-acceptance and reanchor event queries by the wallet's own registration block (resolved via a bounded backward walk, memoized per wallet), reducing scanned range vs. the prior genesis-to-tip fallback.resolveCustodiedReservationKeyssorts candidate keys before the per-key lookup loop so the partial-failure path reproduces across runs.pkg/tbtcpggained a bounded concurrent worker pool that fetches Bitcoin transaction bodies/confirmations for reservation-acceptance proof candidates. Separately,pkg/maintainer/spv's proof loop gained RBF multi-candidate proof selection and a wallet transaction cache -- these are two distinct changes in two different packages, not one combined feature.pkg/maintainer/spv: the stale-deposit poller was restructured into a two-tier pending/parked model (ReservationStaleDepositWatcher) with its ownRun/pollTickand a periodicparkedReconcilepass, replacing the prior flat polling loop.pkg/tbtc:MarshalandUnmarshalvalidation tightened to reject non-positive-but-nonzero byte encodings on the reservation fields (both directions, not Marshal only); the deposit-sweep reserved-deposit sweep-protection gate now fails closed (hard error) on an unrecoverable proof loop error instead of silently degrading, and reads the reservation-activation predicate through the newly-exportedReservationsActivationBlock, which -- unlike the removedreservationsActivationBlockit replaces -- returnsmath.MaxUint64(never active) for every network exceptethereum.Developeruntil an explicit entry is added toreservationsActivationBlocks, including forethereum.Unknown.reservationParametersFetchRetrieswas removed and a newreservationsActiveparameter threaded through five exported functions to carry this activation decision explicitly.SubmitReservationProofwas split into two methods (dropping theproofType/mainUtxoparameters each no longer needs), andNotifyReservationActionTimeoutno longer takeswalletMembersIDs.TestResolveCustodiedReservationKeysadded, covering dedup, custody filter, reanchored-in, sorted output, and the lookup-error short-circuit (5 subtests); mutation-verified the RequestNonce mapping actually fails when broken.go.sumchurn (~300 lines): module-graph hash refresh only (go mod tidy-style/go.mod h1:entries), nogo.modchange and no new content hashes -- verified, not a dependency bump.Review fixes (round 1, prior commits on this branch)
Applied all confirmed P1/P2/P3 findings from the first multi-agent review pass (see
agent-docs/reviews/pr-4324/report.md), including: the misconfiguration self-check's RPC-failure/zero-activity conflation, the action-timeout stranding-recheck timing race,FindDeposits' hardcodedreservationsActive=falseblocking moving-funds proposals, the registration-block bounding gap, the leader/follower activation-height mismatch, the stale-deposit watcher's submission-vs-mined terminal-state bug, and a set of documentation/simplicity/performance corrections. One architectural question (dual watcher wiring across the client and maintainer processes) was reviewed and confirmed as intentional split-deployment support, not a defect. One deployment-sequencing question (does the activation-height table land before the ReservationRouter/vault is trusted by the Bridge) was deferred to the deployment/ops team.Review fixes (round 2, this update)
A second multi-agent review pass (see
agent-docs/reviews/pr-4324-round2/report.md) against the larger diff above found 1 P0, 3 P1, and 16 P2/P3 confirmed issues (after two adversarial validation passes), now fixed:Fatalf/os.Exita correctly-configured node on a quiet/freshly-activated network -- downgraded to a logged error, no more process kill.IsReservedDepositRPC-error handling asymmetry that could let a moving-funds proposal proceed on a still-reserved deposit; and the wallet-registration-block bound that fell back to an unbounded genesis scan for the dominant (months-old wallet) case.stop()scheduling race and total-RPC-bound doc gap in the reservation-acceptance worker pool, acandidatesExaminedbudget-ordering bug, several safety-adjacent doc/comment corrections, aWalletTerminationCauseunbounded-scan cost (now bounded by registration block), missing integration/regression test coverage, and aproofInfoCachemethod relocated off a type it doesn't actually cache anything on.agent-docs/gap-inventory.md).agent-docs/reviews/pr-4324-round2/report.md.Verification
go build ./...clean.go vetclean on touched packages.pkg/maintainer/spv,pkg/tbtc,pkg/tbtcpg,pkg/chain/ethereum,pkg/clientinfo,config,cmd.