feat: implement parallel filter matching #303
Conversation
📝 WalkthroughWalkthroughRefactors compact-filter matching from a wallet-driven async check to an address-driven, parallel matching module; updates WalletInterface, wallet manager, SPV message handling, tests, and adds rayon dependency and new matching module with unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant SyncMgr as SyncManager
participant WalletMgr as WalletManager
participant Matching as FilterMatching
participant BlockStore as BlockStorage
Note over SyncMgr,WalletMgr: Old flow (removed): wallet-owned filter check
SyncMgr->>WalletMgr: check_compact_filter(filter, block_hash)
activate WalletMgr
WalletMgr->>WalletMgr: examine monitored addresses & match
WalletMgr-->>SyncMgr: bool (match/no-match)
deactivate WalletMgr
Note over SyncMgr,Matching: New flow (added): address-driven, parallel matching
SyncMgr->>WalletMgr: monitored_addresses()
activate WalletMgr
WalletMgr-->>SyncMgr: Vec<Address>
deactivate WalletMgr
SyncMgr->>Matching: check_compact_filters_for_addresses(filters_map, addresses)
activate Matching
Matching->>Matching: parallel iterate filters (rayon)
Matching->>Matching: for each filter: match_any(address_scripts, hash)
Matching-->>SyncMgr: BTreeSet<FilterMatchKey> (matches)
deactivate Matching
alt matches found
SyncMgr->>BlockStore: request blocks for matched heights
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
f70a140 to
f4cd072
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
changed the title
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
f4cd072 to
b00b66c
Compare
github-actions
bot
removed
the
merge-conflict
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
b00b66c to
bf5bb3a
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
bf5bb3a to
b7b44b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@key-wallet-manager/Cargo.toml`:
- Line 28: The crate currently unconditionally depends on rayon (rayon =
"1.11"), which breaks no_std builds; make the dependency optional and tie it to
the crate's std feature by changing the Cargo.toml entry to an optional
dependency (rayon = { version = "1.11", optional = true }) and add a features
table that maps the std feature to enable rayon (e.g. features = { std =
["rayon"] }); also audit uses of Rayon types/traits in code (functions/modules
that reference rayon) and wrap those with cfg(feature = "std") so compilation
succeeds for no_std builds.
In `@key-wallet-manager/src/wallet_manager/matching.rs`:
- Around line 122-124: The test creates Block::dummy instances (block_2,
block_3) at height 100 but constructs FilterMatchKey with mismatched heights
(FilterMatchKey::new(200, ...) and FilterMatchKey::new(300, ...)); update either
the Block::dummy calls to use heights 200 and 300 respectively or change the
FilterMatchKey::new calls to use 100 so the block heights and the FilterMatchKey
metadata (seen in FilterMatchOutput sorting) are consistent — locate
block_2/block_3 and their corresponding FilterMatchKey::new invocations to apply
the change.
In `@key-wallet-manager/tests/check_compact_filters_tests.rs`:
- Around line 34-35: The tests create Regtest dummy addresses but the wallet
under test uses Network::Testnet; update the Address::dummy calls to use
Network::Testnet so addresses match the wallet network (e.g., change
Address::dummy(Network::Regtest, 100) / Address::dummy(Network::Regtest, 101)
and the similar instances at the other locations to
Address::dummy(Network::Testnet, ...)); ensure the variables (unrelated_1,
unrelated_2 and the other dummy address variables around lines 65 and 75) are
constructed with Network::Testnet to keep network consistency.
- Around line 13-17: Replace the non-deterministic wallet creation that calls
WalletManager::<ManagedWalletInfo>::create_wallet_with_random_mnemonic with a
deterministic creation using create_wallet_from_mnemonic (or a deterministic
helper) passing a fixed test mnemonic like the standard test vector; update the
five test cases that currently use create_wallet_with_random_mnemonic (and
WalletAccountCreationOptions::Default) to call
WalletManager::create_wallet_from_mnemonic with the fixed mnemonic and the same
options so tests are reproducible and stable in CI.
🧹 Nitpick comments (2)
key-wallet-manager/src/wallet_manager/process_block.rs (1)
86-88: Consider running the rayon work in a blocking task.This async method performs CPU-bound rayon matching synchronously, which can block the async executor thread. A
spawn_blockingwrapper would keep the runtime responsive.♻️ Suggested change
async fn check_compact_filters(&self, input: FilterMatchInput) -> FilterMatchOutput { - check_compact_filters_for_addresses(input, self.monitored_addresses()) + let addresses = self.monitored_addresses(); + match tokio::task::spawn_blocking(move || { + check_compact_filters_for_addresses(input, addresses) + }) + .await + { + Ok(output) => output, + Err(_) => FilterMatchOutput::default(), + } }key-wallet-manager/src/wallet_manager/matching.rs (1)
32-49: Optional fast-path for empty inputsYou can skip allocation and Rayon scheduling when
inputoraddressesis empty.♻️ Proposed tweak
pub fn check_compact_filters_for_addresses( input: FilterMatchInput, addresses: Vec<Address>, ) -> FilterMatchOutput { + if input.is_empty() || addresses.is_empty() { + return FilterMatchOutput::new(); + } let script_pubkey_bytes: Vec<Vec<u8>> = addresses.iter().map(|address| address.script_pubkey().to_bytes()).collect();
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
About the performance improvement, since it is cause by parallelizing a collection processing, would be more useful to comment what is the performance improvement per core used than throwing a number without saying the number of cores
| pub async fn check_filters_for_matches< | ||
| W: key_wallet_manager::wallet_interface::WalletInterface, | ||
| >( | ||
| &self, |
There was a problem hiding this comment.
Whats the idea of this function?? Dont see any usage of it and having a method function that wraps other function without using the self field feels pointless
|
|
||
| async fn check_compact_filters(&self, input: FilterMatchInput) -> FilterMatchOutput { | ||
| check_compact_filters_for_addresses(input, self.monitored_addresses()) | ||
| } |
There was a problem hiding this comment.
why dont you include the matching.rs module logic inside this method?? That way we can also take rid of the unit tests you wrote in matching.rs since they are testing the same logic the new integrations tests are testing
There was a problem hiding this comment.
Need to match for specific addresses too so moving the logic in here wouldnt work. But i dropped this and added monitored_addresses to the wallet interface instead so we can use the standalone function where we need to match filters.
| } | ||
|
|
||
| async fn check_compact_filters(&self, input: FilterMatchInput) -> FilterMatchOutput { | ||
| check_compact_filters_for_addresses(input, self.monitored_addresses()) |
There was a problem hiding this comment.
Since this is not used yet, can you mark it with a TODO or something to make it explicit and what is this method waiting for??
| height: CoreBlockHeight, | ||
| hash: BlockHash, | ||
| } | ||
|
|
There was a problem hiding this comment.
I cannot give suggestions about this struct since I don't know how is it gonna be used
| filter | ||
| .match_any(key.hash(), script_pubkey_bytes.iter().map(|v| v.as_slice())) | ||
| .unwrap_or(false) | ||
| .then_some(key) |
There was a problem hiding this comment.
Not sure about what I am about to say but would like to comment it anyway just in case. I think this method can only return result if the internal Reader fails to read, but since everything is in memory I expect to always succeed. I feel like we can be missing possible bugs by silently unwrapping to false since thats the only reason a Err would be returned. At the same time I feel like it is impossible to have a bug that makes this return an Err making all this comment irrelevant
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
b7b44b3 to
564af04
Compare
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-manager/src/wallet_interface.rs (1)
62-70: Docs contradict the return type inearliest_required_height.The documentation states the default returns
Noneand references a "specified network," but the method signature returnsCoreBlockHeightwith no network parameter and defaults to0. Update the documentation to match the actual implementation:
- Remove the reference to "specified network" (no such parameter exists)
- Clarify that the default returns
0as a sentinel value, notNone- Document that callers should handle
0as "unknown" height✍️ Suggested doc fix
- /// Return the earliest block height that should be scanned for this wallet on the - /// specified network. Implementations can use the wallet's birth height or other + /// Return the earliest block height that should be scanned for this wallet. + /// Implementations can use the wallet's birth height or other /// metadata to provide a more precise rescan starting point. /// - /// The default implementation returns `None`, which signals that the caller should - /// fall back to its existing behaviour. + /// The default implementation returns `0` as a sentinel for "unknown"; callers + /// should fall back to their existing behaviour in that case. async fn earliest_required_height(&self) -> CoreBlockHeight { 0 }
♻️ Duplicate comments (2)
key-wallet-manager/Cargo.toml (1)
28-28: Gaterayonbehind thestdfeature if no_std is supported.If this crate still advertises no_std builds (e.g.,
#![cfg_attr(not(feature="std"), no_std)]), an unconditionalrayondependency will fail compilation because rayon is std-only. Either gate rayon behind thestdfeature or drop no_std support explicitly.💡 Suggested Cargo.toml adjustment
[features] -default = ["std", "bincode"] -std = ["key-wallet/std", "dashcore/std", "dashcore_hashes/std", "secp256k1/std"] +default = ["std", "bincode"] +std = ["key-wallet/std", "dashcore/std", "dashcore_hashes/std", "secp256k1/std", "rayon"] [dependencies] -rayon = "1.11" +rayon = { version = "1.11", optional = true }#!/bin/bash # Verify whether key-wallet-manager still declares no_std and relies on std feature gating rg -n "cfg_attr\\(not\\(feature *= *\"std\"\\), *no_std\\)|no_std" --type rust rg -n "\\[features\\]"key-wallet-manager/src/wallet_manager/matching.rs (1)
37-43: Avoid swallowingmatch_anyerrors.Line 41–42 uses
unwrap_or(false), which turns filter errors into “no match” and can silently skip relevant blocks. Consider returning aResult(or at least surfacing/logging errors and handling them conservatively) so failures are visible.#!/bin/bash # Inspect BlockFilter::match_any error semantics to decide how to surface failures rg -n "fn match_any" --type rust rg -n "bip158|BlockFilter" --type rust
🧹 Nitpick comments (1)
key-wallet-manager/src/wallet_interface.rs (1)
49-50: Clarify coverage and cost expectations formonitored_addresses.This list drives filter matching; the “incoming” wording is ambiguous and may cause implementers to omit internal/change addresses or recompute heavily. Consider tightening the doc to avoid mis-implementations.
✍️ Doc tweak
- /// Get all addresses the wallet is monitoring for incoming transactions + /// Get all wallet addresses to monitor (external + internal/change outputs). + /// Implementations should avoid expensive recomputation (prefer cached lists). fn monitored_addresses(&self) -> Vec<Address>;
xdustinface
force-pushed
the
feat/parallel-filter-matching
branch
from
564af04 to
cc6937d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-manager/src/wallet_interface.rs (1)
62-68: Doc/behavior mismatch forearliest_required_height.Lines 62–67 say the default returns
None, but the signature returnsCoreBlockHeightand the default implementation returns0. Update the docs to reflect0(genesis) or change the signature toOption<CoreBlockHeight>ifNoneis intended.Doc-only fix
- /// The default implementation returns `None`, which signals that the caller should - /// fall back to its existing behaviour. + /// The default implementation returns `0` (genesis), which signals that the caller should + /// fall back to its existing behaviour if a more precise height is unavailable.
♻️ Duplicate comments (2)
key-wallet-manager/Cargo.toml (1)
28-28: Gaterayonbehind thestdfeature (or drop no_std).Line 28 adds a std-only dependency. If key-wallet-manager still advertises no_std, this will break no_std builds. Make
rayonoptional and tie it to thestdfeature (and gatematching.rsaccordingly), or explicitly remove no_std support. As per coding guidelines, use conditional compilation for optional features.Proposed Cargo.toml update
[features] -default = ["std", "bincode"] -std = ["key-wallet/std", "dashcore/std", "dashcore_hashes/std", "secp256k1/std"] +default = ["std", "bincode"] +std = ["key-wallet/std", "dashcore/std", "dashcore_hashes/std", "secp256k1/std", "rayon"] [dependencies] - ray on = "1.11" +rayon = { version = "1.11", optional = true }key-wallet-manager/src/wallet_manager/matching.rs (1)
37-43: Don’t silently treatmatch_anyerrors as “no match”.At Line 41–42,
unwrap_or(false)hides filter read/parse errors. A corrupted filter would be treated as non-matching and could suppress relevant block downloads. Consider surfacing errors (e.g., returnResultand let callers resync/request the block) or at least log and treat errors as matches to avoid false negatives. As per coding guidelines, propagate errors appropriately.
| /// Check compact filters for addresses and return the keys that matched. | ||
| pub fn check_compact_filters_for_addresses( | ||
| input: &HashMap<FilterMatchKey, BlockFilter>, | ||
| addresses: Vec<Address>, |
There was a problem hiding this comment.
since we are only using the hash inside FilterMatchKey it feels overkill to have a new struct for that. I also see kinda excesive a HashMap when we are only iterating over the entries, isn't possible to do:
pub fn check_compact_filters_for_addresses(
input: &[BlockFilter],
addresses: &[Address],
) -> BTreeSet<&BlockFilter> {
let script_pubkey_bytes: Vec<Vec<u8>> =
addresses.iter().map(|address| address.script_pubkey().to_bytes()).collect();
input
.into_par_iter()
.filter_map(|(filter)| {
filter
.match_any(filter.block_hash, script_pubkey_bytes.iter().map(|v| v.as_slice()))
.unwrap_or(false)
.then_some(key.clone())
})
.collect()
}And if we need the height at some point after this method call, we can query the storage using the filter_block_hash.
About the returned collections, since idk the usage of it I cant tell if a Vec<&BlockFilter> can do the job
There was a problem hiding this comment.
It's done like this to get sorted filter match outputs to request/process blocks in height order. Sure there might be other ways to do this but thats how its handled now in the sync rewrite. You can later come up with something else if you need to.
Introduce parallel filter matching for the wallet, not currently in use in this PR but preparation for follow up sync improvements. This leverages
rayonto match filters in parallel. This brings batch processing of 5000 filters in #257 from ~16s down to ~2s. I did some benchmarking on this and it appears to be the filter size which slows things down.Summary by CodeRabbit
New Features
Behavior Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.