feat(dash-spv): adaptive gap-limit probe escalation for BIP44 discovery - #959
Open
PastaPastaPasta wants to merge 7 commits into
Open
feat(dash-spv): adaptive gap-limit probe escalation for BIP44 discovery#959PastaPastaPasta wants to merge 7 commits into
PastaPastaPasta wants to merge 7 commits into
Conversation
…re commit A filter batch used to commit once the gap-limit chase's latest wave derived no new scripts. That signal is wrong twice over: scripts derived from blocks owned by other batches never enter this batch's collected set, and a wave whose visible transactions all pay already-derived indices (backfill - inevitable when dust waves are mined out of derivation order) derives nothing even though the batch still holds blocks paying indices past the window. Committing on it ends the chase early, and committed batches are never rescanned, so every transaction above the window at commit time is permanently lost and discovery flatlines: later activity pays underived addresses, filters stop matching, and the pool never extends again. Observed in production on a mainnet wallet with 345,261 sequentially used addresses: discovery froze at address index 2,400 after ~500 blocks of activity and silently missed 880k+ transactions while sync ran to tip. The manager now keeps a monotone script-derivation generation, bumped whenever block processing derives new scripts - including for blocks whose owning batch is already gone, whose scripts were previously dropped without ever being matched. Each batch records the generation it was last matched against the wallets' full script sets (initial scan or verification). At commit time, a batch whose recorded generation is stale is re-matched against the full current sets and may not complete its rescan while that verification still finds blocks. Quiet syncs never pay for this: with no derivations the generations match and the verification is skipped. The regression test drives the real FiltersManager + WalletManager over a synthetic dust restore whose payments land out of derivation order across a batch boundary; without this fix it loses 36 of 3,000 transactions, with it discovery completes (as does the in-order control). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n flight A block in flight for a later batch can still derive new scripts when it lands, and those scripts can match an earlier batch's filters. Sealing the earlier batch during that window raced the delivery - the same knowledge-behind-the-watermark loss the verification rescan prevents, through a narrower window. Commits now wait for global block quiescence, so a seal implies no undelivered derivations exist anywhere and the verification fixpoint argument has no gaps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…overy BIP44 discovery derives only gap_limit (default 30) addresses past the highest used index, so a run of unused indices longer than the gap limit stalls discovery silently. A real mainnet wallet has 273 such runs of 30 or more (the longest exactly 100); plain gap-limit discovery freezes at the first one and severs the rest of the wallet's history. Raising the constant is the wrong fix: any fixed K is arbitrary, costs every normal wallet, and still fails for the first run longer than K. This adds the wallet half of adaptive gap-probe escalation: AddressPool::probe_gap_limit derives the window a temporarily widened gap limit would require and then restores the steady-state limit without un-deriving (pools only grow; the probed tail stays monitored). ManagedAccountCollection::probe_extend_gap_with fans that out over every funds-bearing account's pools using the same key-source lookup the wallet checker uses for gap maintenance, and WalletInterface::probe_extend_gap (default: empty no-op) exposes it per wallet so a filter-sync driver can probe an escalating window against the chain's own compact filters and resume the chase past any hole it confirms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ommit The commit-time verification rescan proves a batch has been matched against every script derived so far - but the derived set itself only reaches gap_limit past each pool's usage frontier. A run of unused indices longer than the gap limit therefore stalls discovery with every gate looking clean: the batch commits, committed batches are never rescanned, and all history past the hole is silently lost. The production wallet this was found on has 273 unused-index runs of 30 or more (the longest exactly 100) in 345k used addresses; gap-30 discovery froze at the first one and missed 890k of its transactions, and experiment confirmed gap limit 100 still freezes at the length-100 gap while 101 discovers everything. A bigger fixed gap is the wrong fix: arbitrary, paid by every wallet, and still beaten by the next longer run. Instead the steady state stays gap-30 and the stall itself triggers a bounded probe against the chain's own filters. When a batch with wallet activity reaches its commit fixpoint (verification clean, no blocks in flight anywhere), the manager walks a per-wallet probe ladder of 100/300/1000: derive the widened window through the new WalletInterface::probe_extend_gap seam (which restores the steady-state gap before returning), re-match the freshly derived scripts against this batch and every later scanned batch, and resume the normal chase on any hit. A rung that derives nothing escalates immediately, ladder progress is monotone per wallet and resumes where it left off, and real progress (a confirmed transaction or a new derivation for the wallet) resets it - a moving frontier makes the next stall a new stall. Only when the maximum rung (MAX_GAP_LIMIT, the widest window a pool can derive) finds nothing does the batch commit: termination is now a positive verdict from the chain rather than a guess. Commit-time collected-script rescans now also drain every scanned batch rather than only the lowest: a chase running inside a later batch used to stall with fresh scripts sitting unrescanned while an earlier batch was considered for commit, and the probe would fire on a lull that was never a wallet fixpoint. The dust-restore harness gains configurable unused-index holes and verifies the ladder end to end: a hole of 54 (production's first gap) crossed by the first rung, a hole of 150 requiring escalation, and a two-hole terminal run proving the sync completes, the frontier lands exactly on the last used index, and pool growth stays bounded by frontier + MAX_GAP_LIMIT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Debug-build siphash is roughly 50x slower than optimized, and BIP158 compact-filter matching hashes every query element against every filter, so the dust-restore and gap-probe regression tests spent most of their wall time inside it. dashcore_hashes is a rarely edited leaf crate: opting it into opt-level 2 for dev costs one slightly longer cold build and speeds every filter- and hash-heavy test severalfold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eness Review follow-up: note that probe_levels is in-memory scan state (a restart cheaply re-walks derive-nothing rungs), and that the confirmed-transaction reset intentionally covers every wallet the block was processed for because BlockProcessed does not attribute confirmed txids per wallet - over-resetting is the safe direction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Simplification pass over the probe-escalation code, no behavior change: drain collected scripts in one iter_mut pass instead of collect-then-relookup, hoist the batch's full-match generation into a shared binding, fold the committing batch and later scanned batches into one rescan-target list, write each wallet's probe level once after the rung loop instead of at three exit points, and import the types probe_extend_gap_with names instead of spelling full paths. All five dust-restore tests re-run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds adaptive wallet gap probing to compact-filter synchronization. Wallet address pools derive wider windows on demand. Filter batches track matched wallets and script generations. Batch commits rescan and verify scripts before using an escalating probe ladder. ChangesAdaptive wallet gap probing
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR adds adaptive gap-limit probing to improve wallet address discovery; no actionable merge-blocking risk remains at the current head. Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
#846 acceptance tests pass against this branch — including the cross-commit reproAuthor of #866 here. I ran the #846 acceptance suite (the deterministic repros ported from
The cross-commit case passes because the probe fires before the batch commits: the inverted indices sit within the first probe rung of the frontier, so the early block is caught while its batch is still active — prevention where #866 does after-the-fact recovery. Nice property: it also means the "#851 skips recovered fully-spent txs from history" interaction (reported on #851) largely doesn't arise here, since blocks are found before their spends are processed rather than re-applied afterwards. Two follow-ups:
If this merges, I'll close #866 in its favor — the unbounded committed-range sweep can be revisited as a follow-up if a real wallet ever demonstrates the beyond-rung-1,000 shape. 🤖 Generated with Claude Code |
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
2 participants
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.
The problem
BIP44 discovery derives only
gap_limit(30) addresses past the highest used index. A wallet whose on-chain usage skips more than 30 consecutive indices anywhere in its history silently loses everything past that hole: discovery concludes "no more history," sync completes normally, and the wallet under-reports funds and transactions with no error.This is not hypothetical. A real mainnet wallet (dust/spam target, 297,967 used external addresses, 890,840 transactions) has 273 unused-index runs ≥ 30 in its usage space. Discovery stops at the first one — indices 2,371–2,424 — finding 8,076 of 890,840 transactions.
Why a bigger constant is the wrong fix
We validated the mechanism by experiment against that wallet (each a full mainnet sync from a checkpoint):
highest_used = 2,370— 8,076 txshighest_used = 36,342— exactly one index short of the wallet's single length-100 hole — 95,212 txshighest_used = 345,256, 890,837 txsEvery fixed constant K fails identically for the wallet with a K+1 hole, silently; and raising the default taxes every normal wallet with a permanently larger scan set.
The fix: adaptive probe escalation, terminated by chain evidence
Steady state stays gap-30. When discovery reaches a genuine fixpoint (a filter batch with wallet activity is fully chased and about to commit), the wallet probes: it temporarily widens the derivation window (100 → 300 → 1,000 =
MAX_GAP_LIMIT), re-matches the newly derived scripts against the batch's (and later active batches') compact filters, andNormal wallets pay one cheap probe ladder at the true end of their history. Gap-riddled wallets self-heal at every hole without anyone choosing a number for them.
Builds on two earlier correctness fixes to the batch-commit flow included in this branch (full-set verification rescan before commit; commit held while any active batch has blocks in flight), which the probe ladder requires to define a sound fixpoint.
Validation
End-to-end (the acceptance test): full mainnet sync of the 890k-tx wallet with default gap-30 + this feature:
highest_used = 345,256(the wallet's true maximum used index), 890,837 transactions — byte-identical outcome to the gap-101 control run — crossing all 273 holes via 279 probe events, terminating on an explicit rung-1,000 probe. ~52 min wall vs ~12 min for the (overfitted) fixed-gap control; the overhead is bounded probe rescans, paid only at genuine stalls.Tests (deterministic, real
FiltersManager+WalletManagerover synthetic chains):dust_restore_crosses_gap_hole_of_54— production's first hole shape, crossed at rung 100 with default gapdust_restore_crosses_gap_hole_of_150_via_escalation— requires rung 300dust_restore_terminates_after_last_payment— completion, exact final frontier, bounded pool growth (≤ max_used + 1,000 + slack)cargo test -p dash-spv --lib --all-features: 551 passed; key-wallet / key-wallet-manager suites green; clippy-D warningscleanNotes
MAX_GAP_LIMIT, documented.🤖 Generated with Claude Code
Summary by CodeRabbit