diff --git a/key-wallet/src/account/account_type.rs b/key-wallet/src/account/account_type.rs index 7c56faec8..f5ce4961c 100644 --- a/key-wallet/src/account/account_type.rs +++ b/key-wallet/src/account/account_type.rs @@ -344,11 +344,14 @@ impl AccountType { Self::CoinJoin { index, } => { - // m/9'/coin_type'/account' + // m/9'/coin_type'/4'/account' Ok(DerivationPath::from(vec![ - ChildNumber::from_hardened_idx(9).map_err(crate::error::Error::Bip32)?, + ChildNumber::from_hardened_idx(crate::dip9::FEATURE_PURPOSE) + .map_err(crate::error::Error::Bip32)?, ChildNumber::from_hardened_idx(coin_type) .map_err(crate::error::Error::Bip32)?, + ChildNumber::from_hardened_idx(crate::dip9::FEATURE_PURPOSE_COINJOIN) + .map_err(crate::error::Error::Bip32)?, ChildNumber::from_hardened_idx(*index).map_err(crate::error::Error::Bip32)?, ])) } diff --git a/key-wallet/src/managed_account/managed_account_collection.rs b/key-wallet/src/managed_account/managed_account_collection.rs index 0739b6fb1..8eaaac6e7 100644 --- a/key-wallet/src/managed_account/managed_account_collection.rs +++ b/key-wallet/src/managed_account/managed_account_collection.rs @@ -585,9 +585,13 @@ impl ManagedAccountCollection { AccountType::CoinJoin { index, } => { + // CoinJoin addresses live on the external branch (m/9'/coin'/4'/account'/0/index) + // to match Dash Core, so derive through the `External` pool which uses [0, index]. + let mut coinjoin_path = base_path; + coinjoin_path.push(crate::bip32::ChildNumber::from_normal_idx(0)?); let addresses = AddressPool::new( - base_path, - AddressPoolType::Absent, + coinjoin_path, + AddressPoolType::External, DEFAULT_COINJOIN_GAP_LIMIT, network, key_source, diff --git a/key-wallet/src/managed_account/managed_account_type.rs b/key-wallet/src/managed_account/managed_account_type.rs index d89e12fd2..d0f0492a4 100644 --- a/key-wallet/src/managed_account/managed_account_type.rs +++ b/key-wallet/src/managed_account/managed_account_type.rs @@ -529,12 +529,15 @@ impl ManagedAccountType { AccountType::CoinJoin { index, } => { - let path = account_type + // CoinJoin addresses live on the external branch (m/9'/coin'/4'/account'/0/index) + // to match Dash Core, so derive through the `External` pool which uses [0, index]. + let mut path = account_type .derivation_path(network) .unwrap_or_else(|_| DerivationPath::master()); + path.push(crate::bip32::ChildNumber::from_normal_idx(0)?); let pool = AddressPool::new( path, - AddressPoolType::Absent, + AddressPoolType::External, DEFAULT_COINJOIN_GAP_LIMIT, network, key_source, diff --git a/key-wallet/src/tests/account_tests.rs b/key-wallet/src/tests/account_tests.rs index de0a3f51b..d21b238bf 100644 --- a/key-wallet/src/tests/account_tests.rs +++ b/key-wallet/src/tests/account_tests.rs @@ -4,6 +4,8 @@ use crate::account::{Account, AccountType, StandardAccountType}; use crate::bip32::{ExtendedPrivKey, ExtendedPubKey}; +use crate::managed_account::address_pool::KeySource; +use crate::managed_account::managed_account_type::ManagedAccountType; use crate::mnemonic::{Language, Mnemonic}; use crate::Network; use secp256k1::Secp256k1; @@ -128,8 +130,28 @@ fn test_coinjoin_account_creation() { _ => panic!("Expected CoinJoin account type"), } - // Verify derivation path for CoinJoin: m/9'/1'/index' (testnet coin type) - assert_eq!(derivation_path.to_string(), format!("m/9'/1'/{}'", index)); + // Verify the CoinJoin account derivation path matches Dash Core: + // m/9'/1'/4'/account' (testnet coin type, FEATURE_PURPOSE_COINJOIN = 4'). + assert_eq!(derivation_path.to_string(), format!("m/9'/1'/4'/{}'", index)); + + // The managed CoinJoin pool must derive addresses on the external (`/0`) + // branch, so the first address sits at m/9'/1'/4'/account'/0/0. + let key_source = KeySource::Public(account.account_xpub); + let managed_type = + ManagedAccountType::from_account_type(account.account_type, network, &key_source) + .unwrap(); + let pool = match &managed_type { + ManagedAccountType::CoinJoin { + addresses, + .. + } => addresses, + _ => panic!("Expected CoinJoin managed account type"), + }; + let first_address = + pool.address_at_index(0).expect("CoinJoin pool should pre-generate address 0"); + let first_info = + pool.address_info(&first_address).expect("address info for index 0 should exist"); + assert_eq!(first_info.path.to_string(), format!("m/9'/1'/4'/{}'/0/0", index)); } } @@ -498,7 +520,7 @@ fn test_account_derivation_path_uniqueness() { AccountType::CoinJoin { index: 0, }, - "m/9'/1'/0'".to_string(), + "m/9'/1'/4'/0'".to_string(), ), (AccountType::IdentityRegistration, "m/9'/1'/5'/1'".to_string()), ( diff --git a/key-wallet/src/transaction_checking/transaction_router/mod.rs b/key-wallet/src/transaction_checking/transaction_router/mod.rs index 93bd98b13..b7d74b290 100644 --- a/key-wallet/src/transaction_checking/transaction_router/mod.rs +++ b/key-wallet/src/transaction_checking/transaction_router/mod.rs @@ -79,18 +79,33 @@ impl TransactionRouter { } } + /// All account types that hold spendable funds on the Core chain. + /// + /// Ownership of a transaction is membership-based across every keychain, exactly like + /// Dash Core's `IsMine`, which tests each scriptPubKey against all script-pubkey managers + /// uniformly (regular external, internal, and the CoinJoin descriptor). A transaction's + /// shape (`TransactionType::Standard` vs `TransactionType::CoinJoin`) is only a downstream + /// label, never a precondition for discovery, so both shapes must consult the full set of + /// fund-bearing accounts. An account only matches when a scriptPubKey or spent UTXO actually + /// belongs to it, so checking extra accounts never produces false positives. + fn fund_bearing_account_types() -> Vec { + vec![ + AccountTypeToCheck::StandardBIP44, + AccountTypeToCheck::StandardBIP32, + AccountTypeToCheck::CoinJoin, + AccountTypeToCheck::DashpayReceivingFunds, + AccountTypeToCheck::DashpayExternalAccount, + ] + } + /// Determine which account types should be checked for a given transaction type pub fn get_relevant_account_types(tx_type: &TransactionType) -> Vec { match tx_type { - TransactionType::Standard => { - vec![ - AccountTypeToCheck::StandardBIP44, - AccountTypeToCheck::StandardBIP32, - AccountTypeToCheck::DashpayReceivingFunds, - AccountTypeToCheck::DashpayExternalAccount, - ] + // Standard and CoinJoin transactions are distinguished only by their stored label; + // discovery is membership-based, so both check every fund-bearing account. + TransactionType::Standard | TransactionType::CoinJoin => { + Self::fund_bearing_account_types() } - TransactionType::CoinJoin => vec![AccountTypeToCheck::CoinJoin], TransactionType::ProviderRegistration => vec![ AccountTypeToCheck::ProviderOwnerKeys, AccountTypeToCheck::ProviderOperatorKeys, @@ -141,7 +156,12 @@ impl TransactionRouter { } } - /// Check if a transaction appears to be a CoinJoin transaction + /// Check if a transaction appears to be a CoinJoin transaction. + /// + /// This heuristic only determines the stored [`TransactionType`] label; it never gates which + /// accounts are consulted for ownership (that is membership-based across all keychains, like + /// Dash Core). A small denomination spend that fails this heuristic is still discovered by the + /// CoinJoin account because that account owns the relevant scriptPubKeys. fn is_coinjoin_transaction(tx: &Transaction) -> bool { // CoinJoin transactions typically have: // - Multiple inputs from different addresses @@ -154,13 +174,14 @@ impl TransactionRouter { /// Check if transaction has denomination outputs typical of CoinJoin fn has_denomination_outputs(tx: &Transaction) -> bool { - // Check for standard CoinJoin denominations + // Standard CoinJoin denominations, each including the per-round fee + // (Dash Core `coinjoin/common.h`): denom + denom/1000 + 1, with COIN = 100_000_000. const COINJOIN_DENOMINATIONS: [u64; 5] = [ - 100_000_000, // 1 DASH - 10_000_000, // 0.1 DASH - 1_000_000, // 0.01 DASH - 100_000, // 0.001 DASH - 10_000, // 0.0001 DASH + 1_000_010_000, // 10 DASH + fee + 100_001_000, // 1 DASH + fee + 10_000_100, // 0.1 DASH + fee + 1_000_010, // 0.01 DASH + fee + 100_001, // 0.001 DASH + fee ]; let mut denomination_count = 0; diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/classification.rs b/key-wallet/src/transaction_checking/transaction_router/tests/classification.rs index 9996c1bbb..30fbe8277 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/classification.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/classification.rs @@ -33,11 +33,11 @@ fn test_classify_coinjoin_transaction() { &addr, 0..5, &[ - 100_000_000, // 1 DASH denomination - 100_000_000, // 1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 1_000_000, // 0.01 DASH denomination + 100_001_000, // 1 DASH denomination (+ fee) + 100_001_000, // 1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 1_000_010, // 0.01 DASH denomination (+ fee) ], ); assert_eq!(TransactionRouter::classify_transaction(&tx), TransactionType::CoinJoin); diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs b/key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs index f031a710b..9ca48264c 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/coinjoin.rs @@ -14,21 +14,27 @@ fn test_coinjoin_mixing_round() { &addr, 0..6, // Multiple participants &[ - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination - 10_000_000, // 0.1 DASH denomination + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) + 10_000_100, // 0.1 DASH denomination (+ fee) ], ); let tx_type = TransactionRouter::classify_transaction(&tx); assert_eq!(tx_type, TransactionType::CoinJoin); + // The CoinJoin label does not narrow discovery: ownership is membership-based, so a CoinJoin + // tx checks every fund-bearing account (it commonly touches standard funds for collateral, + // funding, and change too). let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - assert_eq!(accounts.len(), 1); - assert_eq!(accounts[0], AccountTypeToCheck::CoinJoin); + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); + assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); } #[test] @@ -39,14 +45,14 @@ fn test_coinjoin_with_multiple_denominations() { &addr, 0..8, &[ - 100_000_000, // 1 DASH - 100_000_000, // 1 DASH - 10_000_000, // 0.1 DASH - 10_000_000, // 0.1 DASH - 1_000_000, // 0.01 DASH - 1_000_000, // 0.01 DASH - 100_000, // 0.001 DASH - 100_000, // 0.001 DASH + 100_001_000, // 1 DASH (+ fee) + 100_001_000, // 1 DASH (+ fee) + 10_000_100, // 0.1 DASH (+ fee) + 10_000_100, // 0.1 DASH (+ fee) + 1_000_010, // 0.01 DASH (+ fee) + 1_000_010, // 0.01 DASH (+ fee) + 100_001, // 0.001 DASH (+ fee) + 100_001, // 0.001 DASH (+ fee) ], ); @@ -54,7 +60,8 @@ fn test_coinjoin_with_multiple_denominations() { assert_eq!(tx_type, TransactionType::CoinJoin); let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - assert_eq!(accounts[0], AccountTypeToCheck::CoinJoin); + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); } #[test] @@ -65,8 +72,8 @@ fn test_coinjoin_threshold_exactly_half_denominations() { &addr, 0..4, &[ - 100_000_000, // Denomination - 100_000_000, // Denomination + 100_001_000, // Denomination + 100_001_000, // Denomination 50_000_000, // Non-denomination 50_000_000, // Non-denomination ], @@ -85,7 +92,7 @@ fn test_not_coinjoin_just_under_threshold() { &addr, 0..3, &[ - 100_000_000, // Denomination + 100_001_000, // Denomination 50_000_000, // Non-denomination 75_000_000, // Non-denomination 25_000_000, // Non-denomination diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/routing.rs b/key-wallet/src/transaction_checking/transaction_router/tests/routing.rs index a3dcfd2e5..b5a49a1d5 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/routing.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/routing.rs @@ -14,7 +14,7 @@ use crate::wallet::initialization::WalletAccountCreationOptions; use crate::wallet::{ManagedWalletInfo, Wallet}; use crate::Network; use dashcore::blockdata::transaction::Transaction; -use dashcore::{ScriptBuf, TxOut}; +use dashcore::TxOut; #[test] fn test_standard_transaction_routing() { @@ -23,6 +23,10 @@ fn test_standard_transaction_routing() { assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + // A standard-shaped tx can still spend or fund a CoinJoin UTXO (small denomination spends + // classify as Standard), so the CoinJoin account must be checked too. Discovery is + // membership-based, like Dash Core's `IsMine`, not gated on the tx shape. + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); } #[tokio::test] @@ -170,71 +174,57 @@ async fn test_transaction_routing_to_coinjoin_account() { .expect("Expected CoinJoin account at index 0 to exist"); let xpub = account.account_xpub; - let managed_account = managed_wallet_info - .first_coinjoin_managed_account_mut() - .expect("Failed to get first CoinJoin managed account"); - - // Get an address from the CoinJoin account - // Note: CoinJoin accounts may have special address generation logic - // This might fail if next_receive_address is not supported for CoinJoin accounts - let address = match managed_account.get_next_address_index() { - Some(_) => { - // For CoinJoin accounts, we might need different address generation - // Let's try to get an address from the pool directly - if let ManagedAccountType::CoinJoin { - addresses, - .. - } = managed_account.managed_account_type_mut() - { - addresses.next_unused(&KeySource::Public(xpub), true).unwrap_or_else(|_| { - // If that fails, generate a dummy address for testing - dashcore::Address::p2pkh( - &dashcore::PublicKey::from_slice(&[0x02; 33]) - .expect("Failed to create public key from bytes"), - Network::Testnet, - ) - }) - } else { - panic!("Expected CoinJoin account type"); - } - } - None => { - // Generate a dummy address for testing - dashcore::Address::p2pkh( - &dashcore::PublicKey::from_slice(&[0x02; 33]) - .expect("Failed to create public key from bytes"), - Network::Testnet, - ) - } + // Derive a real address owned by the CoinJoin account so ownership is genuine. + let address = { + let managed_account = managed_wallet_info + .first_coinjoin_managed_account_mut() + .expect("Failed to get first CoinJoin managed account"); + let ManagedAccountType::CoinJoin { + addresses, + .. + } = managed_account.managed_account_type_mut() + else { + panic!("Expected CoinJoin account type"); + }; + addresses + .next_unused(&KeySource::Public(xpub), true) + .expect("Failed to derive CoinJoin address") }; - // Create a CoinJoin-like transaction (multiple inputs/outputs with same denominations) + // A small 2-in/2-out CoinJoin denomination spend classifies as `Standard` (the CoinJoin + // heuristic needs >= 3 inputs and outputs), yet one output pays our CoinJoin address. Routing + // must still attribute it to the CoinJoin account because discovery is membership-based. let addr = test_addr(); - let mut tx = Transaction::dummy(&addr, 0..1, &[100_000]); - - // Add multiple outputs with CoinJoin denominations + let mut tx = Transaction::dummy(&addr, 0..2, &[100_001]); + tx.output.clear(); tx.output.push(TxOut { - value: 100_000, // 0.001 DASH (standard CoinJoin denomination) + value: 100_001, // 0.001 DASH + per-round fee, paid to our CoinJoin address script_pubkey: address.script_pubkey(), }); tx.output.push(TxOut { - value: 100_000, // Same denomination for other participants - script_pubkey: ScriptBuf::new(), - }); - tx.output.push(TxOut { - value: 100_000, - script_pubkey: ScriptBuf::new(), + value: 100_001, // counterparty output, not ours + script_pubkey: test_addr().script_pubkey(), }); + assert_eq!( + TransactionRouter::classify_transaction(&tx), + TransactionType::Standard, + "2-in/2-out denomination spend should classify as Standard" + ); + let context = TransactionContext::InBlock(test_block_info(100000)); let result = managed_wallet_info.check_core_transaction(&tx, context, &mut wallet, true, true).await; - // This test may fail if CoinJoin detection is not properly implemented - println!( - "CoinJoin transaction result: is_relevant={}, received={}", - result.is_relevant, result.total_received + assert!(result.is_relevant, "CoinJoin spend paying our CoinJoin address should be relevant"); + + let coinjoin_account = managed_wallet_info + .first_coinjoin_managed_account() + .expect("CoinJoin managed account should exist"); + assert!( + coinjoin_account.transactions().contains_key(&tx.txid()), + "tx should be attributed to the CoinJoin account" ); } @@ -428,8 +418,13 @@ fn test_coinjoin_transaction_routing() { let tx_type = TransactionType::CoinJoin; let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - assert_eq!(accounts.len(), 1); - assert_eq!(accounts[0], AccountTypeToCheck::CoinJoin); + // A CoinJoin tx checks every fund-bearing account, since it can also touch standard funds + // (collateral, funding/change). Discovery is membership-based, not gated on the tx shape. + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); + assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); } #[test] diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 734fe1566..9a5f803bf 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -397,9 +397,11 @@ mod tests { let result = managed_wallet.check_core_transaction(&tx, context, &mut wallet, true, true).await; - // Since this is not a coinjoin looking transaction, we should not pick up on it. - assert!(!result.is_relevant); - assert_eq!(result.total_received, 0); + // The tx does not look like CoinJoin (it classifies as Standard), but routing checks + // every fund-bearing account, so a payment to our CoinJoin address is still picked up. + // Discovery is membership-based, like Dash Core's `IsMine`, not gated on the tx shape. + assert!(result.is_relevant); + assert_eq!(result.total_received, 75_000); } } @@ -1655,7 +1657,7 @@ mod tests { }; // Build a CoinJoin-like tx: 3+ inputs, 3+ outputs with denomination amounts - let denomination = 100_000u64; // 0.001 DASH + let denomination = 100_001u64; // 0.001 DASH + per-round fee let external_addr = Address::dummy(Network::Testnet, 99); let tx = Transaction { version: 2,