fix(key-wallet): correct CoinJoin discovery

[xdustinface]

Three fixes:

• Derivation path & denominations: derive at m/9'/coin'/4'/account'/0/index (was missing 4' and /0) and use the protocol denominations including the per-round fee, so filters match and rounds aren't misclassified as Standard.
• Ownership routing: check all fund-bearing accounts for every tx instead of gating by transaction shape, matching Dash Core. CoinJoin classification is now just a label, never a precondition for discovery.

Summary by CodeRabbit

• New Features

  • Added CLI option --coinjoin-gap-limit to configure the number of CoinJoin addresses pre-generated (default: 30)

• Improvements

  • Updated CoinJoin address derivation paths with additional components
  • Enhanced transaction routing to check multiple account types for CoinJoin transactions
  • Adjusted CoinJoin denomination values

[coderabbitai]

📝 Walkthrough

Walkthrough

This 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 --coinjoin-gap-limit option, threads that parameter through wallet manager and account constructors, updates CoinJoin derivation paths to use DIP-9 feature-purpose components, and refactors transaction routing to check all fund-bearing accounts uniformly. Denomination constants are updated to include fee components.

Changes

CoinJoin Address Pool Gap Limit Configuration

Layer / File(s)	Summary
CLI configuration and SPV client setup dash-spv/src/client/config.rs, dash-spv/src/main.rs	New --coinjoin-gap-limit CLI option (default 30) is added to the Args struct and threaded into ClientConfig via a new coinjoin_gap_limit field and with_coinjoin_gap_limit builder method.
Wallet manager parameter extension key-wallet-manager/src/lib.rs, key-wallet-ffi/src/wallet_manager.rs, key-wallet-manager/examples/wallet_creation.rs, key-wallet-manager/src/test_helpers.rs, key-wallet-manager/src/process_block.rs, key-wallet-manager/tests/integration_test.rs, dash-spv/src/test_utils/wallet.rs, dash-spv/tests/dashd_sync/tests_multi_wallet.rs	WalletManager::create_wallet_from_mnemonic signature extended with coinjoin_gap_limit: Option<u32> parameter. All call sites updated to pass the parameter (None in tests and helpers, Some in SPV client).
CoinJoin account derivation path update key-wallet/src/account/account_type.rs	CoinJoin account derivation path changed from 9'/coin_type'/account' to 9'/1'/4'/account' using DIP-9 feature-purpose hardened components.
Managed wallet and account collection infrastructure key-wallet/src/wallet/managed_wallet_info/mod.rs, key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet/src/managed_account/managed_account_collection.rs	New from_wallet_with_coinjoin_gap_limit trait method and implementations enable optional CoinJoin gap limit override at wallet creation time. New from_account_collection_with_coinjoin_gap_limit threads the parameter through account collection and managed account type initialization.
Managed account type and core keys constructors key-wallet/src/managed_account/managed_account_type.rs, key-wallet/src/managed_account/managed_core_keys_account.rs	ManagedAccountType::from_account_type updated to accept coinjoin_gap_limit and configure CoinJoin pools as External type with configurable gap. Core keys account constructors updated to pass None for the parameter.
Transaction routing semantic change and denomination constants key-wallet/src/transaction_checking/transaction_router/mod.rs	Transaction routing refactored to use membership-based account discovery: both Standard and CoinJoin transactions now check all fund-bearing account types via shared helper. CoinJoin denomination constants updated to include per-round fee components. Documentation expanded to clarify the heuristic only affects transaction labeling.
Comprehensive test suite updates key-wallet/src/tests/account_tests.rs, key-wallet/src/transaction_checking/transaction_router/tests/classification.rs, key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs, key-wallet/src/transaction_checking/transaction_router/tests/routing.rs, key-wallet/src/transaction_checking/wallet_checker.rs	Test fixtures updated with adjusted CoinJoin denomination values (fee-inclusive). Assertions expanded to verify membership-based account discovery. Derivation path assertions updated to match DIP-9 four-segment structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

ready-for-review

Suggested reviewers

• ZocoLini

Poem

🐰 A hop through the gap limit's delight,
CoinJoin addresses now shine so bright!
DIP-9 paths take their proper place,
While transactions check all accounts with grace—
Configuration flows from CLI to deep,
A feature so grand for wallets to keep!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The PR title focuses on CoinJoin discovery routing and ownership, which is covered in the PR but represents only one of three major objectives; it omits the derivation path fix and configurable gap limit changes.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/coin-join-gap-limit

---

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 and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.54%. Comparing base (a132945) to head (8628d30).

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 

Flag	Coverage Δ
core	76.54% <ø> (ø)
ffi	45.43% <ø> (-0.99%)	⬇️
rpc	20.00% <ø> (ø)
spv	90.27% <ø> (-0.04%)	⬇️
wallet	71.31% <100.00%> (+0.02%)	⬆️

Files with missing lines	Coverage Δ
key-wallet/src/account/account_type.rs	45.19% <100.00%> (+0.80%)	⬆️
.../src/managed_account/managed_account_collection.rs	58.49% <100.00%> (+0.10%)	⬆️
...wallet/src/managed_account/managed_account_type.rs	39.67% <100.00%> (+3.65%)	⬆️
...src/transaction_checking/transaction_router/mod.rs	89.06% <100.00%> (+0.26%)	⬆️
...-wallet/src/transaction_checking/wallet_checker.rs	99.21% <100.00%> (+<0.01%)	⬆️

... and 18 files with indirect coverage changes

[coderabbitai]

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 win

PR 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 win

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

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);+ }
}

As per coding guidelines, "`**/*.rs`: Write unit tests for new functionality".

🤖 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 win

Constrain coinjoin_gap_limit before propagating it into wallet initialization.

u32 currently accepts 0 and 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 a feat(...) 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

📥 Commits

Reviewing files that changed from the base of the PR and between a132945 and 3df81f0.

📒 Files selected for processing (22)

• dash-spv/src/client/config.rs
• dash-spv/src/main.rs
• dash-spv/src/test_utils/wallet.rs
• dash-spv/tests/dashd_sync/tests_multi_wallet.rs
• key-wallet-ffi/src/wallet_manager.rs
• key-wallet-manager/examples/wallet_creation.rs
• key-wallet-manager/src/lib.rs
• key-wallet-manager/src/process_block.rs
• key-wallet-manager/src/test_helpers.rs
• key-wallet-manager/tests/integration_test.rs
• key-wallet/src/account/account_type.rs
• key-wallet/src/managed_account/managed_account_collection.rs
• key-wallet/src/managed_account/managed_account_type.rs
• key-wallet/src/managed_account/managed_core_keys_account.rs
• key-wallet/src/tests/account_tests.rs
• key-wallet/src/transaction_checking/transaction_router/mod.rs
• key-wallet/src/transaction_checking/transaction_router/tests/classification.rs
• key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs
• key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
• key-wallet/src/transaction_checking/wallet_checker.rs
• key-wallet/src/wallet/managed_wallet_info/mod.rs
• key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs