Skip to content

fix(dash-spv): preserve already defined change addr in the tx builder #920

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

Open
ZocoLini wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(dash-spv): preserve already defined change addr in the tx builder #920

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
36 changes: 35 additions & 1 deletion key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ impl TransactionBuilder {
/// must therefore not be held across an `await` between `set_funding` and
/// `build_signed` or `assemble_unsigned`, since suspending there reopens the
/// read-then-reserve window for a concurrent build.
///
/// A change address already set by [`set_change_address`](Self::set_change_address)
/// wins: the funding account is only asked for one when the builder has none.
/// Deriving unconditionally would both discard the caller's choice and burn a
/// pool address, since `next_change_address` advances the pool state.
pub fn set_funding(mut self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self {
let reserved = funds_acc.reservations().reserved(self.current_height);
self.inputs = funds_acc
Expand All @@ -119,7 +124,9 @@ impl TransactionBuilder {
.cloned()
.collect();
self.reservations = Some(funds_acc.reservations().clone());
self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok();
if self.change_addr.is_none() {
self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok();
}
Comment on lines +127 to +129

@coderabbitai coderabbitai Bot Aug 3, 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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect builder chains and address-pool mutations.
rg -n -C 8 'set_funding|set_change_address|next_change_address' \
  key-wallet/src --glob '*.rs'

Repository: dashpay/rust-dashcore

Length of output: 50378

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Map the transaction_builder source and inspect relevant builder implementation plus tests.
wc -l key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '1,170p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '1210,1305p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

# Find all TransactionBuilder chain calls in key-wallet source without tests/context noise.
printf '\n--- builder chains in key-wallet src ---\n'
python3 - <<'PY'
from pathlib import Path
import re
root = Path('key-wallet/src')
pairs = set()
for path in root.rglob('*.rs'):
    text = path.read_text(errors='replace')
    for m in re.finditer(r'TransactionBuilder::new\(\)', text):
        start = m.start()
        line_start = text.rfind('\n', 0, start) + 1
        line_end = text.find('\n', start + 2000)
        text = text if line_end < 0 else text[:line_end] + text[line_end+1:]
        chain = text[start:start+2000]
        h1 = 'set_funding' in chain
        h2 = 'set_change_address' in chain
        if h1 or h2:
            # capture minimal surrounding block up to next semicolon/new call-ish boundary
            out = []
            for ch in chain:
                out.append(ch)
                if ch == '.':
                    break
            out.append('...')
            pairs.add((int(text[:start].count('\n')+1), path.as_posix(), out[:200]))
            # stop at first full builder chain block
            break
print('\n'.join(f"{ln:5} {path} {chain}" for ln, path, chain in sorted(pairs)))
PY

Repository: dashpay/rust-dashcore

Length of output: 11046

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- all TransactionBuilder builder chains in key-wallet/src ---'
python3 - <<'PY'
import re
from pathlib import Path
for path in Path('key-wallet/src').rglob('*.rs'):
    text = path.read_text(errors='replace')
    for m in re.finditer(r'TransactionBuilder::new\(\)', text):
        line = text[:m.start()].count('\n') + 1
        end = min(len(text), m.start() + 2000)
        chunk = text[m.start():end]
        if 'set_funding' in chunk or 'set_change_address' in chunk:
            print(f'{path}:{line}:')
            for i, block in enumerate(chunk.split('\n'), 1):
                print(f'  {block}')
            break
PY

printf '%s\n' '--- ManagedCoreFundsAccount next_change_address definitions/usages ---'
rg -n -C 5 'fn next_change_address|next_receive_address|advance.*change|change_addr|ChangeAddresses' key-wallet/src --glob '*.rs' | head -n 220

printf '%s\n' '--- focused builder chain occurrences ---'
rg -n -C 3 'set_funding|set_change_address' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

Repository: dashpay/rust-dashcore

Length of output: 35390

Preserve next_change_address calls from advancing the change pool.

set_funding calls funds_acc.next_change_address(..., true), but later .set_change_address(...) can replace change_addr. Existing chains use this order, including production calls in TransactionBuilder and build_and_sign_transaction_with_signer, so one change address can be consumed and discarded. Defer change derivation until the build path has the final address, or make the setter order explicit and add a regression covering .set_funding(...).set_change_address(...).build_unsigned().

🤖 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_builder.rs` around
lines 127 - 129, Update TransactionBuilder::set_funding so it does not call
next_change_address or advance the change pool when initializing change_addr;
defer derivation until the build path has the final address, or otherwise ensure
set_change_address explicitly prevents and replaces any derived address.
Preserve the existing behavior for callers that do not provide a change address,
and add regression coverage for
set_funding(...).set_change_address(...).build_unsigned().

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Locate the account network source and address-validation helpers.
rg -n -C 8 -P 'struct\s+ManagedCoreFundsAccount|network|is_valid_for_network|Network::' \
  key-wallet/src/managed_account key-wallet/src/wallet --glob '*.rs'

# Confirm the networks used by the regression fixture.
rg -n -C 6 -P 'dummy_bip44|Address::dummy\(Network::(Testnet|Regtest)' \
  key-wallet/src --glob '*.rs'

Repository: dashpay/rust-dashcore

Length of output: 50378

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect the builder state initialization, network source methods, change address handling,
# and the specific regression test around lines 127 and 1218-1232.
sed -n '1,180p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '118,200p' key-wallet/src/managed_account/managed_funds_account.rs
sed -n '55,95p' key-wallet/src/managed_account/managed_account_ref.rs
sed -n '1215,1235p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

# Find address validation helpers and error text related to network/address validation.
rg -n -C 4 'fn .*network|is_.+_network|NetworkMismatch|invalid.*network|invalid_address|validate.*address' key-wallet/src --glob '*.rs'

Repository: dashpay/rust-dashcore

Length of output: 7193

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Locate the managed funds account source and test for address network validation helpers.
fd -a '.*managed.*funds.*\.rs|.*account.*ref.*\.rs|.*funding.*account.*\.rs' key-wallet/src || true
rg -n -C 8 'ManagedCoreFundsAccount|dummy_bip44|fn network\(|is_.*network|validate_|Address::dummy\(Network::Regtest|set_change_address' key-wallet/src --glob '*.rs' | sed -n '1,220p'

# Inspect the exact regression test.
sed -n '1215,1235p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

# Locate where transaction builder obtains the funds account/network during build.
rg -n -C 6 'fn build_signed|fn build_unsigned|ManagedCoreFundsAccount|funding_account|set_funding|next_change_address|change_addr' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

Repository: dashpay/rust-dashcore

Length of output: 41463

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect the managed funds account implementation and network helpers.
sed -n '1,220p' key-wallet/src/managed_account/managed_core_funds_account.rs
sed -n '1,180p' key-wallet/src/managed_account/managed_account_ref.rs

# Search narrowly for network validation helpers in managed account types.
rg -n -C 4 'fn network|network\s*:\s*Network|is_.*network|is_valid_for_network|NetworkMismatch|invalid.*network|Address::dummy\(Network::Regtest' key-wallet/src/managed_account --glob '*.rs'

# Search narrowly for error kinds that the transaction builder should use.
rg -n -C 3 'enum BuilderError|BuilderError::|Change|change_addr|Network' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs ke

Repository: dashpay/rust-dashcore

Length of output: 50377

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Locate the transaction builder error enum and builder construction/entrypoints to see where funding/account network can be compared.
rg -n -C 5 'enum BuilderError|NetworkMismatch|Invalid.*Network|change_addr|set_funding\(.*Account|set_funding\(&mut funds_acc' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

# Locate ManagedCoreFundsAccount constructor methods to confirm the dummy fixture network.
rg -n -C 6 'impl ManagedCoreFundsAccount|pub fn dummy_bip44|managed_account_type\(\)\.into\(\)|Network::Regtest|Network::Testnet' key-wallet/src/managed_account/managed_core_funds_account.rs

# Locate Account network source.
rg -n -C 4 'struct Account|impl AccountTrait for Account|fn network\(&self\)|network:' key-wallet/src/account --glob '*.rs'

Repository: dashpay/rust-dashcore

Length of output: 34445

Reject a cross-network explicit change address in set_change_address.

set_funding only preserves an already-set change_addr, while assemble_unsigned later emits change_addr.script_pubkey() without validating network membership. In the explicit-address path, builder.network() is unavailable, but set_funding can compare the provided change address against funds_acc.network()/acc.network and add a BuilderError variant for mismatch.

Also update set_funding_keeps_an_explicit_change_address to use the fixture network or construct a matching address explicitly, and add a regression test case that rejects a mismatched explicit change address.

🤖 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_builder.rs` around
lines 127 - 129, Validate explicit change addresses in set_funding by comparing
the address network with funds_acc.network()/acc.network before preserving
change_addr, and return a new BuilderError variant on mismatch. Keep matching
explicit addresses unchanged, update
set_funding_keeps_an_explicit_change_address to use a matching-network
fixture/address, and add a regression test covering rejection of a cross-network
address.

Source: Coding guidelines

self
}

Expand Down Expand Up @@ -1206,6 +1213,33 @@ mod tests {
assert!(candidates.contains(&free.outpoint));
}

#[test]
fn set_funding_keeps_an_explicit_change_address() {
let ctx = TestWalletContext::new_random();
let account =
ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone();

let mut funds = ManagedCoreFundsAccount::dummy_bip44();
let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true);
funds.utxos.insert(utxo.outpoint, utxo);

let explicit = Address::dummy(Network::Testnet, 1);
let builder = TransactionBuilder::new()
.set_current_height(200)
.set_change_address(explicit.clone())
.set_funding(&mut funds, &account);

assert_eq!(builder.change_addr.as_ref(), Some(&explicit));

// Nor was a pool address burned to produce one that would be thrown away:
// `funds` still hands out the same address a pristine account would.
let mut control = ManagedCoreFundsAccount::dummy_bip44();
assert_eq!(
funds.next_change_address(Some(&account.account_xpub), true).expect("change address"),
control.next_change_address(Some(&account.account_xpub), true).expect("change address"),
);
}

#[tokio::test]
async fn build_signed_releases_reservation_on_signing_failure() {
let ctx = TestWalletContext::new_random();
Expand Down
Loading