Skip to content

fix(key-wallet): skip already used accounts when funding a transaction #930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZocoLini wants to merge 1 commit into from
+40 −5
Closed

fix(key-wallet): skip already used accounts when funding a transaction #930

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 40 additions & 5 deletions key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ use crate::wallet::ManagedWalletInfo;
use crate::{DerivationPath, Wallet};
use dashcore::address::NetworkUnchecked;
use dashcore::{Address, Transaction};
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};

/// Account type preference for transaction building
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum AccountTypePreference {
BIP44,
BIP32,
Expand Down Expand Up @@ -162,9 +162,15 @@ impl ManagedWalletInfo {
};

let mut paths = HashMap::new();
let mut funded = 0usize;
let mut funded: HashSet<AccountTypePreference> = HashSet::new();

for &preference in preferences {
// Funding one account twice would offer coin selection every one of
// its UTXOs twice, letting it spend a single output as two inputs.
if funded.contains(&preference) {
continue;
}

let account = match preference {
AccountTypePreference::BIP44 => wallet.get_bip44_account(source_index),
AccountTypePreference::BIP32 => wallet.get_bip32_account(source_index),
Expand Down Expand Up @@ -197,10 +203,10 @@ impl ManagedWalletInfo {
}
}
builder = builder.add_funding(managed_account, account);
funded += 1;
funded.insert(preference);
}

if funded == 0 {
if funded.is_empty() {
return Err(BuilderError::AccountNotFound(format!(
"no funding account of any type at index {source_index}"
)));
Expand Down Expand Up @@ -887,6 +893,35 @@ mod tests {
outpoint
}

/// Naming one account twice must not offer its UTXOs twice: a 400k target
/// is not met by a single 300k coin, however often its account is named.
#[tokio::test]
async fn naming_an_account_twice_does_not_double_its_funds() {
let (wallet, mut info) = test_wallet_and_info();
fund(&wallet, &mut info, AccountTypePreference::BIP44, 0x11);
info.update_last_processed_height(1100);

let result = info
.build_and_sign_transaction(
&wallet,
&[AccountTypePreference::BIP44, AccountTypePreference::BIP44],
0,
dest_outputs(400_000),
FeeRate::normal(),
SelectionStrategy::BranchAndBound,
)
.await;

assert!(
matches!(
result,
Err(BuilderError::InsufficientFunds { .. }) | Err(BuilderError::CoinSelection(_))
),
"300k must not cover a 400k target, got: {:?}",
result.map(|(tx, _)| tx.input.len())
);
Comment on lines +915 to +922

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Avoid moving result before building the diagnostic.

matches!(result, ...) consumes the non-Copy Result. The later result.map(...) use then causes a compile error. Compute the input count before the assertion, or borrow result in the match.

Proposed fix 
+        let input_count = result.as_ref().map(|(tx, _)| tx.input.len());
         assert!(
             matches!(
                 result,
                 Err(BuilderError::InsufficientFunds { .. }) | Err(BuilderError::CoinSelection(_))
             ),
             "300k must not cover a 400k target, got: {:?}",
-            result.map(|(tx, _)| tx.input.len())
+            input_count
         );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert!(
matches!(
result,
Err(BuilderError::InsufficientFunds { .. }) | Err(BuilderError::CoinSelection(_))
),
"300k must not cover a 400k target, got: {:?}",
result.map(|(tx, _)| tx.input.len())
);
let input_count = result.as_ref().map(|(tx, _)| tx.input.len());
assert!(
matches!(
result,
Err(BuilderError::InsufficientFunds { .. }) | Err(BuilderError::CoinSelection(_))
),
"300k must not cover a 400k target, got: {:?}",
input_count
);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs` around
lines 915 - 922, Update the assertion around result to avoid consuming the
non-Copy Result before constructing its diagnostic: borrow result in matches!,
or compute the input count before the assertion and reuse it in the message.
Preserve the existing insufficient-funds/coin-selection validation and
diagnostic output.

}

/// Neither account covers the 500k target on its own, so the build only
/// succeeds by pooling both — and signing them proves the derivation paths
/// were collected across both accounts.
Expand Down
Loading