From 951b79026b2488406e41eb2225105f107cc1a0fa Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 08:30:22 -0500 Subject: [PATCH 01/11] fix(key-wallet): route Coinbase/AssetUnlock to all fund-bearing accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coinbase and AssetUnlock outputs can pay any user-chosen address, including CoinJoin and DashPay. get_relevant_account_types previously returned only StandardBIP44/BIP32 for those classifications, so a match on those addresses was dropped after the block download — the credit-side mirror of the #867 AssetLock debit bug. Use fund_bearing_account_types() for both arms (membership-based discovery, like Dash Core's IsMine). Update routing unit tests to expect all five fund-bearing types. Closes #900 --- .../transaction_router/mod.rs | 17 ++++++++++------- .../transaction_router/tests/asset_unlock.rs | 13 ++++++++++--- .../transaction_router/tests/coinbase.rs | 11 +++++++---- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/key-wallet/src/transaction_checking/transaction_router/mod.rs b/key-wallet/src/transaction_checking/transaction_router/mod.rs index 51f35acbe..d974ac3a3 100644 --- a/key-wallet/src/transaction_checking/transaction_router/mod.rs +++ b/key-wallet/src/transaction_checking/transaction_router/mod.rs @@ -156,14 +156,17 @@ impl TransactionRouter { ]); accounts } - TransactionType::AssetUnlock => { - vec![AccountTypeToCheck::StandardBIP44, AccountTypeToCheck::StandardBIP32] + // Credit-side mirror of the AssetLock debit fix (#867 / #900): a coinbase + // (mining reward / masternode payout) or asset unlock (Platform credit + // withdrawal) can pay any user-chosen address, including CoinJoin and + // DashPay. Only the account types returned here are consulted for + // ownership, so omitting those accounts dropped the coin after the + // block was already downloaded. Discovery is membership-based like + // Dash Core's `IsMine`, so consulting the full fund-bearing set never + // yields false positives. + TransactionType::AssetUnlock | TransactionType::Coinbase => { + Self::fund_bearing_account_types() } - TransactionType::Coinbase => vec![ - // Check all account types for unknown special transactions - AccountTypeToCheck::StandardBIP44, - AccountTypeToCheck::StandardBIP32, - ], TransactionType::Ignored => vec![], } } diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs b/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs index 6371710b1..9b911342f 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs @@ -21,10 +21,14 @@ fn test_asset_unlock_routing() { let tx_type = TransactionType::AssetUnlock; let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - // Asset unlock only goes to standard accounts - assert_eq!(accounts.len(), 2); + // Asset unlock withdrawals can pay any fund-bearing address (destination is + // user-chosen), so routing must cover the full fund-bearing set. + assert_eq!(accounts.len(), 5, "AssetUnlock should route to all fund-bearing account types"); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); // Should NOT check identity accounts - those are for locks only assert!(!accounts.contains(&AccountTypeToCheck::IdentityRegistration)); @@ -62,9 +66,12 @@ fn test_asset_unlock_classification() { // Verify routing for AssetUnlock let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - assert_eq!(accounts.len(), 2, "AssetUnlock should route to exactly 2 account types"); + assert_eq!(accounts.len(), 5, "AssetUnlock should route to all fund-bearing account types"); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); } #[tokio::test] diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs b/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs index 0ed906a49..d543a061d 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs @@ -273,13 +273,16 @@ fn test_coinbase_routing() { let tx_type = TransactionType::Coinbase; let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - // Coinbase should route to standard accounts - assert_eq!(accounts.len(), 2, "Coinbase should route to exactly 2 account types"); + // Coinbase can pay any fund-bearing address (mining reward / masternode + // payout is user-chosen), so routing must cover the full fund-bearing set. + assert_eq!(accounts.len(), 5, "Coinbase should route to all fund-bearing account types"); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); + assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); + assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); - // Should NOT route to special account types - assert!(!accounts.contains(&AccountTypeToCheck::CoinJoin)); + // Should NOT route to non-fund-bearing special account types assert!(!accounts.contains(&AccountTypeToCheck::IdentityRegistration)); assert!(!accounts.contains(&AccountTypeToCheck::ProviderOwnerKeys)); } From d66cd9655265b945fbbb21ff8e0fe3c4ac96cf0c Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 08:32:10 -0500 Subject: [PATCH 02/11] test(key-wallet): credit CoinJoin for coinbase and asset-unlock outputs End-to-end regressions for #900: a coinbase mining reward and an AssetUnlock Platform withdrawal that pay a CoinJoin address must be discovered, create a UTXO on that account, and credit the wallet balance. Mirrors the #867 AssetLock debit regression shape; fails on pre-fix routing where only StandardBIP44/BIP32 were consulted for those classifications. --- .../transaction_checking/wallet_checker.rs | 254 ++++++++++++++++++ 1 file changed, 254 insertions(+) diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 7ea653924..bc48c52f1 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -744,6 +744,260 @@ mod tests { ); } + /// Regression: a coinbase that pays a CoinJoin address must credit that account. + /// + /// `check_core_transaction` only consults the account types returned by + /// `TransactionRouter::get_relevant_account_types`. Before the fix, the + /// `Coinbase` arm returned only StandardBIP44/BIP32, so a mining reward or + /// masternode payout to a CoinJoin (or DashPay) address was never matched: + /// the block was still downloaded (filters query all scripts), but the + /// output was dropped purely by the account-type narrowing, undercounting + /// the balance (dashpay/rust-dashcore#900). Discovery is membership-based + /// like Dash Core's `IsMine`, so consulting the full fund-bearing set cannot + /// yield a false positive. + #[tokio::test] + async fn test_coinbase_paying_coinjoin_address_is_credited() { + use crate::managed_account::managed_account_type::ManagedAccountType; + use crate::transaction_checking::transaction_router::TransactionRouter; + + let network = Network::Testnet; + + let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None) + .expect("Should create wallet"); + wallet + .add_account( + AccountType::CoinJoin { + index: 0, + }, + None, + ) + .expect("Should add CoinJoin account"); + + let mut managed_wallet = + ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); + + let coinjoin_xpub = + wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub; + let coinjoin_address = { + let managed_account = + managed_wallet.first_coinjoin_managed_account_mut().expect("managed coinjoin"); + if let ManagedAccountType::CoinJoin { + external_addresses, + .. + } = managed_account.managed_account_type_mut() + { + external_addresses + .next_unused(&KeySource::Public(coinjoin_xpub), true) + .expect("coinjoin address") + } else { + panic!("Expected CoinJoin account type"); + } + }; + + let reward = 5_000_000_000u64; + let coinbase_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: Txid::all_zeros(), + vout: 0xffffffff, + }, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![TxOut { + value: reward, + script_pubkey: coinjoin_address.script_pubkey(), + }], + special_transaction_payload: None, + }; + assert_eq!( + TransactionRouter::classify_transaction(&coinbase_tx), + TransactionType::Coinbase, + "tx must classify as Coinbase so it routes through the Coinbase arm" + ); + + let block_height = 100_000; + let context = TransactionContext::InBlock(BlockInfo::new( + block_height, + BlockHash::from_slice(&[9u8; 32]).expect("Should create block hash"), + 1_650_000_200, + )); + let result = managed_wallet + .check_core_transaction(&coinbase_tx, context, &mut wallet, true, true) + .await; + managed_wallet.update_last_processed_height(block_height); + + assert!(result.is_relevant, "coinbase paying a CoinJoin address must be relevant"); + assert_eq!( + result.total_received, reward, + "coinbase must credit the CoinJoin output value" + ); + + let coinjoin_account = + managed_wallet.first_coinjoin_managed_account().expect("coinjoin account"); + assert!( + coinjoin_account.transactions().contains_key(&coinbase_tx.txid()), + "coinbase must be recorded on the CoinJoin account" + ); + assert_eq!( + coinjoin_account.utxos.len(), + 1, + "coinbase must create a CoinJoin UTXO" + ); + let utxo = coinjoin_account.utxos.values().next().expect("CoinJoin UTXO"); + assert!(utxo.is_coinbase, "credited UTXO must be marked coinbase"); + + // Aggregate wallet balance — the actual regression is a lost credit. + // Before the fix, routing never consulted CoinJoin, so the reward was + // dropped and immature balance stayed 0. + assert_eq!( + managed_wallet.balance.immature(), + reward, + "immature balance must credit the CoinJoin coinbase reward" + ); + assert_eq!( + managed_wallet.balance.total(), + reward, + "total balance must include the immature CoinJoin coinbase" + ); + } + + /// Sibling credit-side regression for AssetUnlock (Platform credit withdrawal). + /// + /// Same membership-based routing gap as the coinbase case above: before the + /// fix, `AssetUnlock` only consulted StandardBIP44/BIP32, so a withdrawal to + /// a CoinJoin address was never credited (dashpay/rust-dashcore#900). + #[tokio::test] + async fn test_asset_unlock_paying_coinjoin_address_is_credited() { + use crate::managed_account::managed_account_type::ManagedAccountType; + use crate::transaction_checking::transaction_router::TransactionRouter; + use dashcore::blockdata::transaction::special_transaction::asset_unlock::qualified_asset_unlock::AssetUnlockPayload; + use dashcore::blockdata::transaction::special_transaction::asset_unlock::request_info::AssetUnlockRequestInfo; + use dashcore::blockdata::transaction::special_transaction::asset_unlock::unqualified_asset_unlock::AssetUnlockBasePayload; + use dashcore::blockdata::transaction::special_transaction::TransactionPayload; + use dashcore::bls_sig_utils::BLSSignature; + + let network = Network::Testnet; + + let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None) + .expect("Should create wallet"); + wallet + .add_account( + AccountType::CoinJoin { + index: 0, + }, + None, + ) + .expect("Should add CoinJoin account"); + + let mut managed_wallet = + ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); + + let coinjoin_xpub = + wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub; + let coinjoin_address = { + let managed_account = + managed_wallet.first_coinjoin_managed_account_mut().expect("managed coinjoin"); + if let ManagedAccountType::CoinJoin { + external_addresses, + .. + } = managed_account.managed_account_type_mut() + { + external_addresses + .next_unused(&KeySource::Public(coinjoin_xpub), true) + .expect("coinjoin address") + } else { + panic!("Expected CoinJoin account type"); + } + }; + + let unlock_value = 100_000_000u64; + let asset_unlock_tx = Transaction { + version: 3, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: Txid::from_byte_array([1u8; 32]), + vout: 0, + }, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![TxOut { + value: unlock_value, + script_pubkey: coinjoin_address.script_pubkey(), + }], + special_transaction_payload: Some(TransactionPayload::AssetUnlockPayloadType( + AssetUnlockPayload { + base: AssetUnlockBasePayload { + version: 1, + index: 42, + fee: 1000, + }, + request_info: AssetUnlockRequestInfo { + request_height: 500_000, + quorum_hash: [5u8; 32].into(), + }, + quorum_sig: BLSSignature::from([6u8; 96]), + }, + )), + }; + assert_eq!( + TransactionRouter::classify_transaction(&asset_unlock_tx), + TransactionType::AssetUnlock, + "tx must classify as AssetUnlock so it routes through the AssetUnlock arm" + ); + + // Use InBlock (not chainlocked) so the full record is retained under the + // default `keep-finalized-transactions=OFF` feature; the load-bearing + // assertions are UTXO creation and confirmed balance credit. + let context = TransactionContext::InBlock(BlockInfo::new( + 500_100, + BlockHash::from_slice(&[10u8; 32]).expect("Should create block hash"), + 1_650_000_300, + )); + let result = managed_wallet + .check_core_transaction(&asset_unlock_tx, context, &mut wallet, true, true) + .await; + managed_wallet.update_last_processed_height(500_100); + + assert!( + result.is_relevant, + "asset unlock paying a CoinJoin address must be relevant" + ); + assert_eq!( + result.total_received, unlock_value, + "asset unlock must credit the CoinJoin output value" + ); + + let coinjoin_account = + managed_wallet.first_coinjoin_managed_account().expect("coinjoin account"); + assert!( + coinjoin_account.transactions().contains_key(&asset_unlock_tx.txid()), + "asset unlock must be recorded on the CoinJoin account" + ); + assert_eq!( + coinjoin_account.utxos.len(), + 1, + "asset unlock must create a CoinJoin UTXO" + ); + + assert_eq!( + managed_wallet.balance.confirmed(), + unlock_value, + "confirmed balance must credit the CoinJoin asset-unlock withdrawal" + ); + assert_eq!( + managed_wallet.balance.total(), + unlock_value, + "total balance must include the CoinJoin asset-unlock credit" + ); + } + /// Test the full coinbase maturity flow - immature to mature transition #[tokio::test] async fn test_wallet_checker_immature_transaction_flow() { From 8d09ce121d1c8ec4501641edf190958c64fc97e8 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 08:32:28 -0500 Subject: [PATCH 03/11] style(key-wallet): rustfmt wallet_checker CoinJoin credit tests --- .../transaction_checking/wallet_checker.rs | 22 ++++--------------- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index bc48c52f1..f42cdd663 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -831,10 +831,7 @@ mod tests { managed_wallet.update_last_processed_height(block_height); assert!(result.is_relevant, "coinbase paying a CoinJoin address must be relevant"); - assert_eq!( - result.total_received, reward, - "coinbase must credit the CoinJoin output value" - ); + assert_eq!(result.total_received, reward, "coinbase must credit the CoinJoin output value"); let coinjoin_account = managed_wallet.first_coinjoin_managed_account().expect("coinjoin account"); @@ -842,11 +839,7 @@ mod tests { coinjoin_account.transactions().contains_key(&coinbase_tx.txid()), "coinbase must be recorded on the CoinJoin account" ); - assert_eq!( - coinjoin_account.utxos.len(), - 1, - "coinbase must create a CoinJoin UTXO" - ); + assert_eq!(coinjoin_account.utxos.len(), 1, "coinbase must create a CoinJoin UTXO"); let utxo = coinjoin_account.utxos.values().next().expect("CoinJoin UTXO"); assert!(utxo.is_coinbase, "credited UTXO must be marked coinbase"); @@ -965,10 +958,7 @@ mod tests { .await; managed_wallet.update_last_processed_height(500_100); - assert!( - result.is_relevant, - "asset unlock paying a CoinJoin address must be relevant" - ); + assert!(result.is_relevant, "asset unlock paying a CoinJoin address must be relevant"); assert_eq!( result.total_received, unlock_value, "asset unlock must credit the CoinJoin output value" @@ -980,11 +970,7 @@ mod tests { coinjoin_account.transactions().contains_key(&asset_unlock_tx.txid()), "asset unlock must be recorded on the CoinJoin account" ); - assert_eq!( - coinjoin_account.utxos.len(), - 1, - "asset unlock must create a CoinJoin UTXO" - ); + assert_eq!(coinjoin_account.utxos.len(), 1, "asset unlock must create a CoinJoin UTXO"); assert_eq!( managed_wallet.balance.confirmed(), From b0c9aecdb3bc0b90c4fb47de8d51b1fafa1b669a Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 08:34:52 -0500 Subject: [PATCH 04/11] refactor(key-wallet): share CoinJoin credit-test setup and routing asserts Extract wallet_with_coinjoin_address() for the #900 credit regressions and assert Coinbase/AssetUnlock routing against fund_bearing_account_types() instead of re-listing the five types. Keeps the production list as the single source of truth for those unit tests. --- .../transaction_router/mod.rs | 4 +- .../transaction_router/tests/asset_unlock.rs | 22 ++--- .../transaction_router/tests/coinbase.rs | 11 +-- .../transaction_checking/wallet_checker.rs | 96 ++++++------------- 4 files changed, 46 insertions(+), 87 deletions(-) diff --git a/key-wallet/src/transaction_checking/transaction_router/mod.rs b/key-wallet/src/transaction_checking/transaction_router/mod.rs index d974ac3a3..0b2ebc0d9 100644 --- a/key-wallet/src/transaction_checking/transaction_router/mod.rs +++ b/key-wallet/src/transaction_checking/transaction_router/mod.rs @@ -88,7 +88,9 @@ impl TransactionRouter { /// 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 { + /// Visible to unit tests so routing assertions can compare against the + /// production list rather than re-listing the five types by hand. + pub(crate) fn fund_bearing_account_types() -> Vec { vec![ AccountTypeToCheck::StandardBIP44, AccountTypeToCheck::StandardBIP32, diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs b/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs index 9b911342f..1ff4205c3 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs @@ -23,12 +23,11 @@ fn test_asset_unlock_routing() { // Asset unlock withdrawals can pay any fund-bearing address (destination is // user-chosen), so routing must cover the full fund-bearing set. - assert_eq!(accounts.len(), 5, "AssetUnlock should route to all fund-bearing account types"); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); - assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); + assert_eq!( + accounts, + TransactionRouter::fund_bearing_account_types(), + "AssetUnlock should route to all fund-bearing account types" + ); // Should NOT check identity accounts - those are for locks only assert!(!accounts.contains(&AccountTypeToCheck::IdentityRegistration)); @@ -66,12 +65,11 @@ fn test_asset_unlock_classification() { // Verify routing for AssetUnlock let accounts = TransactionRouter::get_relevant_account_types(&tx_type); - assert_eq!(accounts.len(), 5, "AssetUnlock should route to all fund-bearing account types"); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); - assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); + assert_eq!( + accounts, + TransactionRouter::fund_bearing_account_types(), + "AssetUnlock should route to all fund-bearing account types" + ); } #[tokio::test] diff --git a/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs b/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs index d543a061d..0d59d675c 100644 --- a/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs +++ b/key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs @@ -275,12 +275,11 @@ fn test_coinbase_routing() { // Coinbase can pay any fund-bearing address (mining reward / masternode // payout is user-chosen), so routing must cover the full fund-bearing set. - assert_eq!(accounts.len(), 5, "Coinbase should route to all fund-bearing account types"); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP44)); - assert!(accounts.contains(&AccountTypeToCheck::StandardBIP32)); - assert!(accounts.contains(&AccountTypeToCheck::CoinJoin)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayReceivingFunds)); - assert!(accounts.contains(&AccountTypeToCheck::DashpayExternalAccount)); + assert_eq!( + accounts, + TransactionRouter::fund_bearing_account_types(), + "Coinbase should route to all fund-bearing account types" + ); // Should NOT route to non-fund-bearing special account types assert!(!accounts.contains(&AccountTypeToCheck::IdentityRegistration)); diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index f42cdd663..15717d5b0 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -257,6 +257,32 @@ mod tests { use dashcore::{Address, BlockHash, TxIn, Txid}; use dashcore_hashes::Hash; + /// Wallet with a single CoinJoin account and one derived external address. + /// Shared fixture for credit/debit regressions that target CoinJoin ownership. + fn wallet_with_coinjoin_address() -> (Wallet, ManagedWalletInfo, Address) { + let network = Network::Testnet; + let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None) + .expect("Should create wallet"); + wallet + .add_account( + AccountType::CoinJoin { + index: 0, + }, + None, + ) + .expect("Should add CoinJoin account"); + let mut managed_wallet = + ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); + let coinjoin_xpub = + wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub; + let coinjoin_address = managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin") + .next_address(Some(&coinjoin_xpub), true) + .expect("coinjoin address"); + (wallet, managed_wallet, coinjoin_address) + } + /// Test wallet checker with unrelated transaction #[tokio::test] async fn test_wallet_checker_unrelated_transaction() { @@ -757,42 +783,9 @@ mod tests { /// yield a false positive. #[tokio::test] async fn test_coinbase_paying_coinjoin_address_is_credited() { - use crate::managed_account::managed_account_type::ManagedAccountType; use crate::transaction_checking::transaction_router::TransactionRouter; - let network = Network::Testnet; - - let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None) - .expect("Should create wallet"); - wallet - .add_account( - AccountType::CoinJoin { - index: 0, - }, - None, - ) - .expect("Should add CoinJoin account"); - - let mut managed_wallet = - ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); - - let coinjoin_xpub = - wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub; - let coinjoin_address = { - let managed_account = - managed_wallet.first_coinjoin_managed_account_mut().expect("managed coinjoin"); - if let ManagedAccountType::CoinJoin { - external_addresses, - .. - } = managed_account.managed_account_type_mut() - { - external_addresses - .next_unused(&KeySource::Public(coinjoin_xpub), true) - .expect("coinjoin address") - } else { - panic!("Expected CoinJoin account type"); - } - }; + let (mut wallet, mut managed_wallet, coinjoin_address) = wallet_with_coinjoin_address(); let reward = 5_000_000_000u64; let coinbase_tx = Transaction { @@ -865,7 +858,6 @@ mod tests { /// a CoinJoin address was never credited (dashpay/rust-dashcore#900). #[tokio::test] async fn test_asset_unlock_paying_coinjoin_address_is_credited() { - use crate::managed_account::managed_account_type::ManagedAccountType; use crate::transaction_checking::transaction_router::TransactionRouter; use dashcore::blockdata::transaction::special_transaction::asset_unlock::qualified_asset_unlock::AssetUnlockPayload; use dashcore::blockdata::transaction::special_transaction::asset_unlock::request_info::AssetUnlockRequestInfo; @@ -873,39 +865,7 @@ mod tests { use dashcore::blockdata::transaction::special_transaction::TransactionPayload; use dashcore::bls_sig_utils::BLSSignature; - let network = Network::Testnet; - - let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None) - .expect("Should create wallet"); - wallet - .add_account( - AccountType::CoinJoin { - index: 0, - }, - None, - ) - .expect("Should add CoinJoin account"); - - let mut managed_wallet = - ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); - - let coinjoin_xpub = - wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub; - let coinjoin_address = { - let managed_account = - managed_wallet.first_coinjoin_managed_account_mut().expect("managed coinjoin"); - if let ManagedAccountType::CoinJoin { - external_addresses, - .. - } = managed_account.managed_account_type_mut() - { - external_addresses - .next_unused(&KeySource::Public(coinjoin_xpub), true) - .expect("coinjoin address") - } else { - panic!("Expected CoinJoin account type"); - } - }; + let (mut wallet, mut managed_wallet, coinjoin_address) = wallet_with_coinjoin_address(); let unlock_value = 100_000_000u64; let asset_unlock_tx = Transaction { From 49590048f200879a0bfc1d89f003b0154ed0c922 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:42:02 -0500 Subject: [PATCH 05/11] fix(dash-spv): harden dashd test harness wallet load and startup Stop treating every loadwallet failure as permission to createwallet. The regtest fixtures already ship a `default` wallet, so the old fallback raced into "Database already exists" under parallel Windows CI. Clear stale fixture lock files, classify wallet RPC errors, surface readiness failures with debug.log context, and retain datadirs on startup panics so CI artifacts are useful. Fixes: dashpay/rust-dashcore#903 --- dash-spv/src/test_utils/context.rs | 13 +- dash-spv/src/test_utils/fs_helpers.rs | 69 +++++ dash-spv/src/test_utils/node.rs | 398 +++++++++++++++++++++++--- 3 files changed, 442 insertions(+), 38 deletions(-) diff --git a/dash-spv/src/test_utils/context.rs b/dash-spv/src/test_utils/context.rs index fbb66b4ed..0d85e82e4 100644 --- a/dash-spv/src/test_utils/context.rs +++ b/dash-spv/src/test_utils/context.rs @@ -9,7 +9,7 @@ use std::net::SocketAddr; use tempfile::TempDir; use tracing::info; -use super::fs_helpers::{copy_dir, retain_test_dir}; +use super::fs_helpers::{clear_stale_runtime_locks, copy_dir, retain_test_dir, RetainOnPanic}; use super::node::TestChain; use super::{DashCoreConfig, DashCoreNode, WalletFile}; @@ -53,9 +53,16 @@ impl DashdTestContext { async fn create(mut config: DashCoreConfig) -> Self { let datadir = TempDir::new().expect("failed to create temp dir"); copy_dir(&config.datadir, datadir.path()).expect("failed to copy datadir"); + // Fixture archives are snapshots of a previously running node and may + // still contain lock files that block a fresh dashd start. + clear_stale_runtime_locks(datadir.path()); config.datadir = datadir.path().to_path_buf(); config.wallet = "wallet".to_string(); + // Retain the temp datadir if startup panics before Self is built + // (DashCoreNode::start / ensure_wallet failures). + let retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); + let wallet = WalletFile::from_json(datadir.path(), "wallet"); info!( "Loaded '{}' wallet: {} transactions, {} UTXOs, balance: {:.8} DASH", @@ -68,7 +75,8 @@ impl DashdTestContext { // Load a separate wallet for mining so coinbase rewards don't pollute // the test wallet's address space (the "wallet" wallet and SPV wallet - // share the same mnemonic). + // share the same mnemonic). The fixture already ships this wallet on + // disk; ensure_wallet must load it rather than recreate it. node.ensure_wallet("default"); info!("Mining wallet 'default' ready"); @@ -80,6 +88,7 @@ impl DashdTestContext { info!("RPC miner not available (tests requiring block generation will be skipped)"); } + retain_guard.defuse(); DashdTestContext { node, addr, diff --git a/dash-spv/src/test_utils/fs_helpers.rs b/dash-spv/src/test_utils/fs_helpers.rs index 258506cad..46daf41f9 100644 --- a/dash-spv/src/test_utils/fs_helpers.rs +++ b/dash-spv/src/test_utils/fs_helpers.rs @@ -19,6 +19,37 @@ pub(super) fn copy_dir(src: &Path, dst: &Path) -> io::Result<()> { Ok(()) } +/// Remove runtime lock files that must not survive a datadir copy. +/// +/// The regtest fixtures are snapshots of a previously running node, so they +/// may contain `regtest/.lock` and per-wallet `.walletlock` files. A live +/// dashd refuses to start (or fails wallet load) when those are present. +pub(super) fn clear_stale_runtime_locks(datadir: &Path) { + let regtest = datadir.join("regtest"); + remove_if_exists(®test.join(".lock")); + + // Wallet directories may sit under regtest// or regtest/wallets//. + for wallet_root in [regtest.clone(), regtest.join("wallets")] { + let Ok(entries) = fs::read_dir(&wallet_root) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + remove_if_exists(&path.join(".walletlock")); + } + } + } +} + +fn remove_if_exists(path: &Path) { + if path.exists() { + if let Err(e) = fs::remove_file(path) { + eprintln!("Failed to remove stale lock {}: {}", path.display(), e); + } + } +} + /// When `DASHD_TEST_RETAIN_DIR` is set, copy `src` to a test-named /// subdirectory for post-mortem inspection. /// @@ -33,6 +64,14 @@ pub fn retain_test_dir(src: &Path, label: &str) { return; } + retain_test_dir_now(src, label); +} + +/// Unconditionally retain `src` when `DASHD_TEST_RETAIN_DIR` is set. +/// +/// Use this before panicking during setup that has not yet constructed a type +/// whose `Drop` impl calls [`retain_test_dir`]. +pub(super) fn retain_test_dir_now(src: &Path, label: &str) { let Ok(retain_dir) = std::env::var("DASHD_TEST_RETAIN_DIR") else { return; }; @@ -48,3 +87,33 @@ pub fn retain_test_dir(src: &Path, label: &str) { eprintln!("Test data retained at: {}", dest.display()); } } + +/// Retains `path` on panic drop when `DASHD_TEST_RETAIN_DIR` is set. +/// +/// Used while constructing [`super::DashdTestContext`] so startup failures +/// still leave dashd logs for CI artifacts. +pub(super) struct RetainOnPanic { + path: PathBuf, + label: String, +} + +impl RetainOnPanic { + pub(super) fn new(path: impl Into, label: impl Into) -> Self { + Self { + path: path.into(), + label: label.into(), + } + } + + pub(super) fn defuse(self) { + std::mem::forget(self); + } +} + +impl Drop for RetainOnPanic { + fn drop(&mut self) { + if std::thread::panicking() { + retain_test_dir(&self.path, &self.label); + } + } +} diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index 41dcd9b71..e3d45ad27 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -9,6 +9,7 @@ use serde::Deserialize; use serde_json::{Map, Value}; use std::collections::HashMap; use std::fs; +use std::io::Read; use std::net::SocketAddr; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU16, Ordering}; @@ -16,6 +17,30 @@ use std::time::Duration; use tokio::process::Child; use tokio::time::{sleep, timeout}; +use super::fs_helpers::{clear_stale_runtime_locks, retain_test_dir_now}; + +/// Default readiness wait for dashd startup. +/// +/// Windows CI hosts frequently need longer than Unix runners when several +/// independent dashd processes start in parallel. Override with +/// `DASHD_STARTUP_TIMEOUT_SECS` when diagnosing slow environments. +fn readiness_timeout() -> Duration { + const DEFAULT_SECS: u64 = if cfg!(windows) { 90 } else { 30 }; + match std::env::var("DASHD_STARTUP_TIMEOUT_SECS") { + Ok(raw) => match raw.parse::() { + Ok(secs) if secs > 0 => Duration::from_secs(secs), + _ => { + tracing::warn!( + "invalid DASHD_STARTUP_TIMEOUT_SECS={raw:?}; using default {}s", + DEFAULT_SECS + ); + Duration::from_secs(DEFAULT_SECS) + } + }, + Err(_) => Duration::from_secs(DEFAULT_SECS), + } +} + /// Atomic counter for unique port allocation across parallel tests. /// Starts below the standard Dash regtest ports (19898/19899) to avoid conflicts. static NEXT_PORT: AtomicU16 = AtomicU16::new(19400); @@ -133,6 +158,8 @@ impl DashCoreNode { tracing::info!(" RPC port: {}", self.config.rpc_port); fs::create_dir_all(&self.config.datadir).expect("failed to create datadir"); + // Fixture snapshots may include lock files from the process that built them. + clear_stale_runtime_locks(&self.config.datadir); let mut args_vec = vec![ "-regtest".to_string(), @@ -170,22 +197,19 @@ impl DashCoreNode { self.process = Some(child); - tracing::info!("Waiting for dashd to be ready..."); + tracing::info!( + "Waiting for dashd to be ready (timeout {}s)...", + readiness_timeout().as_secs() + ); + // Brief yield so a process that dies on spawn is observed immediately. tokio::time::sleep(Duration::from_millis(500)).await; - if let Some(ref mut proc) = self.process { - if let Ok(Some(status)) = proc.try_wait() { - panic!("dashd exited immediately with status: {}", status); - } + if let Some(status) = self.process_exit_status() { + self.fail_startup(&format!("dashd exited immediately with status: {status}")); } - let ready = self.wait_for_ready().await; - if !ready { - if let Some(ref mut proc) = self.process { - if let Ok(Some(status)) = proc.try_wait() { - panic!("dashd exited with status: {}", status); - } - } - panic!("dashd failed to start within timeout"); + match self.wait_for_ready().await { + Ok(()) => {} + Err(reason) => self.fail_startup(&reason), } let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); @@ -194,40 +218,90 @@ impl DashCoreNode { addr } - async fn wait_for_ready(&self) -> bool { - let max_wait = Duration::from_secs(30); + fn process_exit_status(&mut self) -> Option { + let proc = self.process.as_mut()?; + match proc.try_wait() { + Ok(status) => status, + Err(e) => { + tracing::warn!("failed to poll dashd process status: {e}"); + None + } + } + } + + fn fail_startup(&self, reason: &str) -> ! { + let debug_log = self.config.datadir.join("regtest/debug.log"); + let tail = read_log_tail(&debug_log, 40); + // Ensure CI artifacts capture the datadir even if the caller has not + // yet constructed a Drop-based retainer. + retain_test_dir_now(&self.config.datadir, &format!("dashd-{}", self.config.p2p_port)); + panic!( + "{reason}\n binary: {}\n datadir: {}\n p2p: {}\n rpc: {}\n debug.log tail:\n{tail}", + self.config.dashd_path.display(), + self.config.datadir.display(), + self.config.p2p_port, + self.config.rpc_port, + ); + } + + async fn wait_for_ready(&mut self) -> Result<(), String> { + let max_wait = readiness_timeout(); let check_interval = Duration::from_millis(500); + let mut last_rpc_error = String::from("no RPC attempt yet"); + let mut p2p_ready = false; + let mut cookie_seen = false; let result = timeout(max_wait, async { - // Wait for the P2P port to accept connections loop { - let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); - if tokio::net::TcpStream::connect(addr).await.is_ok() { - break; + if let Some(status) = self.process_exit_status() { + return Err(format!("dashd exited during startup with status: {status}")); + } + + if !p2p_ready { + let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); + if tokio::net::TcpStream::connect(addr).await.is_ok() { + p2p_ready = true; + tracing::debug!("dashd P2P port accepting connections"); + } else { + sleep(check_interval).await; + continue; + } } - sleep(check_interval).await; - } - // Wait for RPC to be fully responsive (not just "warming up") - loop { let url = format!("http://127.0.0.1:{}", self.config.rpc_port); let cookie_path = self.config.datadir.join("regtest/.cookie"); if cookie_path.exists() { - if let Ok(client) = Client::new(&url, Auth::CookieFile(cookie_path)) { - match client.get_blockchain_info() { - Ok(_) => return true, + cookie_seen = true; + match Client::new(&url, Auth::CookieFile(cookie_path)) { + Ok(client) => match client.get_blockchain_info() { + Ok(_) => return Ok(()), Err(e) => { - tracing::debug!("RPC not ready yet: {}", e); + last_rpc_error = format!("getblockchaininfo: {e}"); + tracing::debug!("RPC not ready yet: {e}"); } + }, + Err(e) => { + last_rpc_error = format!("cookie auth: {e}"); + tracing::debug!("RPC client not ready yet: {e}"); } } + } else { + last_rpc_error = "RPC cookie file not created yet".to_string(); } sleep(check_interval).await; } }) .await; - result.unwrap_or(false) + match result { + Ok(Ok(())) => Ok(()), + Ok(Err(e)) => Err(e), + Err(_) => Err(format!( + "dashd failed to become ready within {}s \ + (p2p_ready={p2p_ready}, cookie_seen={cookie_seen}, last_rpc_error={last_rpc_error})", + max_wait.as_secs() + )), + } } /// Get block count via RPC. @@ -254,16 +328,91 @@ impl DashCoreNode { Client::new(&url, auth).expect("failed to create rpc client") } - /// Load a wallet by name, creating it if it doesn't exist. + /// Load a wallet by name, creating it only when dashd reports it is missing. + /// + /// The regtest fixtures ship both a `wallet` and a `default` wallet on + /// disk. Treating every `loadwallet` failure as permission to call + /// `createwallet` races with those existing databases and panics with + /// "Database already exists" (especially under parallel Windows CI). pub fn ensure_wallet(&self, wallet_name: &str) { - let client = self.rpc_client(); + // Wallet management RPCs are node-global; use the base endpoint so we + // are not coupled to whichever wallet was started with `-wallet=`. + let client = self.rpc_client_base(); match client.load_wallet(wallet_name) { - Ok(_) => tracing::info!("Loaded wallet: {}", wallet_name), - Err(_) => { - client - .create_wallet(wallet_name, None, None, None, None) - .unwrap_or_else(|e| panic!("failed to create wallet '{}': {}", wallet_name, e)); - tracing::info!("Created wallet: {}", wallet_name); + Ok(_) => { + tracing::info!("Loaded wallet: {wallet_name}"); + return; + } + Err(e) if wallet_already_loaded(&e) => { + tracing::info!("Wallet already loaded: {wallet_name}"); + return; + } + Err(e) if wallet_does_not_exist(&e) => { + tracing::info!("Wallet {wallet_name} not found; creating"); + } + Err(e) => { + // Prefer loading an on-disk wallet over creating a new one when + // the error is ambiguous but the database path already exists. + if wallet_database_exists(self.config.datadir.as_path(), wallet_name) { + panic!( + "failed to load existing wallet '{wallet_name}' \ + (database present under datadir): {e}" + ); + } + panic!("failed to load wallet '{wallet_name}': {e}"); + } + } + + match client.create_wallet(wallet_name, None, None, None, None) { + Ok(_) => tracing::info!("Created wallet: {wallet_name}"), + Err(e) if wallet_already_loaded(&e) || wallet_already_exists(&e) => { + // Lost a race with another load/create, or the wallet appeared + // on disk between our load and create attempts. Confirm it is + // usable rather than treating the create error as success. + self.confirm_wallet_available(&client, wallet_name, &e); + } + Err(e) => panic!("failed to create wallet '{wallet_name}': {e}"), + } + } + + /// Base (non-wallet) RPC client for node-global methods. + fn rpc_client_base(&self) -> Client { + let url = format!("http://127.0.0.1:{}", self.config.rpc_port); + let cookie_path = self.config.datadir.join("regtest/.cookie"); + assert!( + cookie_path.exists(), + "RPC cookie file not found at {}. Is dashd running with this datadir?", + cookie_path.display() + ); + Client::new(&url, Auth::CookieFile(cookie_path)).expect("failed to create rpc client") + } + + fn confirm_wallet_available( + &self, + client: &Client, + wallet_name: &str, + create_err: &dashcore_rpc::Error, + ) { + match client.load_wallet(wallet_name) { + Ok(_) => { + tracing::info!("Loaded wallet after create race: {wallet_name}"); + return; + } + Err(e) if wallet_already_loaded(&e) => { + tracing::info!("Wallet already loaded after create race: {wallet_name}"); + return; + } + Err(load_err) => { + if let Ok(wallets) = client.list_wallets() { + if wallets.iter().any(|w| w == wallet_name) { + tracing::info!("Wallet {wallet_name} present in listwallets"); + return; + } + } + panic!( + "failed to create wallet '{wallet_name}': {create_err}; \ + subsequent load also failed: {load_err}" + ); } } } @@ -565,3 +714,180 @@ impl WalletFile { serde_json::from_str(&contents).expect("Failed to deserialize wallet file") } } + +/// RPC error code used by Bitcoin/Dash Core when a wallet file is missing. +const RPC_WALLET_NOT_FOUND: i32 = -18; +/// RPC error code used when a wallet is already loaded. +const RPC_WALLET_ALREADY_LOADED: i32 = -35; +/// RPC error code used for generic wallet errors (e.g. database already exists). +const RPC_WALLET_ERROR: i32 = -4; + +fn rpc_error_parts(err: &dashcore_rpc::Error) -> Option<(i32, &str)> { + match err { + dashcore_rpc::Error::JsonRpc(dashcore_rpc::jsonrpc::Error::Rpc(rpc)) => { + Some((rpc.code, rpc.message.as_str())) + } + _ => None, + } +} + +/// True when `loadwallet`/`createwallet` reports the wallet is already loaded. +pub(crate) fn wallet_already_loaded(err: &dashcore_rpc::Error) -> bool { + match rpc_error_parts(err) { + Some((RPC_WALLET_ALREADY_LOADED, _)) => true, + Some((_, msg)) => { + let lower = msg.to_ascii_lowercase(); + lower.contains("already loaded") + } + None => false, + } +} + +/// True when `loadwallet` reports the wallet file does not exist. +pub(crate) fn wallet_does_not_exist(err: &dashcore_rpc::Error) -> bool { + match rpc_error_parts(err) { + Some((RPC_WALLET_NOT_FOUND, _)) => true, + Some((_, msg)) => { + let lower = msg.to_ascii_lowercase(); + lower.contains("not found") + || (lower.contains("does not exist") && lower.contains("wallet")) + } + None => false, + } +} + +/// True when `createwallet` reports the wallet database already exists. +pub(crate) fn wallet_already_exists(err: &dashcore_rpc::Error) -> bool { + match rpc_error_parts(err) { + Some((code, msg)) => { + let lower = msg.to_ascii_lowercase(); + // Bitcoin/Dash Core uses -4 (RPC_WALLET_ERROR) for this; match the + // message so minor code drift does not reintroduce create-on-exists. + let message_match = lower.contains("database already exists") + || (lower.contains("already exists") + && (lower.contains("wallet") || lower.contains("database"))); + message_match || (code == RPC_WALLET_ERROR && lower.contains("already exists")) + } + None => false, + } +} + +/// Whether a wallet database directory already exists under the datadir. +pub(crate) fn wallet_database_exists(datadir: &Path, wallet_name: &str) -> bool { + let regtest = datadir.join("regtest"); + let candidates = [ + regtest.join(wallet_name).join("wallet.dat"), + regtest.join("wallets").join(wallet_name).join("wallet.dat"), + regtest.join(wallet_name), + regtest.join("wallets").join(wallet_name), + ]; + candidates.iter().any(|p| p.exists()) +} + +fn read_log_tail(path: &Path, max_lines: usize) -> String { + let mut file = match fs::File::open(path) { + Ok(f) => f, + Err(e) => return format!(" ", path.display(), e), + }; + let mut contents = String::new(); + if let Err(e) = file.read_to_string(&mut contents) { + return format!(" ", path.display(), e); + } + let lines: Vec<&str> = contents.lines().collect(); + let start = lines.len().saturating_sub(max_lines); + if lines.is_empty() { + return " ".to_string(); + } + lines[start..] + .iter() + .map(|l| format!(" {l}")) + .collect::>() + .join("\n") +} + +#[cfg(test)] +mod tests { + use super::*; + use dashcore_rpc::jsonrpc::error::RpcError; + use dashcore_rpc::Error as RpcErrorEnum; + use std::io::Write; + use tempfile::TempDir; + + fn rpc_err(code: i32, message: &str) -> RpcErrorEnum { + RpcErrorEnum::JsonRpc(dashcore_rpc::jsonrpc::Error::Rpc(RpcError { + code, + message: message.to_string(), + data: None, + })) + } + + #[test] + fn classifies_wallet_not_found() { + let err = rpc_err(RPC_WALLET_NOT_FOUND, "Wallet file not found."); + assert!(wallet_does_not_exist(&err)); + assert!(!wallet_already_loaded(&err)); + assert!(!wallet_already_exists(&err)); + } + + #[test] + fn classifies_wallet_already_loaded() { + let err = rpc_err(RPC_WALLET_ALREADY_LOADED, "Wallet already loaded."); + assert!(wallet_already_loaded(&err)); + assert!(!wallet_does_not_exist(&err)); + } + + #[test] + fn classifies_database_already_exists() { + // Exact message observed on Windows CI for issue #903. + let err = rpc_err( + RPC_WALLET_ERROR, + "Wallet file verification failed. Failed to create database path \ + 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\.tmpwqBeK5\\regtest\\default'. \ + Database already exists.", + ); + assert!(wallet_already_exists(&err)); + assert!(!wallet_does_not_exist(&err)); + // The old ensure_wallet treated this as create-permission; it must not + // be classified as a missing wallet. + assert!(!wallet_does_not_exist(&err)); + } + + #[test] + fn wallet_database_exists_detects_fixture_layout() { + let tmp = TempDir::new().unwrap(); + let wallet_dir = tmp.path().join("regtest").join("default"); + fs::create_dir_all(&wallet_dir).unwrap(); + fs::write(wallet_dir.join("wallet.dat"), b"dummy").unwrap(); + assert!(wallet_database_exists(tmp.path(), "default")); + assert!(!wallet_database_exists(tmp.path(), "missing")); + } + + #[test] + fn clear_stale_runtime_locks_removes_fixture_locks() { + let tmp = TempDir::new().unwrap(); + let regtest = tmp.path().join("regtest"); + let wallet = regtest.join("default"); + fs::create_dir_all(&wallet).unwrap(); + fs::write(regtest.join(".lock"), b"").unwrap(); + fs::write(wallet.join(".walletlock"), b"").unwrap(); + + clear_stale_runtime_locks(tmp.path()); + + assert!(!regtest.join(".lock").exists()); + assert!(!wallet.join(".walletlock").exists()); + } + + #[test] + fn read_log_tail_returns_last_lines() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("debug.log"); + let mut f = fs::File::create(&path).unwrap(); + for i in 0..10 { + writeln!(f, "line{i}").unwrap(); + } + let tail = read_log_tail(&path, 3); + assert!(tail.contains("line7")); + assert!(tail.contains("line9")); + assert!(!tail.contains("line0")); + } +} From 2428c52f6394ee4c81568667d8f8143526585abd Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:43:19 -0500 Subject: [PATCH 06/11] style(dash-spv): satisfy clippy and rustfmt on test harness Remove needless returns in wallet availability confirmation and apply rustfmt to the readiness timeout helper. --- dash-spv/src/test_utils/node.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index e3d45ad27..5b3b8379d 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -25,7 +25,11 @@ use super::fs_helpers::{clear_stale_runtime_locks, retain_test_dir_now}; /// independent dashd processes start in parallel. Override with /// `DASHD_STARTUP_TIMEOUT_SECS` when diagnosing slow environments. fn readiness_timeout() -> Duration { - const DEFAULT_SECS: u64 = if cfg!(windows) { 90 } else { 30 }; + const DEFAULT_SECS: u64 = if cfg!(windows) { + 90 + } else { + 30 + }; match std::env::var("DASHD_STARTUP_TIMEOUT_SECS") { Ok(raw) => match raw.parse::() { Ok(secs) if secs > 0 => Duration::from_secs(secs), @@ -396,11 +400,9 @@ impl DashCoreNode { match client.load_wallet(wallet_name) { Ok(_) => { tracing::info!("Loaded wallet after create race: {wallet_name}"); - return; } Err(e) if wallet_already_loaded(&e) => { tracing::info!("Wallet already loaded after create race: {wallet_name}"); - return; } Err(load_err) => { if let Ok(wallets) = client.list_wallets() { @@ -798,11 +800,7 @@ fn read_log_tail(path: &Path, max_lines: usize) -> String { if lines.is_empty() { return " ".to_string(); } - lines[start..] - .iter() - .map(|l| format!(" {l}")) - .collect::>() - .join("\n") + lines[start..].iter().map(|l| format!(" {l}")).collect::>().join("\n") } #[cfg(test)] From 89495c250747b2763daa69442c0662ec2926a666 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:44:47 -0500 Subject: [PATCH 07/11] refactor(dash-spv): dedupe dashd test RPC client and lock cleanup Share cookie-based Client construction across wallet and base RPC paths, and clear stale fixture locks only in DashCoreNode::start so all callers get the same policy without a double walk from DashdTestContext. --- dash-spv/src/test_utils/context.rs | 11 +++++------ dash-spv/src/test_utils/node.rs | 26 +++++++++++--------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/dash-spv/src/test_utils/context.rs b/dash-spv/src/test_utils/context.rs index 0d85e82e4..9033a5120 100644 --- a/dash-spv/src/test_utils/context.rs +++ b/dash-spv/src/test_utils/context.rs @@ -9,7 +9,7 @@ use std::net::SocketAddr; use tempfile::TempDir; use tracing::info; -use super::fs_helpers::{clear_stale_runtime_locks, copy_dir, retain_test_dir, RetainOnPanic}; +use super::fs_helpers::{copy_dir, retain_test_dir, RetainOnPanic}; use super::node::TestChain; use super::{DashCoreConfig, DashCoreNode, WalletFile}; @@ -53,14 +53,13 @@ impl DashdTestContext { async fn create(mut config: DashCoreConfig) -> Self { let datadir = TempDir::new().expect("failed to create temp dir"); copy_dir(&config.datadir, datadir.path()).expect("failed to copy datadir"); - // Fixture archives are snapshots of a previously running node and may - // still contain lock files that block a fresh dashd start. - clear_stale_runtime_locks(datadir.path()); + // Stale fixture locks are cleared in DashCoreNode::start (covers all + // callers, including masternode harnesses). config.datadir = datadir.path().to_path_buf(); config.wallet = "wallet".to_string(); - // Retain the temp datadir if startup panics before Self is built - // (DashCoreNode::start / ensure_wallet failures). + // Retain the temp datadir if ensure_wallet (or other post-start setup) + // panics before Self is built. start() failures retain via fail_startup. let retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); let wallet = WalletFile::from_json(datadir.path(), "wallet"); diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index 5b3b8379d..a0235892b 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -321,15 +321,23 @@ impl DashCoreNode { /// Get an RPC client targeting a specific wallet. fn rpc_client_for_wallet(&self, wallet_name: &str) -> Client { - let url = format!("http://127.0.0.1:{}/wallet/{}", self.config.rpc_port, wallet_name); + self.rpc_client_at_path(&format!("/wallet/{wallet_name}")) + } + + /// Base (non-wallet) RPC client for node-global methods. + fn rpc_client_base(&self) -> Client { + self.rpc_client_at_path("") + } + + fn rpc_client_at_path(&self, path: &str) -> Client { + let url = format!("http://127.0.0.1:{}{path}", self.config.rpc_port); let cookie_path = self.config.datadir.join("regtest/.cookie"); assert!( cookie_path.exists(), "RPC cookie file not found at {}. Is dashd running with this datadir?", cookie_path.display() ); - let auth = Auth::CookieFile(cookie_path); - Client::new(&url, auth).expect("failed to create rpc client") + Client::new(&url, Auth::CookieFile(cookie_path)).expect("failed to create rpc client") } /// Load a wallet by name, creating it only when dashd reports it is missing. @@ -379,18 +387,6 @@ impl DashCoreNode { } } - /// Base (non-wallet) RPC client for node-global methods. - fn rpc_client_base(&self) -> Client { - let url = format!("http://127.0.0.1:{}", self.config.rpc_port); - let cookie_path = self.config.datadir.join("regtest/.cookie"); - assert!( - cookie_path.exists(), - "RPC cookie file not found at {}. Is dashd running with this datadir?", - cookie_path.display() - ); - Client::new(&url, Auth::CookieFile(cookie_path)).expect("failed to create rpc client") - } - fn confirm_wallet_available( &self, client: &Client, From 4ccfcfe15ed4711794104f0ee6951ea99949feb8 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:46:47 -0500 Subject: [PATCH 08/11] refactor(dash-spv): simplify dashd test harness retain and RPC paths Install RetainOnPanic only after start succeeds so startup failures retain once, share cookie-based soft/hard RPC client construction, and thin-wrap primary wallet send helpers. Fixture mining wallet now uses load_wallet. --- dash-spv/src/test_utils/context.rs | 12 +- dash-spv/src/test_utils/fs_helpers.rs | 20 ++- dash-spv/src/test_utils/node.rs | 212 +++++++++++++------------- 3 files changed, 126 insertions(+), 118 deletions(-) diff --git a/dash-spv/src/test_utils/context.rs b/dash-spv/src/test_utils/context.rs index 9033a5120..9e62b5e3f 100644 --- a/dash-spv/src/test_utils/context.rs +++ b/dash-spv/src/test_utils/context.rs @@ -58,10 +58,6 @@ impl DashdTestContext { config.datadir = datadir.path().to_path_buf(); config.wallet = "wallet".to_string(); - // Retain the temp datadir if ensure_wallet (or other post-start setup) - // panics before Self is built. start() failures retain via fail_startup. - let retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); - let wallet = WalletFile::from_json(datadir.path(), "wallet"); info!( "Loaded '{}' wallet: {} transactions, {} UTXOs, balance: {:.8} DASH", @@ -69,14 +65,18 @@ impl DashdTestContext { ); let mut node = DashCoreNode::with_config(config); + // start() failures retain via fail_startup. Install the panic retainer + // only for post-start setup (ensure_wallet, etc.) so a start failure + // does not copy the datadir twice under two labels. let addr = node.start().await; info!("DashCoreNode started at {}", addr); + let retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); // Load a separate wallet for mining so coinbase rewards don't pollute // the test wallet's address space (the "wallet" wallet and SPV wallet // share the same mnemonic). The fixture already ships this wallet on - // disk; ensure_wallet must load it rather than recreate it. - node.ensure_wallet("default"); + // disk — load only; never create. + node.load_wallet("default"); info!("Mining wallet 'default' ready"); let initial_height = node.get_block_count(); diff --git a/dash-spv/src/test_utils/fs_helpers.rs b/dash-spv/src/test_utils/fs_helpers.rs index 46daf41f9..e9a725353 100644 --- a/dash-spv/src/test_utils/fs_helpers.rs +++ b/dash-spv/src/test_utils/fs_helpers.rs @@ -27,10 +27,13 @@ pub(super) fn copy_dir(src: &Path, dst: &Path) -> io::Result<()> { pub(super) fn clear_stale_runtime_locks(datadir: &Path) { let regtest = datadir.join("regtest"); remove_if_exists(®test.join(".lock")); + // Legacy single-wallet layout stores the lock at regtest/.walletlock. + remove_if_exists(®test.join(".walletlock")); - // Wallet directories may sit under regtest// or regtest/wallets//. - for wallet_root in [regtest.clone(), regtest.join("wallets")] { - let Ok(entries) = fs::read_dir(&wallet_root) else { + // Named wallet directories may sit under regtest// or regtest/wallets//. + let wallets_root = regtest.join("wallets"); + for wallet_root in [®test, &wallets_root] { + let Ok(entries) = fs::read_dir(wallet_root) else { continue; }; for entry in entries.flatten() { @@ -43,10 +46,10 @@ pub(super) fn clear_stale_runtime_locks(datadir: &Path) { } fn remove_if_exists(path: &Path) { - if path.exists() { - if let Err(e) = fs::remove_file(path) { - eprintln!("Failed to remove stale lock {}: {}", path.display(), e); - } + match fs::remove_file(path) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => eprintln!("Failed to remove stale lock {}: {}", path.display(), e), } } @@ -113,7 +116,8 @@ impl RetainOnPanic { impl Drop for RetainOnPanic { fn drop(&mut self) { if std::thread::panicking() { - retain_test_dir(&self.path, &self.label); + // Already know we are panicking; skip retain_test_dir's re-check. + retain_test_dir_now(&self.path, &self.label); } } } diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index a0235892b..bdd2d1ee8 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -30,19 +30,19 @@ fn readiness_timeout() -> Duration { } else { 30 }; - match std::env::var("DASHD_STARTUP_TIMEOUT_SECS") { + let secs = match std::env::var("DASHD_STARTUP_TIMEOUT_SECS") { Ok(raw) => match raw.parse::() { - Ok(secs) if secs > 0 => Duration::from_secs(secs), + Ok(secs) if secs > 0 => secs, _ => { tracing::warn!( - "invalid DASHD_STARTUP_TIMEOUT_SECS={raw:?}; using default {}s", - DEFAULT_SECS + "invalid DASHD_STARTUP_TIMEOUT_SECS={raw:?}; using default {DEFAULT_SECS}s" ); - Duration::from_secs(DEFAULT_SECS) + DEFAULT_SECS } }, - Err(_) => Duration::from_secs(DEFAULT_SECS), - } + Err(_) => DEFAULT_SECS, + }; + Duration::from_secs(secs) } /// Atomic counter for unique port allocation across parallel tests. @@ -201,19 +201,16 @@ impl DashCoreNode { self.process = Some(child); - tracing::info!( - "Waiting for dashd to be ready (timeout {}s)...", - readiness_timeout().as_secs() - ); + let ready_timeout = readiness_timeout(); + tracing::info!("Waiting for dashd to be ready (timeout {}s)...", ready_timeout.as_secs()); // Brief yield so a process that dies on spawn is observed immediately. tokio::time::sleep(Duration::from_millis(500)).await; if let Some(status) = self.process_exit_status() { self.fail_startup(&format!("dashd exited immediately with status: {status}")); } - match self.wait_for_ready().await { - Ok(()) => {} - Err(reason) => self.fail_startup(&reason), + if let Err(reason) = self.wait_for_ready(ready_timeout).await { + self.fail_startup(&reason); } let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); @@ -236,8 +233,9 @@ impl DashCoreNode { fn fail_startup(&self, reason: &str) -> ! { let debug_log = self.config.datadir.join("regtest/debug.log"); let tail = read_log_tail(&debug_log, 40); - // Ensure CI artifacts capture the datadir even if the caller has not - // yet constructed a Drop-based retainer. + // Callers that need post-start retain (e.g. DashdTestContext) install + // RetainOnPanic only after start() returns, so this is the sole retain + // path for startup failures. retain_test_dir_now(&self.config.datadir, &format!("dashd-{}", self.config.p2p_port)); panic!( "{reason}\n binary: {}\n datadir: {}\n p2p: {}\n rpc: {}\n debug.log tail:\n{tail}", @@ -248,8 +246,7 @@ impl DashCoreNode { ); } - async fn wait_for_ready(&mut self) -> Result<(), String> { - let max_wait = readiness_timeout(); + async fn wait_for_ready(&mut self, max_wait: Duration) -> Result<(), String> { let check_interval = Duration::from_millis(500); let mut last_rpc_error = String::from("no RPC attempt yet"); let mut p2p_ready = false; @@ -272,25 +269,24 @@ impl DashCoreNode { } } - let url = format!("http://127.0.0.1:{}", self.config.rpc_port); - let cookie_path = self.config.datadir.join("regtest/.cookie"); - if cookie_path.exists() { - cookie_seen = true; - match Client::new(&url, Auth::CookieFile(cookie_path)) { - Ok(client) => match client.get_blockchain_info() { + match self.try_rpc_client_base() { + Ok(client) => { + cookie_seen = true; + match client.get_blockchain_info() { Ok(_) => return Ok(()), Err(e) => { last_rpc_error = format!("getblockchaininfo: {e}"); tracing::debug!("RPC not ready yet: {e}"); } - }, - Err(e) => { - last_rpc_error = format!("cookie auth: {e}"); - tracing::debug!("RPC client not ready yet: {e}"); } } - } else { - last_rpc_error = "RPC cookie file not created yet".to_string(); + Err(e) => { + if self.rpc_cookie_path().exists() { + cookie_seen = true; + } + last_rpc_error = e; + tracing::debug!("RPC client not ready yet: {last_rpc_error}"); + } } sleep(check_interval).await; } @@ -329,23 +325,44 @@ impl DashCoreNode { self.rpc_client_at_path("") } + fn rpc_cookie_path(&self) -> PathBuf { + self.config.datadir.join("regtest/.cookie") + } + fn rpc_client_at_path(&self, path: &str) -> Client { - let url = format!("http://127.0.0.1:{}{path}", self.config.rpc_port); - let cookie_path = self.config.datadir.join("regtest/.cookie"); + let cookie_path = self.rpc_cookie_path(); assert!( cookie_path.exists(), "RPC cookie file not found at {}. Is dashd running with this datadir?", cookie_path.display() ); + let url = format!("http://127.0.0.1:{}{path}", self.config.rpc_port); Client::new(&url, Auth::CookieFile(cookie_path)).expect("failed to create rpc client") } - /// Load a wallet by name, creating it only when dashd reports it is missing. + /// Soft base RPC client for readiness probes and best-effort RPCs. + /// + /// Returns a diagnostic string on failure so readiness timeouts can report + /// whether the cookie was missing or cookie auth itself failed. + fn try_rpc_client_base(&self) -> Result { + let cookie_path = self.rpc_cookie_path(); + if !cookie_path.exists() { + return Err("RPC cookie file not created yet".to_string()); + } + let url = format!("http://127.0.0.1:{}", self.config.rpc_port); + Client::new(&url, Auth::CookieFile(cookie_path)).map_err(|e| format!("cookie auth: {e}")) + } + + /// Ensure a wallet is loaded, creating it only when Core reports it is missing + /// and no on-disk database already exists. /// /// The regtest fixtures ship both a `wallet` and a `default` wallet on /// disk. Treating every `loadwallet` failure as permission to call /// `createwallet` races with those existing databases and panics with /// "Database already exists" (especially under parallel Windows CI). + /// + /// Prefer [`Self::load_wallet`] when the wallet is known to ship in the + /// fixture (e.g. the mining `default` wallet). pub fn ensure_wallet(&self, wallet_name: &str) { // Wallet management RPCs are node-global; use the base endpoint so we // are not coupled to whichever wallet was started with `-wallet=`. @@ -360,6 +377,12 @@ impl DashCoreNode { return; } Err(e) if wallet_does_not_exist(&e) => { + if wallet_database_exists(self.config.datadir.as_path(), wallet_name) { + panic!( + "loadwallet reported wallet '{wallet_name}' missing, but a \ + database path already exists under the datadir: {e}" + ); + } tracing::info!("Wallet {wallet_name} not found; creating"); } Err(e) => { @@ -377,41 +400,39 @@ impl DashCoreNode { match client.create_wallet(wallet_name, None, None, None, None) { Ok(_) => tracing::info!("Created wallet: {wallet_name}"), - Err(e) if wallet_already_loaded(&e) || wallet_already_exists(&e) => { - // Lost a race with another load/create, or the wallet appeared - // on disk between our load and create attempts. Confirm it is - // usable rather than treating the create error as success. - self.confirm_wallet_available(&client, wallet_name, &e); + Err(e) if wallet_already_loaded(&e) => { + tracing::info!("Wallet already loaded during create: {wallet_name}"); + } + Err(e) if wallet_already_exists(&e) => { + // Database appeared between load and create. Load it rather + // than treating the create error as success by name alone. + match client.load_wallet(wallet_name) { + Ok(_) => tracing::info!("Loaded wallet after create race: {wallet_name}"), + Err(load_err) if wallet_already_loaded(&load_err) => { + tracing::info!("Wallet already loaded after create race: {wallet_name}"); + } + Err(load_err) => panic!( + "failed to create wallet '{wallet_name}': {e}; \ + subsequent load also failed: {load_err}" + ), + } } Err(e) => panic!("failed to create wallet '{wallet_name}': {e}"), } } - fn confirm_wallet_available( - &self, - client: &Client, - wallet_name: &str, - create_err: &dashcore_rpc::Error, - ) { + /// Load a wallet that is expected to already exist (fixture or prior create). + /// + /// Unlike [`Self::ensure_wallet`], this never calls `createwallet`, so a + /// shipped fixture wallet cannot race into "Database already exists". + pub fn load_wallet(&self, wallet_name: &str) { + let client = self.rpc_client_base(); match client.load_wallet(wallet_name) { - Ok(_) => { - tracing::info!("Loaded wallet after create race: {wallet_name}"); - } + Ok(_) => tracing::info!("Loaded wallet: {wallet_name}"), Err(e) if wallet_already_loaded(&e) => { - tracing::info!("Wallet already loaded after create race: {wallet_name}"); - } - Err(load_err) => { - if let Ok(wallets) = client.list_wallets() { - if wallets.iter().any(|w| w == wallet_name) { - tracing::info!("Wallet {wallet_name} present in listwallets"); - return; - } - } - panic!( - "failed to create wallet '{wallet_name}': {create_err}; \ - subsequent load also failed: {load_err}" - ); + tracing::info!("Wallet already loaded: {wallet_name}"); } + Err(e) => panic!("failed to load expected wallet '{wallet_name}': {e}"), } } @@ -455,12 +476,7 @@ impl DashCoreNode { /// Send DASH to an address from the primary wallet. pub fn send_to_address(&self, address: &Address, amount: Amount) -> Txid { - let client = self.rpc_client(); - let txid = client - .send_to_address(address, amount, None, None, None, None, None, None, None, None) - .expect("failed to send to address"); - tracing::info!("Sent {} to {}, txid: {}", amount, address, txid); - txid + self.send_to_address_from_wallet(&self.config.wallet, address, amount) } /// Send DASH to many addresses in a single transaction from the primary @@ -514,35 +530,20 @@ impl DashCoreNode { destination: &Address, fee: Amount, ) -> Txid { - let client = self.rpc_client_for_wallet(wallet_name); - - let inputs = vec![rpc_json::CreateRawTransactionInput { - txid: input_txid, - vout: input_vout, - sequence: None, - }]; - let send_amount = input_amount.checked_sub(fee).expect("fee exceeds input amount"); - let mut outputs = HashMap::new(); - outputs.insert(destination.to_string(), send_amount); - - let raw_tx: Transaction = client - .create_raw_transaction(&inputs, &outputs, None) - .expect("failed to create raw tx"); - - let signed = client - .sign_raw_transaction_with_wallet(&raw_tx, None, None) - .expect("failed to sign raw tx"); - assert!(signed.complete, "raw transaction signing incomplete"); - - let txid = client - .send_raw_transaction(&signed.transaction().expect("invalid signed tx")) - .expect("failed to send raw tx"); - tracing::info!( - "Sent raw tx from wallet '{}': {} -> {}, txid: {}", + let tx = self.create_signed_transaction( wallet_name, + input_txid, + input_vout, input_amount, destination, - txid + fee, + ); + let txid = self + .rpc_client_for_wallet(wallet_name) + .send_raw_transaction(&tx) + .expect("failed to send raw tx"); + tracing::info!( + "Sent raw tx from wallet '{wallet_name}': {input_amount} -> {destination}, txid: {txid}" ); txid } @@ -647,14 +648,7 @@ impl DashCoreNode { /// Uses the base URL (no wallet path) which works for all non-wallet RPCs. /// Useful during DKG orchestration where transient failures are expected. pub fn try_rpc_call(&self, method: &str, params: &[serde_json::Value]) -> Option { - let url = format!("http://127.0.0.1:{}", self.config.rpc_port); - let cookie_path = self.config.datadir.join("regtest/.cookie"); - if !cookie_path.exists() { - return None; - } - let auth = Auth::CookieFile(cookie_path); - let client = Client::new(&url, auth).ok()?; - client.call(method, params).ok() + self.try_rpc_client_base().ok()?.call(method, params).ok() } pub fn datadir(&self) -> &Path { @@ -744,11 +738,16 @@ pub(crate) fn wallet_already_loaded(err: &dashcore_rpc::Error) -> bool { /// True when `loadwallet` reports the wallet file does not exist. pub(crate) fn wallet_does_not_exist(err: &dashcore_rpc::Error) -> bool { match rpc_error_parts(err) { - Some((RPC_WALLET_NOT_FOUND, _)) => true, + Some((RPC_WALLET_NOT_FOUND, msg)) => { + // Code -18 is Core's wallet-not-found; still require a wallet-ish + // message so unrelated -18 codes never authorize createwallet. + let lower = msg.to_ascii_lowercase(); + lower.contains("wallet") || lower.contains("not found") + } Some((_, msg)) => { let lower = msg.to_ascii_lowercase(); - lower.contains("not found") - || (lower.contains("does not exist") && lower.contains("wallet")) + lower.contains("wallet") + && (lower.contains("not found") || lower.contains("does not exist")) } None => false, } @@ -821,6 +820,10 @@ mod tests { assert!(wallet_does_not_exist(&err)); assert!(!wallet_already_loaded(&err)); assert!(!wallet_already_exists(&err)); + + // Unrelated "not found" must not authorize createwallet. + let method = rpc_err(-32601, "Method not found"); + assert!(!wallet_does_not_exist(&method)); } #[test] @@ -840,7 +843,6 @@ mod tests { Database already exists.", ); assert!(wallet_already_exists(&err)); - assert!(!wallet_does_not_exist(&err)); // The old ensure_wallet treated this as create-permission; it must not // be classified as a missing wallet. assert!(!wallet_does_not_exist(&err)); @@ -863,11 +865,13 @@ mod tests { let wallet = regtest.join("default"); fs::create_dir_all(&wallet).unwrap(); fs::write(regtest.join(".lock"), b"").unwrap(); + fs::write(regtest.join(".walletlock"), b"").unwrap(); fs::write(wallet.join(".walletlock"), b"").unwrap(); clear_stale_runtime_locks(tmp.path()); assert!(!regtest.join(".lock").exists()); + assert!(!regtest.join(".walletlock").exists()); assert!(!wallet.join(".walletlock").exists()); } From fbb9d7b4ced16397db05ed596c938546c93faed0 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:47:06 -0500 Subject: [PATCH 09/11] fix(dash-spv): tighten wallet-not-found classification Require a wallet-related message before treating a loadwallet failure as permission to createwallet, and cover the "Method not found" false positive in unit tests. --- dash-spv/src/test_utils/node.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index bdd2d1ee8..241c54b3a 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -738,16 +738,13 @@ pub(crate) fn wallet_already_loaded(err: &dashcore_rpc::Error) -> bool { /// True when `loadwallet` reports the wallet file does not exist. pub(crate) fn wallet_does_not_exist(err: &dashcore_rpc::Error) -> bool { match rpc_error_parts(err) { - Some((RPC_WALLET_NOT_FOUND, msg)) => { - // Code -18 is Core's wallet-not-found; still require a wallet-ish - // message so unrelated -18 codes never authorize createwallet. - let lower = msg.to_ascii_lowercase(); - lower.contains("wallet") || lower.contains("not found") - } - Some((_, msg)) => { + Some((code, msg)) => { let lower = msg.to_ascii_lowercase(); - lower.contains("wallet") - && (lower.contains("not found") || lower.contains("does not exist")) + // Require a wallet-related phrase so unrelated "not found" / code + // -18 responses never authorize createwallet. + let walletish = lower.contains("wallet"); + let missing = lower.contains("not found") || lower.contains("does not exist"); + walletish && missing && (code == RPC_WALLET_NOT_FOUND || code != 0) } None => false, } From 245e6ffdba1bae53808e7ab7b76e8d0ad0c168f1 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 11:53:02 -0500 Subject: [PATCH 10/11] fix(dash-spv): kill dashd before retaining failed startup datadir Stop the node before reading/copying debug.log so Windows retain does not hit sharing violations on open handles, and only load the last 64KiB of debug.log when dumping a readiness failure. --- dash-spv/src/test_utils/node.rs | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index 241c54b3a..68b872b14 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -230,7 +230,14 @@ impl DashCoreNode { } } - fn fail_startup(&self, reason: &str) -> ! { + fn fail_startup(&mut self, reason: &str) -> ! { + // Kill dashd before reading/copying the datadir so Windows does not + // hit sharing violations on open LevelDB/wallet/debug.log handles. + if let Some(mut process) = self.process.take() { + let _ = process.start_kill(); + let _ = process.try_wait(); + } + let debug_log = self.config.datadir.join("regtest/debug.log"); let tail = read_log_tail(&debug_log, 40); // Callers that need post-start retain (e.g. DashdTestContext) install @@ -779,15 +786,36 @@ pub(crate) fn wallet_database_exists(datadir: &Path, wallet_name: &str) -> bool } fn read_log_tail(path: &Path, max_lines: usize) -> String { + use std::io::{Seek, SeekFrom}; + + // Cap how much of debug.log we load: -debug=all against large fixtures can + // produce multi-hundred-MB logs that would OOM a full read on failure. + const MAX_TAIL_BYTES: u64 = 64 * 1024; + let mut file = match fs::File::open(path) { Ok(f) => f, Err(e) => return format!(" ", path.display(), e), }; + let len = match file.metadata() { + Ok(m) => m.len(), + Err(e) => return format!(" ", path.display(), e), + }; + if len > MAX_TAIL_BYTES { + if let Err(e) = file.seek(SeekFrom::End(-(MAX_TAIL_BYTES as i64))) { + return format!(" ", path.display(), e); + } + } let mut contents = String::new(); if let Err(e) = file.read_to_string(&mut contents) { return format!(" ", path.display(), e); } - let lines: Vec<&str> = contents.lines().collect(); + // Drop a partial first line after a mid-file seek. + let body = if len > MAX_TAIL_BYTES { + contents.split_once('\n').map(|(_, rest)| rest).unwrap_or(&contents) + } else { + contents.as_str() + }; + let lines: Vec<&str> = body.lines().collect(); let start = lines.len().saturating_sub(max_lines); if lines.is_empty() { return " ".to_string(); From 528c52cc27e75fbc93cc907bf7bc8b81d7b97325 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 16 Jul 2026 13:11:46 -0500 Subject: [PATCH 11/11] fix(dash-spv): address startup review findings --- dash-spv/src/test_utils/context.rs | 12 +- dash-spv/src/test_utils/fs_helpers.rs | 83 +++++++++++-- dash-spv/src/test_utils/node.rs | 166 +++++++++++++++++++------- 3 files changed, 201 insertions(+), 60 deletions(-) diff --git a/dash-spv/src/test_utils/context.rs b/dash-spv/src/test_utils/context.rs index 9e62b5e3f..24aec046b 100644 --- a/dash-spv/src/test_utils/context.rs +++ b/dash-spv/src/test_utils/context.rs @@ -64,13 +64,16 @@ impl DashdTestContext { wallet.wallet_name, wallet.transaction_count, wallet.utxo_count, wallet.balance ); + // retain_guard is declared before node so reverse-declaration drop order + // shuts dashd down (DashCoreNode::drop / stop_and_wait) before + // RetainOnPanic copies the datadir on post-start panics. + // start() failures retain via fail_startup instead, so the guard is + // installed only after start returns. + let retain_guard; let mut node = DashCoreNode::with_config(config); - // start() failures retain via fail_startup. Install the panic retainer - // only for post-start setup (ensure_wallet, etc.) so a start failure - // does not copy the datadir twice under two labels. let addr = node.start().await; info!("DashCoreNode started at {}", addr); - let retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); + retain_guard = RetainOnPanic::new(datadir.path(), "dashd-startup"); // Load a separate wallet for mining so coinbase rewards don't pollute // the test wallet's address space (the "wallet" wallet and SPV wallet @@ -102,6 +105,7 @@ impl DashdTestContext { impl Drop for DashdTestContext { fn drop(&mut self) { let label = format!("dashd-{}", self.addr.port()); + self.node.stop_and_wait(); retain_test_dir(self.datadir.path(), &label); } } diff --git a/dash-spv/src/test_utils/fs_helpers.rs b/dash-spv/src/test_utils/fs_helpers.rs index e9a725353..d8b1c9077 100644 --- a/dash-spv/src/test_utils/fs_helpers.rs +++ b/dash-spv/src/test_utils/fs_helpers.rs @@ -24,32 +24,50 @@ pub(super) fn copy_dir(src: &Path, dst: &Path) -> io::Result<()> { /// The regtest fixtures are snapshots of a previously running node, so they /// may contain `regtest/.lock` and per-wallet `.walletlock` files. A live /// dashd refuses to start (or fails wallet load) when those are present. -pub(super) fn clear_stale_runtime_locks(datadir: &Path) { +pub(super) fn clear_stale_runtime_locks(datadir: &Path) -> io::Result<()> { let regtest = datadir.join("regtest"); - remove_if_exists(®test.join(".lock")); + remove_if_exists(®test.join(".lock"))?; // Legacy single-wallet layout stores the lock at regtest/.walletlock. - remove_if_exists(®test.join(".walletlock")); + remove_if_exists(®test.join(".walletlock"))?; // Named wallet directories may sit under regtest// or regtest/wallets//. let wallets_root = regtest.join("wallets"); for wallet_root in [®test, &wallets_root] { - let Ok(entries) = fs::read_dir(wallet_root) else { - continue; + let entries = match fs::read_dir(wallet_root) { + Ok(entries) => entries, + Err(e) if e.kind() == io::ErrorKind::NotFound => continue, + Err(e) => { + return Err(io::Error::new( + e.kind(), + format!("failed to read wallet root {}: {}", wallet_root.display(), e), + )); + } }; - for entry in entries.flatten() { + for entry in entries { + let entry = entry.map_err(|e| { + io::Error::new( + e.kind(), + format!("failed to read entry in {}: {}", wallet_root.display(), e), + ) + })?; let path = entry.path(); - if path.is_dir() { - remove_if_exists(&path.join(".walletlock")); + if entry.file_type()?.is_dir() { + remove_if_exists(&path.join(".walletlock"))?; } } } + + Ok(()) } -fn remove_if_exists(path: &Path) { +fn remove_if_exists(path: &Path) -> io::Result<()> { match fs::remove_file(path) { - Ok(()) => {} - Err(e) if e.kind() == io::ErrorKind::NotFound => {} - Err(e) => eprintln!("Failed to remove stale lock {}: {}", path.display(), e), + Ok(()) => Ok(()), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(io::Error::new( + e.kind(), + format!("failed to remove stale lock {}: {}", path.display(), e), + )), } } @@ -121,3 +139,44 @@ impl Drop for RetainOnPanic { } } } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn remove_if_exists_treats_missing_file_as_success() { + let tmp = TempDir::new().unwrap(); + remove_if_exists(&tmp.path().join("missing.lock")).unwrap(); + } + + #[test] + fn remove_if_exists_propagates_removal_failures() { + let tmp = TempDir::new().unwrap(); + let lock_path = tmp.path().join(".lock"); + fs::create_dir(&lock_path).unwrap(); + + let err = remove_if_exists(&lock_path).unwrap_err(); + + assert_ne!(err.kind(), io::ErrorKind::NotFound); + } + + #[test] + fn clear_stale_runtime_locks_treats_missing_roots_as_success() { + let tmp = TempDir::new().unwrap(); + clear_stale_runtime_locks(tmp.path()).unwrap(); + } + + #[test] + fn clear_stale_runtime_locks_propagates_directory_read_failures() { + let tmp = TempDir::new().unwrap(); + let regtest = tmp.path().join("regtest"); + fs::create_dir(®test).unwrap(); + fs::write(regtest.join("wallets"), b"not a directory").unwrap(); + + let err = clear_stale_runtime_locks(tmp.path()).unwrap_err(); + + assert_ne!(err.kind(), io::ErrorKind::NotFound); + } +} diff --git a/dash-spv/src/test_utils/node.rs b/dash-spv/src/test_utils/node.rs index 68b872b14..de27171fc 100644 --- a/dash-spv/src/test_utils/node.rs +++ b/dash-spv/src/test_utils/node.rs @@ -15,6 +15,7 @@ use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU16, Ordering}; use std::time::Duration; use tokio::process::Child; +use tokio::task; use tokio::time::{sleep, timeout}; use super::fs_helpers::{clear_stale_runtime_locks, retain_test_dir_now}; @@ -163,7 +164,12 @@ impl DashCoreNode { fs::create_dir_all(&self.config.datadir).expect("failed to create datadir"); // Fixture snapshots may include lock files from the process that built them. - clear_stale_runtime_locks(&self.config.datadir); + clear_stale_runtime_locks(&self.config.datadir).unwrap_or_else(|e| { + panic!( + "failed to clear stale dashd runtime locks from {} before startup: {e}", + self.config.datadir.display() + ) + }); let mut args_vec = vec![ "-regtest".to_string(), @@ -206,11 +212,11 @@ impl DashCoreNode { // Brief yield so a process that dies on spawn is observed immediately. tokio::time::sleep(Duration::from_millis(500)).await; if let Some(status) = self.process_exit_status() { - self.fail_startup(&format!("dashd exited immediately with status: {status}")); + self.fail_startup(&format!("dashd exited immediately with status: {status}")).await; } if let Err(reason) = self.wait_for_ready(ready_timeout).await { - self.fail_startup(&reason); + self.fail_startup(&reason).await; } let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); @@ -230,13 +236,10 @@ impl DashCoreNode { } } - fn fail_startup(&mut self, reason: &str) -> ! { + async fn fail_startup(&mut self, reason: &str) -> ! { // Kill dashd before reading/copying the datadir so Windows does not // hit sharing violations on open LevelDB/wallet/debug.log handles. - if let Some(mut process) = self.process.take() { - let _ = process.start_kill(); - let _ = process.try_wait(); - } + let shutdown_status = self.terminate_process_for_startup().await; let debug_log = self.config.datadir.join("regtest/debug.log"); let tail = read_log_tail(&debug_log, 40); @@ -245,7 +248,7 @@ impl DashCoreNode { // path for startup failures. retain_test_dir_now(&self.config.datadir, &format!("dashd-{}", self.config.p2p_port)); panic!( - "{reason}\n binary: {}\n datadir: {}\n p2p: {}\n rpc: {}\n debug.log tail:\n{tail}", + "{reason}\n binary: {}\n datadir: {}\n p2p: {}\n rpc: {}\n {shutdown_status}\n debug.log tail:\n{tail}", self.config.dashd_path.display(), self.config.datadir.display(), self.config.p2p_port, @@ -253,11 +256,34 @@ impl DashCoreNode { ); } + async fn terminate_process_for_startup(&mut self) -> String { + let Some(mut process) = self.process.take() else { + return "dashd shutdown: process was not running".to_string(); + }; + + let kill_result = process.start_kill(); + let wait_result = process.wait().await; + match (kill_result, wait_result) { + (Ok(()), Ok(status)) => format!("dashd shutdown: exited with {status}"), + (Err(kill_err), Ok(status)) => format!( + "dashd shutdown: kill request failed ({kill_err}); process exited with {status}" + ), + (Ok(()), Err(wait_err)) => { + format!("dashd shutdown: kill requested but wait failed: {wait_err}") + } + (Err(kill_err), Err(wait_err)) => { + format!("dashd shutdown: kill request failed ({kill_err}); wait failed: {wait_err}") + } + } + } + async fn wait_for_ready(&mut self, max_wait: Duration) -> Result<(), String> { let check_interval = Duration::from_millis(500); let mut last_rpc_error = String::from("no RPC attempt yet"); + let mut last_blockchain_error = String::from("no blockchain readiness attempt yet"); let mut p2p_ready = false; let mut cookie_seen = false; + let mut rpc_ready = false; let result = timeout(max_wait, async { loop { @@ -265,35 +291,48 @@ impl DashCoreNode { return Err(format!("dashd exited during startup with status: {status}")); } - if !p2p_ready { - let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); - if tokio::net::TcpStream::connect(addr).await.is_ok() { - p2p_ready = true; - tracing::debug!("dashd P2P port accepting connections"); - } else { - sleep(check_interval).await; - continue; - } + let addr = SocketAddr::from(([127, 0, 0, 1], self.config.p2p_port)); + let current_p2p_ready = tokio::net::TcpStream::connect(addr).await.is_ok(); + if current_p2p_ready && !p2p_ready { + tracing::debug!("dashd P2P port accepting connections"); } - - match self.try_rpc_client_base() { - Ok(client) => { - cookie_seen = true; - match client.get_blockchain_info() { - Ok(_) => return Ok(()), - Err(e) => { - last_rpc_error = format!("getblockchaininfo: {e}"); - tracing::debug!("RPC not ready yet: {e}"); + p2p_ready = current_p2p_ready; + + let cookie_path = self.rpc_cookie_path(); + let current_cookie_seen = cookie_path.exists(); + cookie_seen |= current_cookie_seen; + if current_cookie_seen { + let rpc_port = self.config.rpc_port; + match task::spawn_blocking(move || { + probe_blockchain_ready(cookie_path, rpc_port) + }) + .await + { + Ok(Ok(())) => { + rpc_ready = true; + last_rpc_error = "RPC ready".to_string(); + last_blockchain_error = "blockchain ready".to_string(); + if p2p_ready { + return Ok(()); } } - } - Err(e) => { - if self.rpc_cookie_path().exists() { - cookie_seen = true; + Ok(Err(e)) => { + rpc_ready = false; + last_rpc_error = e.clone(); + last_blockchain_error = e; + tracing::debug!("RPC/blockchain not ready yet: {last_rpc_error}"); + } + Err(e) => { + rpc_ready = false; + last_rpc_error = format!("readiness task failed: {e}"); + last_blockchain_error = last_rpc_error.clone(); + tracing::debug!("RPC readiness task failed: {e}"); } - last_rpc_error = e; - tracing::debug!("RPC client not ready yet: {last_rpc_error}"); } + } else { + rpc_ready = false; + last_rpc_error = "RPC cookie file not created yet".to_string(); + tracing::debug!("RPC client not ready yet: {last_rpc_error}"); } sleep(check_interval).await; } @@ -305,7 +344,8 @@ impl DashCoreNode { Ok(Err(e)) => Err(e), Err(_) => Err(format!( "dashd failed to become ready within {}s \ - (p2p_ready={p2p_ready}, cookie_seen={cookie_seen}, last_rpc_error={last_rpc_error})", + (p2p_ready={p2p_ready}, cookie_seen={cookie_seen}, rpc_ready={rpc_ready}, \ + last_rpc_error={last_rpc_error}, last_blockchain_error={last_blockchain_error})", max_wait.as_secs() )), } @@ -669,19 +709,47 @@ impl DashCoreNode { pub fn rpc_port(&self) -> u16 { self.config.rpc_port } + + pub(super) fn stop_and_wait(&mut self) { + let Some(mut process) = self.process.take() else { + return; + }; + + tracing::info!("Stopping dashd process..."); + if let Err(e) = process.start_kill() { + tracing::warn!("Failed to request dashd shutdown: {}", e); + } + loop { + match process.try_wait() { + Ok(Some(status)) => { + tracing::info!("dashd process exited with {}", status); + break; + } + Ok(None) => { + std::thread::sleep(Duration::from_millis(50)); + } + Err(e) => { + tracing::warn!("Failed to wait for dashd process exit: {}", e); + break; + } + } + } + } } impl Drop for DashCoreNode { fn drop(&mut self) { - if let Some(mut process) = self.process.take() { - tracing::info!("Stopping dashd process in Drop..."); - if let Err(e) = process.start_kill() { - tracing::warn!("Failed to kill dashd process: {}", e); - } - } + self.stop_and_wait(); } } +fn probe_blockchain_ready(cookie_path: PathBuf, rpc_port: u16) -> Result<(), String> { + let url = format!("http://127.0.0.1:{rpc_port}"); + let client = Client::new(&url, Auth::CookieFile(cookie_path)) + .map_err(|e| format!("cookie auth: {e}"))?; + client.get_blockchain_info().map(|_| ()).map_err(|e| format!("getblockchaininfo: {e}")) +} + /// Wallet file structure for test wallets. #[derive(Debug, Deserialize)] pub struct WalletFile { @@ -747,11 +815,12 @@ pub(crate) fn wallet_does_not_exist(err: &dashcore_rpc::Error) -> bool { match rpc_error_parts(err) { Some((code, msg)) => { let lower = msg.to_ascii_lowercase(); - // Require a wallet-related phrase so unrelated "not found" / code - // -18 responses never authorize createwallet. + // Require RPC_WALLET_NOT_FOUND (-18) and wallet-missing wording so + // unrelated nonzero codes (e.g. -32601 "Wallet method not found") + // never authorize createwallet. let walletish = lower.contains("wallet"); let missing = lower.contains("not found") || lower.contains("does not exist"); - walletish && missing && (code == RPC_WALLET_NOT_FOUND || code != 0) + code == RPC_WALLET_NOT_FOUND && walletish && missing } None => false, } @@ -851,6 +920,15 @@ mod tests { assert!(!wallet_does_not_exist(&method)); } + #[test] + fn wallet_not_found_requires_rpc_wallet_not_found_code() { + let err = rpc_err(RPC_WALLET_NOT_FOUND, "Wallet file does not exist."); + assert!(wallet_does_not_exist(&err)); + + let wrong_code = rpc_err(-32601, "Wallet file not found."); + assert!(!wallet_does_not_exist(&wrong_code)); + } + #[test] fn classifies_wallet_already_loaded() { let err = rpc_err(RPC_WALLET_ALREADY_LOADED, "Wallet already loaded."); @@ -893,7 +971,7 @@ mod tests { fs::write(regtest.join(".walletlock"), b"").unwrap(); fs::write(wallet.join(".walletlock"), b"").unwrap(); - clear_stale_runtime_locks(tmp.path()); + clear_stale_runtime_locks(tmp.path()).unwrap(); assert!(!regtest.join(".lock").exists()); assert!(!regtest.join(".walletlock").exists());