feat: per-wallet filter scan and runtime wallet catch-up#122
Merged
Conversation
Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without disturbing the wallets that are already in sync. `WalletInterface` is restructured around per-wallet operations: - `process_block_for_wallets(block, height, wallets)` replaces the global `process_block` and only updates the listed wallets. - `wallets_behind(height)` returns the wallet ids that still need filter coverage at `height`. - `monitored_addresses_for(wallet_id)` and `wallet_synced_height(wallet_id)` give per-wallet projections for filter matching. - `update_wallet_synced_height` and `update_wallet_last_processed_height` advance one wallet at a time and are monotonic. - `BlockProcessingResult.new_addresses` and `CheckTransactionsResult.new_addresses` carry gap-limit discoveries with wallet attribution. `FiltersManager.scan_batch` matches each behind wallet's addresses against the batch's filters at heights it hasn't yet covered. The per-block result flows through `BlocksNeeded` to `BlocksManager`, which processes each block only against the wallets whose filters matched it. `FiltersBatch` records the scanned wallet set so commit advances only their `synced_height`. When a late-added wallet's filter matches a block already in flight, its id is merged into the existing entry. If the block has already been processed, it is re-queued so `BlocksManager` reloads it from local storage and processes it for the late wallet only. `process_block_for_wallets` refreshes the cached balance even on rescan paths below the wallet's current `last_processed_height`, because UTXOs may change.
When `wallet.synced_height()` drops below `FiltersManager`'s `progress.committed_height()`, a wallet was added or moved behind the current scan position and needs catch-up coverage. Add a check at the top of `FiltersManager::tick()` that detects this regression, clears in-flight pipeline state, lowers `committed_height` to the new aggregate min, and re-enters `start_download()`. The check runs in `Syncing`, `Synced`, and `WaitForEvents` states so idle additions are caught on the next 100ms tick. Add `test_wallet_added_at_runtime_catches_up` in `dash-spv/tests/dashd_sync/tests_basic.rs`. After initial sync with `W1`, mine a block funding `W2`'s address and add `W2` at runtime with `birth_height` before that block. Assert the rescan picks up `W2`'s funding transaction and `W1`'s state is unchanged. Then add `W3` with `birth_height` beyond the tip and assert no spurious rescan or regression in either existing wallet.
|
Manki — Review complete Planner (22s) Review — 25 findings Judge — 21 kept · 0 dropped (52s) Review metadataConfig:
Judge decisions:
Timing:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #122 +/- ##
=============================================
+ Coverage 70.50% 70.61% +0.10%
=============================================
Files 319 319
Lines 66758 67590 +832
=============================================
+ Hits 47071 47726 +655
- Misses 19687 19864 +177
🚀 New features to boost your workflow:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Per-wallet filter scan looks well-structured, but two real risks lurk: the pipeline keys wallet sets by expected hash and looks them up by actual block hash, and the rescan path silently relies on process_block_for_wallets not short-circuiting on last_processed_height.
📊 21 findings (5 warning, 6 suggestion, 10 nitpick) · 1128 lines · 563s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 562507,
"diffLines": 1128,
"diffAdditions": 858,
"diffDeletions": 270,
"filesReviewed": 20,
"agents": [
"Correctness & Logic",
"Security & Safety",
"Architecture & Design",
"Testing & Coverage"
],
"findingsRaw": 25,
"findingsKept": 21,
"findingsDropped": 4,
"severity": {
"blocker": 0,
"warning": 5,
"suggestion": 6,
"nitpick": 10
},
"verdict": "REQUEST_CHANGES",
"prNumber": 122,
"commitSha": "bf2f2241df18f0aef560d01f3498792d6792ce55",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 6,
"findingsKept": 5,
"responseLength": 7095
},
{
"name": "Security & Safety",
"findingsRaw": 5,
"findingsKept": 4,
"responseLength": 5266
},
{
"name": "Architecture & Design",
"findingsRaw": 7,
"findingsKept": 7,
"responseLength": 8574
},
{
"name": "Testing & Coverage",
"findingsRaw": 7,
"findingsKept": 7,
"responseLength": 8262
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 8,
"medium": 13,
"low": 0
},
"severityChanges": 21,
"mergedDuplicates": 4,
"defensiveHardeningCount": 4,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 20
},
"findingsPerFile": {
"dash-spv/src/sync/blocks/pipeline.rs": 3,
"dash-spv/src/sync/filters/manager.rs": 12,
"dash-spv/src/sync/blocks/manager.rs": 2,
"dash-spv/src/sync/blocks/sync_manager.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 1,
"key-wallet-ffi/src/wallet_manager_tests.rs": 1,
"dash-spv/src/sync/filters/batch.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Encapsulate the off-by-one between `wallets_behind` (strict less-than) and the inclusive height semantics needed by `scan_batch`, so call sites no longer need to compensate with `saturating_add(1)`.
Single-wallet `MockWallet` cannot exercise paths that hinge on multiple wallets having distinct `synced_height` values or independent address sets. `MultiMockWallet` holds per-wallet state keyed by `WalletId`.
Resolves several correctness issues in the per-wallet filter scan and catch-up paths: - \`track_block_match\` now distinguishes three states (\`NewlyTracked\`, \`InFlight\`, \`AlreadyProcessed\`) instead of a boolean. A block that was already processed in a prior round is no longer silently re-queued for re-processing, and a block still in flight still re-emits \`BlocksNeeded\` so the \`BlocksPipeline\` merges late-arriving wallet ids into its pending wallet set. - \`scan_batch\` only adds wallets that contributed addresses to the scan into \`scanned_wallets\`. Empty-address wallets no longer have their \`synced_height\` advanced for free. - \`rescan_batch\` no longer drops new wallet ids when the matching block is in flight; it forwards the wallet ids through a fresh \`BlocksNeeded\`, which the pipeline merges into its pending set. - \`tick\` resets \`committed_height\` to the lowest \`synced_height\` of the stale wallets only, instead of the global wallet \`synced_height\`, so already-synced wallets are not re-scanned from scratch. - \`scan_batch\` runs the filters once over the union of behind-wallet addresses, then attributes each match per-wallet by re-testing the matched filter against that wallet's scripts. Cost drops from O(N_wallets * batch_size) to O(batch_size + N_wallets * matches), and the per-batch borrow of \`active_batches\` is consolidated. - Field \`filters_matched\` renamed to \`matched_block_hashes\` to reflect its actual role as a record (not a deduplication gate). - \`rescan_batch\` now takes \`addresses_by_wallet\` by reference, saving a clone per later-batch rescan in \`try_commit_batches\`. - Uses \`wallets_not_yet_at(batch_end)\` to express the inclusive height intent, hiding the \`+ 1\` off-by-one at the call site.
…eline` Cover the previously-implicit contract that `queue` and `add_from_storage` merge wallet sets per block hash, and that `take_next_ordered_block` returns the merged wallet set.
|
Manki — Review complete Planner (29s) Review — 22 findings Judge — 16 kept · 3 dropped (48s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Two reviewers independently flag the same data-integrity gap: wallets added mid-session silently miss any block already in matched_block_hashes (findings 2, 7, 11), and zero-address behind-wallets perpetually re-trigger scan work without ever advancing synced_height (findings 6, 12). Several previous threads remain unaddressed — particularly the missing tests for InFlight re-emission, per-wallet rescan attribution, and the union+attribution false-positive elimination.
📊 16 findings (6 warning, 5 suggestion, 5 nitpick) · 1695 lines · 906s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 905806,
"diffLines": 1695,
"diffAdditions": 1411,
"diffDeletions": 284,
"filesReviewed": 20,
"agents": [
"Security & Safety",
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"findingsRaw": 22,
"findingsKept": 16,
"findingsDropped": 6,
"severity": {
"blocker": 0,
"warning": 6,
"suggestion": 5,
"nitpick": 5
},
"verdict": "REQUEST_CHANGES",
"prNumber": 122,
"commitSha": "1ab329241813fbe9c9805dc9f9149ecd612754d4",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 5522
},
{
"name": "Correctness & Logic",
"findingsRaw": 5,
"findingsKept": 4,
"responseLength": 7105
},
{
"name": "Architecture & Design",
"findingsRaw": 5,
"findingsKept": 3,
"responseLength": 6657
},
{
"name": "Testing & Coverage",
"findingsRaw": 7,
"findingsKept": 6,
"responseLength": 5776
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 16,
"medium": 3,
"low": 0
},
"severityChanges": 19,
"mergedDuplicates": 3,
"defensiveHardeningCount": 3,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 20
},
"findingsPerFile": {
"dash-spv/src/sync/filters/manager.rs": 12,
"dash-spv/src/sync/blocks/pipeline.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 1,
"dash-spv/src/sync/blocks/manager.rs": 1,
"dash-spv/src/sync/events.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Tighten `scan_batch` so wallets at `synced == batch_end` are skipped (use `wallets_behind(batch_end)` instead of `wallets_not_yet_at`), ensure behind wallets with zero monitored addresses still advance their `synced_height` at commit, and surface filter errors and `AlreadyProcessed` skips through `tracing::warn!` instead of silent fallthrough. Add a `min_height` parameter to `check_compact_filters_for_addresses` so the union pass can skip irrelevant heights without cloning the filter map. Reuse the existing `batch_filters` borrow for attribution. Add tests covering: - `scan_batch` advancing zero-address wallets - `BlockTrackResult::InFlight` re-emission for late-arriving wallets - `BlockTrackResult::AlreadyProcessed` skip path - Union+attribute false-positive elimination - Rescan from non-zero `synced_height` (not just genesis) - `BlocksManager::process_buffered_blocks` routing the wallet set
|
Manki — Review complete Planner (21s) Review — 18 findings Judge — 17 kept · 0 dropped (82s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
New round adds attribution-pass safety concerns and runtime catch-up gaps. Finding 1 (re-enqueue corrupting coordinator state) and finding 3 (storage-loaded blocks never advance per-wallet synced_height) are the load-bearing correctness issues; the rest are real but lower severity.
📊 17 findings (2 blocker, 5 warning, 6 suggestion, 4 nitpick) · 2015 lines · 1098s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 1097640,
"diffLines": 2015,
"diffAdditions": 1720,
"diffDeletions": 295,
"filesReviewed": 22,
"agents": [
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage",
"Performance & Efficiency"
],
"findingsRaw": 18,
"findingsKept": 17,
"findingsDropped": 1,
"severity": {
"blocker": 2,
"warning": 5,
"suggestion": 6,
"nitpick": 4
},
"verdict": "REQUEST_CHANGES",
"prNumber": 122,
"commitSha": "62a70d92c393eb2f31d1b0c660e2b5584f936ceb",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 5384
},
{
"name": "Architecture & Design",
"findingsRaw": 5,
"findingsKept": 4,
"responseLength": 5905
},
{
"name": "Testing & Coverage",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 5865
},
{
"name": "Performance & Efficiency",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4814
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 11,
"medium": 6,
"low": 0
},
"severityChanges": 17,
"mergedDuplicates": 0,
"defensiveHardeningCount": 1,
"verdictReason": "required_present"
},
"fileMetrics": {
"fileTypes": {
".rs": 21,
".toml": 1
},
"findingsPerFile": {
"dash-spv/src/sync/blocks/pipeline.rs": 1,
"dash-spv/src/sync/filters/manager.rs": 11,
"dash-spv-ffi/src/callbacks.rs": 2,
"dash-spv/src/sync/blocks/sync_manager.rs": 2,
"dash-spv/src/sync/blocks/manager.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}`BlocksPipeline::queue` unconditionally called `coordinator.enqueue` for every entry, including hashes that were already pending or in flight from a prior call. When a late-arriving wallet match for an already-queued block was emitted as `BlocksNeeded`, the duplicate `enqueue` corrupted the coordinator's pending count and could trigger a duplicate request to the peer. Only enqueue when the hash is not yet tracked, while still merging the late wallet ids into the per-block wallet set.
`scan_batch` recorded the per-batch `scanned_wallets` set only after the empty-filters early return, so a batch that hit that path was committed with an empty set and the per-wallet `synced_height` never advanced for that range. The next tick relisted the same wallets via `wallets_behind` and re-scanned the same range. Move the `set_scanned_wallets` call ahead of the empty-filter and empty-address fast paths so every behind wallet's `synced_height` advances when the batch commits. Also pass each wallet's own `synced_height` as `min_synced` to `check_compact_filters_for_addresses` in `rescan_batch`. Otherwise a new address could spuriously match heights the wallet has already processed and `track_block_match` would route the result into `AlreadyProcessed` and silently drop it.
Add a `BlocksManager` test that asserts a wallet absent from the pipeline's interested set never receives `process_block_for_wallets`, complementing the existing positive routing test. Add `dash-spv-ffi` callback tests covering the two behavioural changes of this PR: `BlocksNeeded` dispatch reports unique `FilterMatchKey` count (not inflated by the per-block wallet attribution), and `BlockProcessed` dispatch reports the total address count summed across the per-wallet `new_addresses` map.
|
Manki — Review complete Planner (26s) Review — 13 findings Judge — 2 kept · 3 dropped (217s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Three of the five new findings rest on incorrect premises about how hash_to_height is managed: receive_block already removes entries (it's not a permanent sentinel), and scanned_wallets is verifiably set from the full behind set on line 762 — not the truncated mystery the reviewer feared. Only finding 1 (storage-loaded blocks bypass the dedup invariant) is a clean catch worth a defensive fix.
📊 2 findings (2 nitpick) · 2321 lines · 796s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 796117,
"diffLines": 2321,
"diffAdditions": 2011,
"diffDeletions": 310,
"filesReviewed": 22,
"agents": [
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage",
"Dependencies & Integration"
],
"findingsRaw": 13,
"findingsKept": 2,
"findingsDropped": 11,
"severity": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 2
},
"verdict": "APPROVE",
"prNumber": 122,
"commitSha": "b78ba3172c4b917c24f8af99bc6de7ebf6deb823",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 1,
"responseLength": 5390
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"responseLength": 8091
},
{
"name": "Testing & Coverage",
"findingsRaw": 5,
"findingsKept": 1,
"responseLength": 5990
},
{
"name": "Dependencies & Integration",
"findingsRaw": 4,
"findingsKept": 0,
"responseLength": 5313
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 5,
"medium": 0,
"low": 0
},
"severityChanges": 5,
"mergedDuplicates": 0,
"defensiveHardeningCount": 1,
"verdictReason": "only_nit_or_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 21,
".toml": 1
},
"findingsPerFile": {
"dash-spv/src/sync/blocks/pipeline.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}
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.
Rebased onto
v0.42-devafter #117 was squash-merged.Per-wallet filter scan and runtime wallet catch-up implementation.
Part of the v0.42 series.