fix(key-wallet): correct CoinJoin discovery#804
Open
xdustinface wants to merge 2 commits into
Open
Conversation
CoinJoin addresses were derived at `m/9'/coin'/account'/index` instead of Dash Core's `m/9'/coin'/4'/account'/0/index`, so the SPV client monitored the wrong scriptPubKeys and matched none of the wallet's CoinJoin outputs against BIP158 filters. Insert the hardened `4'` feature level and derive the pool on the `/0` branch (via the external pool) to match Dash Core. The CoinJoin denomination constants in `has_denomination_outputs` omitted the per-round fee (`100_000_000` etc. instead of `100_001_000` etc.), so real mixing rounds were classified `Standard`. Use the protocol denominations from `coinjoin/common.h`.
Contributor
📝 WalkthroughWalkthroughThis pull request adds end-to-end support for configurable CoinJoin address pool gap limits in the Dash wallet system. It extends the CLI with a new ChangesCoinJoin Address Pool Gap Limit Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
changed the title
…nJoin classification to a label `get_relevant_account_types` routed `TransactionType::Standard` and `TransactionType::CoinJoin` to disjoint account sets, so a transaction's shape gated which fund-bearing accounts were checked for ownership. A misclassified tx (for example a small denomination spend that the `>= 3 in/out` heuristic labels `Standard`, or a CoinJoin-shaped tx touching standard collateral and change) therefore skipped the account that actually owned its coins and was dropped. Match Dash Core, where ownership is membership-based and never depends on a transaction's type or shape: `AddToWalletIfInvolvingMe` records a tx iff `IsMine || IsFromMe`, and `IsMine` tests each scriptPubKey against all script-pubkey managers uniformly (regular external, internal, and the CoinJoin descriptor). Denomination/CoinJoin classification is purely a downstream annotation, never a precondition for discovery. Both the `Standard` and `CoinJoin` arms now return the full set of fund-bearing account types via a shared `fund_bearing_account_types` helper (StandardBIP44, StandardBIP32, CoinJoin, DashpayReceivingFunds, DashpayExternalAccount). The `is_coinjoin_transaction` heuristic is unchanged: it now only sets the stored `TransactionType` label, never which accounts are consulted. Checking extra accounts introduces no false positives because an account matches only when a scriptPubKey or spent UTXO genuinely belongs to it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #804 +/- ##
==========================================
- Coverage 72.67% 72.54% -0.14%
==========================================
Files 322 322
Lines 71363 71374 +11
==========================================
- Hits 51866 51781 -85
- Misses 19497 19593 +96
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet/src/tests/account_tests.rs (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPR title prefix likely under-describes the scope (
fix+ new feature).This PR includes new functionality (configurable CoinJoin gap limit), so the
fix(...)title may be misleading for semantic history/changelog. Consider retitling (e.g.,feat(...)) or splitting fix and feature concerns into separate PRs.As per coding guidelines, "Check whether the PR title prefix ... accurately describes the changes ... If a PR mixes unrelated concerns ... suggest splitting it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/tests/account_tests.rs` at line 1, The PR title currently uses the "fix(...)" prefix but introduces a new feature (configurable CoinJoin gap limit); update the PR title to use a feature prefix (e.g., "feat(...)") or split the changes into two PRs (one for the bugfixes and one for the new configurable CoinJoin gap limit) and update the corresponding commit messages/PR description accordingly so the semantic history reflects the new functionality.dash-spv/src/main.rs (1)
428-552:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd focused unit tests for CoinJoin gap-limit CLI/config plumbing.
Please add assertions for both default and explicit override so regressions in argument/config wiring are caught.
As per coding guidelines, "`**/*.rs`: Write unit tests for new functionality".Suggested tests
#[test] fn build_client_config_returns_devnet_on_devnet() { let args = args(&["--network", "devnet", "--devnet-name", "alpha"]); let tmp = TempDir::new().unwrap(); let config = build_client_config(&args, tmp.path().to_path_buf()).expect("ok"); let devnet = config.devnet.as_ref().expect("devnet must be set"); assert_eq!(devnet.name, "alpha"); assert_eq!(config.network, Network::Devnet); } + + #[test] + fn coinjoin_gap_limit_defaults_to_30() { + let args = args(&[]); + let tmp = TempDir::new().unwrap(); + let config = build_client_config(&args, tmp.path().to_path_buf()).expect("ok"); + assert_eq!(config.coinjoin_gap_limit, 30); + } + + #[test] + fn coinjoin_gap_limit_override_is_applied() { + let args = args(&["--coinjoin-gap-limit", "64"]); + let tmp = TempDir::new().unwrap(); + let config = build_client_config(&args, tmp.path().to_path_buf()).expect("ok"); + assert_eq!(config.coinjoin_gap_limit, 64); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/main.rs` around lines 428 - 552, Add two unit tests in the existing tests module that verify CoinJoin gap-limit CLI → config wiring: one that parses no --coinjoin-gap-limit flag and asserts the built Args/config (use parse(), args(), and build_client_config()) yields the expected default gap limit, and another that passes "--coinjoin-gap-limit" with an explicit numeric value and asserts that the resulting Args/config contains that override. Locate the CLI flag handling in the Args parsing code and the client config field (e.g., Args::coinjoin_gap_limit or ClientConfig.coinjoin_gap_limit / config.coinjoin.gap_limit) and assert against those symbols so both default and explicit override regressions are caught. Ensure tests mirror existing style (TempDir usage and expect/expect_err patterns) and add clear assertion messages.
🧹 Nitpick comments (2)
dash-spv/src/main.rs (1)
142-144: ⚡ Quick winConstrain
coinjoin_gap_limitbefore propagating it into wallet initialization.
u32currently accepts0and extreme values, which can cause empty address pools or expensive pre-derivation work. Add explicit bounds validation in config building and fail fast on invalid inputs.Suggested patch
+const MIN_COINJOIN_GAP_LIMIT: u32 = 1; +const MAX_COINJOIN_GAP_LIMIT: u32 = 10_000; + fn build_client_config(args: &Args, data_dir: PathBuf) -> Result<ClientConfig, String> { let network: Network = args.network.into(); let validation_mode: ValidationMode = args.validation_mode.into(); + + if !(MIN_COINJOIN_GAP_LIMIT..=MAX_COINJOIN_GAP_LIMIT).contains(&args.coinjoin_gap_limit) { + return Err(format!( + "--coinjoin-gap-limit must be in range {}..={}, got {}", + MIN_COINJOIN_GAP_LIMIT, MAX_COINJOIN_GAP_LIMIT, args.coinjoin_gap_limit + )); + } let mut config = ClientConfig::new(network) .with_storage_path(data_dir) .with_validation_mode(validation_mode) .with_coinjoin_gap_limit(args.coinjoin_gap_limit);Also applies to: 287-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/main.rs` around lines 142 - 144, Validate coinjoin_gap_limit when building the config: ensure the coinjoin_gap_limit field (u32) is clamped/checked to a safe non-zero upper bound (e.g., reject 0 and excessively large values) and return an explicit error before wallet initialization; update the config builder/parsing code that sets coinjoin_gap_limit to perform this bounds check and propagate only the validated value to wallet initialization (e.g., where wallet/CoinJoin account is created such as in the wallet init function), and apply the same validation to the other occurrence of coinjoin_gap_limit usage referenced in the diff so invalid values fail fast.dash-spv/src/client/config.rs (1)
79-81: Consider aligning PR title/scope with mixed fix + feature content.This PR introduces a new user-facing capability (
coinjoin_gap_limit) in addition to bug fixes; consider afeat(...)prefix or splitting fix/feature changes for clearer semantic intent.As per coding guidelines: "Check whether the PR title prefix ... accurately describes the changes ... If a PR mixes unrelated concerns ... suggest splitting it into separate focused PRs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/client/config.rs` around lines 79 - 81, The PR mixes a new user-facing feature (the config field coinjoin_gap_limit in config.rs) with unrelated fixes; either rename the PR/commit titles to use a feature prefix (e.g., feat(config): add coinjoin_gap_limit) to reflect the new capability, or split the change into two focused PRs—one containing the coinjoin_gap_limit addition (and its docs/tests) and a separate PR for the bug fixes—so reviewers can clearly see feature vs fix scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/client/config.rs`:
- Around line 202-206: The with_coinjoin_gap_limit setter allows 0 but the
config.validate() does not reject it, which can break CoinJoin discovery; update
the validate() method to check that self.coinjoin_gap_limit > 0 and return a
validation error if it's 0 (include a clear message referencing
"coinjoin_gap_limit"), and apply the same non-zero validation for the other
gap-limit setters/fields referenced in the same region (e.g., any
receive/change/other gap limit fields defined around lines 209-245) so all gap
limits are validated as > 0 before accepting the config.
In `@dash-spv/src/main.rs`:
- Around line 142-144: The PR appears to add new CLI functionality (the
coinjoin_gap_limit CLI arg defined via the #[arg(...)] field
coinjoin_gap_limit), so update the PR title prefix from "fix(...)" to a more
accurate prefix such as "feat(cli):" (or split the fix vs feature changes into
separate PRs/commits) and ensure the title succinctly describes adding the
coinjoin_gap_limit CLI option; this aligns the PR prefix with the change
introducing the coinjoin_gap_limit argument and CLI behavior.
---
Outside diff comments:
In `@dash-spv/src/main.rs`:
- Around line 428-552: Add two unit tests in the existing tests module that
verify CoinJoin gap-limit CLI → config wiring: one that parses no
--coinjoin-gap-limit flag and asserts the built Args/config (use parse(),
args(), and build_client_config()) yields the expected default gap limit, and
another that passes "--coinjoin-gap-limit" with an explicit numeric value and
asserts that the resulting Args/config contains that override. Locate the CLI
flag handling in the Args parsing code and the client config field (e.g.,
Args::coinjoin_gap_limit or ClientConfig.coinjoin_gap_limit /
config.coinjoin.gap_limit) and assert against those symbols so both default and
explicit override regressions are caught. Ensure tests mirror existing style
(TempDir usage and expect/expect_err patterns) and add clear assertion messages.
In `@key-wallet/src/tests/account_tests.rs`:
- Line 1: The PR title currently uses the "fix(...)" prefix but introduces a new
feature (configurable CoinJoin gap limit); update the PR title to use a feature
prefix (e.g., "feat(...)") or split the changes into two PRs (one for the
bugfixes and one for the new configurable CoinJoin gap limit) and update the
corresponding commit messages/PR description accordingly so the semantic history
reflects the new functionality.
---
Nitpick comments:
In `@dash-spv/src/client/config.rs`:
- Around line 79-81: The PR mixes a new user-facing feature (the config field
coinjoin_gap_limit in config.rs) with unrelated fixes; either rename the
PR/commit titles to use a feature prefix (e.g., feat(config): add
coinjoin_gap_limit) to reflect the new capability, or split the change into two
focused PRs—one containing the coinjoin_gap_limit addition (and its docs/tests)
and a separate PR for the bug fixes—so reviewers can clearly see feature vs fix
scope.
In `@dash-spv/src/main.rs`:
- Around line 142-144: Validate coinjoin_gap_limit when building the config:
ensure the coinjoin_gap_limit field (u32) is clamped/checked to a safe non-zero
upper bound (e.g., reject 0 and excessively large values) and return an explicit
error before wallet initialization; update the config builder/parsing code that
sets coinjoin_gap_limit to perform this bounds check and propagate only the
validated value to wallet initialization (e.g., where wallet/CoinJoin account is
created such as in the wallet init function), and apply the same validation to
the other occurrence of coinjoin_gap_limit usage referenced in the diff so
invalid values fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c742a1b2-4705-4ace-b3f4-27c52bb830a9
📒 Files selected for processing (22)
dash-spv/src/client/config.rsdash-spv/src/main.rsdash-spv/src/test_utils/wallet.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/account/account_type.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rskey-wallet/src/managed_account/managed_core_keys_account.rskey-wallet/src/tests/account_tests.rskey-wallet/src/transaction_checking/transaction_router/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/classification.rskey-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
xdustinface
force-pushed
the
feat/coin-join-gap-limit
branch
from
3df81f0 to
8628d30
Compare
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
1 participant
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.
Three fixes:
m/9'/coin'/4'/account'/0/index(was missing4'and/0) and use the protocol denominations including the per-round fee, so filters match and rounds aren't misclassified asStandard.Summary by CodeRabbit
New Features
--coinjoin-gap-limitto configure the number of CoinJoin addresses pre-generated (default: 30)Improvements