feat: implement parallel filter matching #303
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new batch-level compact filter matching capability across the Dash wallet and SPV sync infrastructure. A new Changes
Sequence DiagramsequenceDiagram
participant SPV as FilterSyncManager
participant WalletMgr as WalletManager
participant Core as matching::core
participant Rayon as Parallel<br/>Processing
SPV->>WalletMgr: check_compact_filters(input_map)
WalletMgr->>WalletMgr: get_monitored_addresses()
WalletMgr->>Core: check_compact_filters_for_addresses(input_map, addresses)
Core->>Rayon: par_iter() over input entries
Rayon->>Rayon: for each (FilterMatchKey, BlockFilter):<br/>filter.match_any(hash, scripts)
Rayon-->>Core: collect matching keys
Core->>Core: sort by height (BTreeSet)
Core-->>WalletMgr: FilterMatchOutput (sorted keys)
WalletMgr-->>SPV: FilterMatchOutput
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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();
| async-trait = "0.1" | ||
| bincode = { version = "=2.0.0-rc.3", optional = true } | ||
| zeroize = { version = "1.8", features = ["derive"] } | ||
| rayon = "1.11" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the key-wallet-manager Cargo.toml for features and lib configuration
cat key-wallet-manager/Cargo.tomlRepository: dashpay/rust-dashcore
Length of output: 1445
🏁 Script executed:
# Search for any no_std usage or feature gates in key-wallet-manager
rg "no_std|default =" key-wallet-manager/Repository: dashpay/rust-dashcore
Length of output: 439
🌐 Web query:
rayon 1.11 MSRV Rust version requirement minimum supported
💡 Result:
Rayon 1.11 requires Rust (rustc) 1.80.0 or newer. [1][2]
Sources:
- Rayon README/docs (rayon 1.11): “Rayon currently requires rustc 1.80.0 or greater.” [1]
- rayon Cargo metadata showing rust-version = "1.80" for v1.11.0. [2]
🌐 Web query:
rayon 1.11 std support no_std compatibility
💡 Result:
Short answer: Rayon 1.11 is not no_std‑compatible — it is a std-only crate (no opt-out "std" feature) and depends on rayon-core and std threading. (docs.rs)
Sources:
- rayon Cargo.toml / features (shows no "std" feature; depends on rayon-core). (docs.rs)
- rayon README (describes threading, platform behavior and that Rayon targets std environments). (github.com)
Citations:
- 1: https://docs.rs/crate/rayon/latest/source/Cargo.toml?utm_source=openai
- 2: https://docs.rs/crate/rayon/latest/source/Cargo.toml?utm_source=openai
- 3: https://github.com/rayon-rs/rayon?utm_source=openai
Gate rayon behind the std feature.
The crate explicitly supports no_std builds (#![cfg_attr(not(feature = "std"), no_std)]), but rayon is a std-only crate and should not be unconditionally included. Move rayon = "1.11" into an optional dependency gated behind the std feature to maintain no_std compatibility.
🤖 Prompt for AI Agents
In `@key-wallet-manager/Cargo.toml` at 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.
There was a problem hiding this comment.
I think we already talk about this crate not supporting no-std builds but would like you to close this rabbit conversation to confirm
There was a problem hiding this comment.
🧩 Analysis chain
Let me check whether key-wallet-manager actually supports no_std builds:
🏁 Script executed:
#!/bin/bash
# Check if key-wallet-manager has no_std support in lib.rs or main module
fd -t f "lib.rs" key-wallet-manager/src/ --exec cat {}
Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Search for any no_std or cfg_attr patterns in key-wallet-manager
rg -n "no_std|cfg_attr" key-wallet-manager/src/
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Check if there are previous discussions or comments about no_std in this repo
gh pr list --repo dashpay/rust-dashcore --search "no_std key-wallet-manager" --json number,title,url --limit 10
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Search for previous discussions about no_std support in key-wallet-manager
gh pr list --repo dashpay/rust-dashcore --state all --search "key-wallet-manager" --json number,title,url --limit 20
Length of output: 2633
🏁 Script executed:
#!/bin/bash
# Search for comments mentioning no_std in recent PRs
gh search issues --repo dashpay/rust-dashcore "no_std OR no-std" --json number,title,url --limit 10
Length of output: 2
🏁 Script executed:
#!/bin/bash
# Check git history for changes related to no_std in key-wallet-manager
git log --all --oneline --grep="no_std\|no-std" -- key-wallet-manager/ | head -20
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find where rayon is actually used in key-wallet-manager
rg -n "rayon|par_iter|ParallelIterator" key-wallet-manager/src/
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Check the matching.rs file specifically since that's where parallel processing is added
cat key-wallet-manager/src/wallet_manager/matching.rs
Length of output: 5928
@ZocoLini, I see the discrepancy here. The code currently declares #![cfg_attr(not(feature = "std"), no_std)] in key-wallet-manager/src/lib.rs at line 17, which indicates the crate is designed to support no_std builds (even though std is in the default features).
However, you're saying the crate doesn't need to support no_std builds. If that's the case, there are two options:
-
Remove no_std support entirely: Remove the
#![cfg_attr(not(feature = "std"), no_std)]attribute from lib.rs, making it clear this is a std-only crate. Then the unconditionalrayondependency is fine. -
Keep no_std support: Gate
rayonbehind thestdfeature (as CodeRabbit suggested) so the crate can still be built without std.
Which approach would you prefer? If you confirm that key-wallet-manager should be std-only, I can dismiss this CodeRabbit comment as the concern wouldn't apply.
🧠 Learnings used
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T14:53:56.708Z
Learning: Applies to **/key-wallet/**/*.rs : Use secure random number generation for keys
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Never serialize or log private keys in production; use public keys or key fingerprints for identification instead
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/bip32/**/*.rs : Cache intermediate key derivation results and batch derive child keys when possible to optimize derivation performance
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) to enable both full wallets and watch-only wallets with the same interface
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/account/**/*.rs : Use enum-based type system for `AccountType` with specific variants (Standard, IdentityAuthentication, IdentityEncryption, MasternodeOperator, etc.) to provide compile-time safety and clear semantics
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.838Z
Learning: Applies to dash-spv/**/Cargo.toml : Maintain Minimum Rust Version (MSRV) of 1.89 and ensure all dependencies compile with `cargo check --all-features`
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.838Z
Learning: Applies to dash-spv/**/*.rs : Run `cargo clippy --all-targets --all-features -- -D warnings` to catch potential bugs and style issues
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Use staged gap limit management with `GapLimitStage` tracking `last_used_index` and `used_indices` to enable efficient address discovery without loading entire chains into memory
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
| let block_2 = Block::dummy(100, vec![tx_2]); | ||
| let filter_2 = BlockFilter::dummy(&block_2); | ||
| let key_2 = FilterMatchKey::new(200, block_2.block_hash()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the file
find . -type f -name "matching.rs" -path "*wallet_manager*"Repository: dashpay/rust-dashcore
Length of output: 116
🏁 Script executed:
# Read the file to see the relevant lines
cat -n ./key-wallet-manager/src/wallet_manager/matching.rs | sed -n '115,135p'Repository: dashpay/rust-dashcore
Length of output: 1126
🏁 Script executed:
# Let's see the full test function to understand its purpose and logic
cat -n ./key-wallet-manager/src/wallet_manager/matching.rs | sed -n '100,160p'Repository: dashpay/rust-dashcore
Length of output: 2970
🏁 Script executed:
# Let's understand FilterMatchKey structure and how height is used
rg -A 10 "struct FilterMatchKey" --type rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Let's find FilterMatchKey definition
rg -A 10 "struct FilterMatchKey" --type rustRepository: dashpay/rust-dashcore
Length of output: 847
🏁 Script executed:
# Let's look at the check_compact_filters_for_addresses function to understand how height is used
rg -B 5 -A 20 "fn check_compact_filters_for_addresses" --type rustRepository: dashpay/rust-dashcore
Length of output: 2047
🏁 Script executed:
# Let's see if there's sorting logic that uses height
rg -B 5 -A 15 "test_output_sorted_by_height" --type rustRepository: dashpay/rust-dashcore
Length of output: 1931
🏁 Script executed:
# Continue reading test_output_sorted_by_height to see how it validates the sorting
cat -n ./key-wallet-manager/src/wallet_manager/matching.rs | sed -n '144,170p'Repository: dashpay/rust-dashcore
Length of output: 1317
🏁 Script executed:
# Let's check FilterMatchOutput type and if it's sorted
rg -B 5 -A 10 "type FilterMatchOutput" --type rustRepository: dashpay/rust-dashcore
Length of output: 1375
🏁 Script executed:
# Let's check the Ord derivation to understand the sorting behavior
rg -B 10 "struct FilterMatchKey" --type rust | head -20Repository: dashpay/rust-dashcore
Length of output: 1043
Keep block heights consistent with FilterMatchKey metadata
block_2/block_3 are created at height 100 while their keys use heights 200/300. Aligning these makes test data realistic and improves clarity, especially since FilterMatchOutput is sorted by height.
Suggested fix
- let block_2 = Block::dummy(100, vec![tx_2]);
+ let block_2 = Block::dummy(200, vec![tx_2]);
let filter_2 = BlockFilter::dummy(&block_2);
let key_2 = FilterMatchKey::new(200, block_2.block_hash());
- let block_3 = Block::dummy(100, vec![tx_3]);
+ let block_3 = Block::dummy(300, vec![tx_3]);
let filter_3 = BlockFilter::dummy(&block_3);
let key_3 = FilterMatchKey::new(300, block_3.block_hash());🤖 Prompt for AI Agents
In `@key-wallet-manager/src/wallet_manager/matching.rs` around lines 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.
| let mut manager = WalletManager::<ManagedWalletInfo>::new(Network::Testnet); | ||
|
|
||
| let _wallet_id = manager | ||
| .create_wallet_with_random_mnemonic(WalletAccountCreationOptions::Default) | ||
| .expect("Failed to create wallet"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "check_compact_filters_tests.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 122
🏁 Script executed:
git ls-files | grep -E "(check_compact_filters|tests)" | head -20Repository: dashpay/rust-dashcore
Length of output: 923
🏁 Script executed:
fd "check_compact_filters" --type fRepository: dashpay/rust-dashcore
Length of output: 120
🏁 Script executed:
cat -n ./key-wallet-manager/tests/check_compact_filters_tests.rsRepository: dashpay/rust-dashcore
Length of output: 6891
🏁 Script executed:
wc -l ./key-wallet-manager/tests/check_compact_filters_tests.rsRepository: dashpay/rust-dashcore
Length of output: 126
🏁 Script executed:
rg "create_wallet_with_random_mnemonic|create_wallet" --type rs -A 3 key-wallet-manager | head -80Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
fd "wallet_manager" --type f | xargs grep -l "create_wallet" | head -5Repository: dashpay/rust-dashcore
Length of output: 243
🏁 Script executed:
rg "impl.*WalletManager" --type rs -A 20 | head -100Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
rg "create_wallet_with_random_mnemonic" --type-list | head -5Repository: dashpay/rust-dashcore
Length of output: 454
🏁 Script executed:
rg "create_wallet_with_random_mnemonic" -A 5 | head -60Repository: dashpay/rust-dashcore
Length of output: 5236
🏁 Script executed:
find key-wallet-manager -name "*.rs" | xargs grep -l "create_wallet"Repository: dashpay/rust-dashcore
Length of output: 359
🏁 Script executed:
cat -n key-wallet-manager/src/wallet_manager/mod.rs | head -100Repository: dashpay/rust-dashcore
Length of output: 4472
🏁 Script executed:
grep -n "create_wallet" key-wallet-manager/src/wallet_manager/mod.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 226
🏁 Script executed:
grep -n "fn create_wallet" key-wallet-manager/src/wallet_manager/mod.rsRepository: dashpay/rust-dashcore
Length of output: 226
🏁 Script executed:
sed -n '250,280p' key-wallet-manager/src/wallet_manager/mod.rsRepository: dashpay/rust-dashcore
Length of output: 1364
🏁 Script executed:
sed -n '92,165p' key-wallet-manager/src/wallet_manager/mod.rsRepository: dashpay/rust-dashcore
Length of output: 3663
🏁 Script executed:
grep -A 5 "create_wallet_from_mnemonic" key-wallet-manager/src/wallet_manager/mod.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 587
🏁 Script executed:
rg "fixed.*mnemonic|test.*vector|deterministic" key-wallet --no-heading | head -20Repository: dashpay/rust-dashcore
Length of output: 1987
🏁 Script executed:
find key-wallet -name "*.rs" | xargs grep -l "test_vectors\|FIXED_SEED\|known.*mnemonic" 2>/dev/nullRepository: dashpay/rust-dashcore
Length of output: 192
🏁 Script executed:
rg "Address::dummy" -B 2 -A 2 key-wallet-manager/tests/check_compact_filters_tests.rsRepository: dashpay/rust-dashcore
Length of output: 578
🏁 Script executed:
rg "test_vector|MNEMONIC|\"abandon" key-wallet/tests --no-heading -A 1 | head -40Repository: dashpay/rust-dashcore
Length of output: 3041
🏁 Script executed:
grep -r "abandon abandon" . --include="*.rs" | head -5Repository: dashpay/rust-dashcore
Length of output: 985
🏁 Script executed:
find key-wallet -name "*test*" -type f | xargs grep -l "Mnemonic\|mnemonic" 2>/dev/nullRepository: dashpay/rust-dashcore
Length of output: 624
🏁 Script executed:
rg "create_wallet_from_mnemonic" key-wallet-manager/tests --no-heading -B 2 -A 2 | head -40Repository: dashpay/rust-dashcore
Length of output: 3313
🏁 Script executed:
cat -n key-wallet-manager/tests/integration_test.rs | head -80Repository: dashpay/rust-dashcore
Length of output: 3744
🏁 Script executed:
grep -n "Network::" key-wallet-manager/tests/check_compact_filters_tests.rsRepository: dashpay/rust-dashcore
Length of output: 675
🏁 Script executed:
grep -n "Mnemonic::generate" key-wallet-manager/src/wallet_manager/mod.rsRepository: dashpay/rust-dashcore
Length of output: 158
🏁 Script executed:
rg "create_wallet_from_mnemonic.*abandon" --no-heading | head -5Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
grep -A 10 "fn test_check_compact_filters" key-wallet-manager/tests/check_compact_filters_tests.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 1269
🏁 Script executed:
cat -n key-wallet-manager/tests/test_serialized_wallets.rs | grep -A 3 "test_mnemonic"Repository: dashpay/rust-dashcore
Length of output: 1376
🏁 Script executed:
grep -B 5 "test_mnemonic.*=" key-wallet-manager/tests/test_serialized_wallets.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 608
🏁 Script executed:
grep -n "const.*MNEMONIC\|static.*MNEMONIC" key-wallet-manager --include="*.rs" -rRepository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
cat key-wallet-manager/tests/test_serialized_wallets.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 1279
🏁 Script executed:
# Quick verification of the network concern
rg "Address::dummy.*Testnet" key-wallet-manager/tests/check_compact_filters_tests.rsRepository: dashpay/rust-dashcore
Length of output: 47
Use a fixed test mnemonic for reproducible test results
All five test functions create wallets with create_wallet_with_random_mnemonic, generating a different random mnemonic each run. For deterministic, reproducible tests, use create_wallet_from_mnemonic with a fixed test vector (e.g., "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about") or a deterministic helper with a known seed. This ensures stable test behavior and enables reliable CI/CD execution.
Applies to lines 13-17, 29-31, 60-62, 96-98, 122-124
🤖 Prompt for AI Agents
In `@key-wallet-manager/tests/check_compact_filters_tests.rs` around lines 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.
| let unrelated_1 = Address::dummy(Network::Regtest, 100); | ||
| let unrelated_2 = Address::dummy(Network::Regtest, 101); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n key-wallet-manager/tests/check_compact_filters_tests.rs | head -100Repository: dashpay/rust-dashcore
Length of output: 4705
Align dummy addresses with the Testnet wallet network
The wallet is initialized with Network::Testnet, but these tests construct Regtest dummy addresses. According to established guidelines, network consistency must be validated when deriving or validating addresses; mixing networks should be avoided.
♻️ Suggested adjustment
- let unrelated_1 = Address::dummy(Network::Regtest, 100);
- let unrelated_2 = Address::dummy(Network::Regtest, 101);
+ let unrelated_1 = Address::dummy(Network::Testnet, 100);
+ let unrelated_2 = Address::dummy(Network::Testnet, 101);- let unrelated = Address::dummy(Network::Regtest, 999);
+ let unrelated = Address::dummy(Network::Testnet, 999);Also applies to: 65-65, 75-75
🤖 Prompt for AI Agents
In `@key-wallet-manager/tests/check_compact_filters_tests.rs` around lines 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.
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-trait = "0.1" | ||
| bincode = { version = "=2.0.0-rc.3", optional = true } | ||
| zeroize = { version = "1.8", features = ["derive"] } | ||
| rayon = "1.11" |
There was a problem hiding this comment.
I think we already talk about this crate not supporting no-std builds but would like you to close this rabbit conversation to confirm
|
|
||
| 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
| } | ||
|
|
||
| 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??
| pub type FilterMatchInput = HashMap<FilterMatchKey, BlockFilter>; | ||
| pub type FilterMatchOutput = BTreeSet<FilterMatchKey>; | ||
|
|
||
| #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] |
There was a problem hiding this comment.
I don't like this types definitions, they don't provide any benefit while they hide the real type. By taking a look at the method we know that the parameter is an input and the result is an output without having to explicitly say it
| 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
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
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.