feat(key-wallet): widen DEFAULT_COINJOIN_GAP_LIMIT 30 -> 100 for dashj parity (proposal)

[bfoss765]

Summary (proposal — for discussion)

Widen DEFAULT_COINJOIN_GAP_LIMIT from 30 → 100 to match dashj's
DeterministicKeyChain lookahead (DEFAULT_LOOKAHEAD_SIZE = 100), the effective discovery
window dashj-core watches for CoinJoin keychains.

I'm raising this as a proposal with the evidence both ways, not a bug fix, because the
right value ties into an ongoing team discussion (see the end). The change itself is a
one-line constant + doc comment; the substance is the reasoning below.

TL;DR: this is window dimensioning, not a scan-algorithm fix. The progressive
rescan-to-quiescence logic is already correct at gap 30 for any spray whose unused runs are
≤ 30. But heavy-mixing wallets in the field contain runs of > 30 consecutive unused
CoinJoin addresses, which no scan order can bridge at gap 30 — dashj finds them only
because its window is 100.

Background: what the gap limit actually bounds

CoinJoin discovery here is dynamic. The address pool watches highest_used + gap_limit
scripts and extends forward as addresses get used
(AddressPool::maintain_gap_limit, key-wallet/src/managed_account/address_pool.rs:905;
mark_used, :717). The compact-filter layer already implements dustinface's
finalize-then-rescan-to-quiescence loop: commit batch N, rescan the batch (and later
scanned batches) with the gap-limit-revealed scripts, repeat until no new block is
processed, then advance
(FiltersManager::try_commit_batches, dash-spv/src/sync/filters/manager.rs:476;
rescan_batch, :636; BlockProcessed feedback in
dash-spv/src/sync/filters/sync_manager.rs).

So the gap limit does not control scan order or convergence. It controls exactly one
thing: the largest run of consecutive unused CoinJoin addresses the wallet can bridge
before discovery stalls.

Evidence (1): the scan algorithm is already correct at gap 30

Two runnable reproductions drive the real key-wallet gap-extension machinery
(mark_used + maintain_gap_limit + block processing + monitored_script_pubkeys):

Real-wallet harness (real WalletManager + real CoinJoin account, spraying synthetic
blocks to chosen external-pool indices, then driving process → widen watched set → rescan
→ repeat):

Pattern	Indices (Δ)	Gap 30	Gap 100
A dense sequential	0,1,…,60 (Δ1)	61/61 discovered, no stall	61/61
B sparse-within-gap	5,34,61,88 (Δ29,27,27)	4/4 discovered, no stall	4/4
C run-exceeds-gap (control)	5,40,90 (Δ35,50)	1/3, stalls at 40 (correct)	3/3

dash-spv filter-orchestration probes drove the real FiltersManager loop
(scan_batch + handle_sync_event(BlockProcessed) + try_process_batch) with a
gap-limit-modelling mock wallet, for a within-batch spray [5,34,61,88] and a cross-batch
spray [(3000,5),(4990,34),(6000,61),(9000,88)] straddling the 5000-block batch boundary.
Both fully discovered at gap 30.

Pattern B (three consecutive Δ≈28 gaps, indices out to 88) fully discovering at gap 30 is
the key result: it shows convergence is not limited by total index reach or by batch
boundaries, only by the largest single unused run. Pattern C confirms the boundary is
exactly the gap: Δ=30 bridgeable, Δ=35 not. The math is off-by-nothing.

Evidence (2): heavy-mixing field wallets have unused runs > 30

• A team wallet required gap ≥ 48 to resolve its full balance — i.e. it genuinely
  contains a run of ~31–47 consecutive unused CoinJoin addresses between two on-chain-used
  ones.
• An Android SDK test wallet (a ~12.09 tDASH reference heavy-mixer; internal two-word
  identifier "…duck say") resolves its full balance at gap 100 but not at gap 30 — again
  from a clean re-creation + from-birth rescan, so it is not a sync/ordering artifact.

No correct scan algorithm can bridge a run of > 30 unused addresses at gap 30 — the wallet
literally isn't watching the address that got used past the window, so it never learns to
extend. dashj discovers these txs solely because its lookahead is 100. At gap 30 the SDK
misses both the tx that created the far-index UTXO and (once it stalls) later txs that
spend nearer ones, which is what produced the persistent balance mismatch vs dashj in
dashpay/dash-wallet#1507.

Conclusion

This is window dimensioning, not a scan bug. Reverting to 30 would reintroduce the field
failure, and "correct balance at gap 30" is unachievable by any correct algorithm for a
wallet whose mixing left a > 30 unused run. 100 aligns our window with dashj's so the SDK
discovers every CoinJoin tx dashj does.

Cost is modest: the CoinJoin account watches two pools (external + internal), so this
pre-derives and filter-matches ~200 CoinJoin scripts per account instead of ~60. BIP158
matching is a set intersection, so the extra scripts add negligible per-block work; the
derivation is a one-time keychain expansion.

Open question / team discussion

The "right" CoinJoin window came up in the June Slack thread between @dustinface,
@QuantumExplorer, and @HashEngineering (progressive dynamic scan vs. fixed lookahead
sizing). This PR takes the "match dashj's 100" position, but I'd like that discussion to
land here on the PR before merge. Specifically:

• Is 100 the value we want long-term, or a different fixed number?
• Should the CoinJoin window instead be configurable / network-dependent?
• Is there appetite for a follow-up that makes the dynamic scan bridge arbitrary unused
  runs (e.g. a birthday-bounded exhaustive pass for CoinJoin keychains), which would make
  the constant a performance knob rather than a correctness floor?

Separate, non-blocking observation

While mapping the scan loop I noticed try_commit_batches propagates newly-revealed scripts
forward only (it rescans the current and later scanned batches, filter
start > batch_start, but never re-scans an already-committed lower-height batch). For
CoinJoin this never bites — an address pool assigns the lowest unused index first, so index
order tracks height order and window extensions always propagate forward in height. But it's
a latent correctness gap for non-monotonic / late-added-wallet / cross-account-type cases
where a script revealed by a higher-height block is needed by an already-committed
lower-height batch. Worth a separate ticket; it does not affect the gap-30 CoinJoin
outcome above.

Verification

• cargo test -p key-wallet — 547 passed, 0 failed
• cargo test -p key-wallet-manager — 58 passed, 0 failed
• cargo clippy -p key-wallet --all-features --all-targets -- -D warnings — clean
• cargo fmt -p key-wallet -- --check — clean

Refs: dashpay/platform#4074, dashpay/dash-wallet#1507

cc @HashEngineering @QuantumExplorer @dustinface

[coderabbitai]

Warning

Review limit reached

@bfoss765, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 892eefda-3629-4d63-aa1a-4771cf5ce29e

📥 Commits

Reviewing files that changed from the base of the PR and between 845ee9e and 54fdb31.

📒 Files selected for processing (1)

• key-wallet/src/gap_limit.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.67%. Comparing base (845ee9e) to head (54fdb31).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #868      +/-   ##
==========================================
+ Coverage   73.62%   73.67%   +0.05%     
==========================================
  Files         324      324              
  Lines       73374    73374              
==========================================
+ Hits        54018    54055      +37     
+ Misses      19356    19319      -37     

Flag	Coverage Δ
core	77.08% <ø> (ø)
ffi	46.43% <ø> (+0.34%)	⬆️
rpc	20.00% <ø> (ø)
spv	90.96% <ø> (+0.02%)	⬆️
wallet	73.11% <ø> (ø)

Files with missing lines	Coverage Δ
key-wallet/src/gap_limit.rs	70.68% <ø> (ø)

... and 20 files with indirect coverage changes

[xdustinface]

I think makes sense yeah i had CoinJoin test wallets here where 30 wasnt enough.

[bfoss765]

Post-merge validation datapoint for this change (full detail in #873 comments): on a real 4,517-CoinJoin-key testnet wallet, a fresh SDK wallet at gap 100 reaches byte-exact parity with a dashj oracle in ~6 minutes on its first scan. The same test at gap 30 (with the #873 re-scan fix included) failed its first scan — zero transaction records persisted, no self-recovery — so 30 is not currently viable for heavy-CoinJoin restores. The 30→100 change this PR shipped stands validated; no revert proposed.