Skip to content
Merged
Changes from 5 commits
Commits
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
264 changes: 263 additions & 1 deletion key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ pub struct TransactionBuilder {
/// account that holds the UTXO, so each account reserves its own share of
/// the chosen inputs — all under the one token this build is stamped with.
funding: Vec<(ReservationSet, HashSet<OutPoint>)>,
/// When set, `add_funding` contributes no candidates of its own — see
/// [`Self::use_only_added_inputs`].
only_added_inputs: bool,
/// Outpoints supplied through [`Self::add_inputs`]. Kept so the restriction
/// can be applied whatever order the caller builds in.
seeded_inputs: HashSet<OutPoint>,
}

impl Default for TransactionBuilder {
Expand All @@ -119,6 +125,8 @@ impl TransactionBuilder {
special_payload: None,
payload_finalizer: None,
funding: Vec::new(),
only_added_inputs: false,
seeded_inputs: HashSet::new(),
}
}

Expand Down Expand Up @@ -199,7 +207,37 @@ impl TransactionBuilder {
}

pub fn add_inputs(mut self, inputs: impl IntoIterator<Item = Utxo>) -> Self {
self.inputs.extend(inputs);
for utxo in inputs {
self.seeded_inputs.insert(utxo.outpoint);
self.inputs.push(utxo);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
self
}

/// Restrict coin selection to the inputs [`Self::add_inputs`] supplied:
/// `add_funding` keeps doing its reservation bookkeeping and still supplies
/// the change address, but contributes no candidates of its own.
///
/// Without this, `add_funding` unions the funding account's entire
/// unreserved UTXO set into the candidate pool, and
/// [`SelectionStrategy::All`] then takes all of it. A caller that splits a
/// large account into batches of at most `MAX_STANDARD_TX_INPUTS` and
/// seeds one batch per build therefore has no effect at all: every build
/// sees the whole account and fails with [`BuilderError::TooManyInputs`],
/// so an account above the cap can never be drained. That is exactly what a
/// chunked CoinJoin sweep does.
///
/// Reservation bookkeeping is unchanged: `owned` still covers every
/// unreserved UTXO of the account, so whichever seeded outpoints selection
/// picks are reserved by the account that holds them.
///
/// Independent of call order: the restriction is applied immediately before
/// coin selection, so it does not matter when this is called relative to
/// `add_inputs` and `add_funding` — every ordering builds the same
/// transaction. Seeded outpoints another in-flight build has reserved are
/// dropped there too.
pub fn use_only_added_inputs(mut self) -> Self {
self.only_added_inputs = true;
self
}

Expand Down Expand Up @@ -523,6 +561,32 @@ impl TransactionBuilder {
self.inputs.retain(|utxo| utxo.is_confirmed || utxo.is_instantlocked);
}

if self.only_added_inputs {
// Applied here rather than where the option is set, so no call
// order can slip a candidate past it: `add_inputs` may run after
// `use_only_added_inputs`, and `add_funding` either side of it.
//
// Three things at once: drop what `add_funding` contributed, drop a
// seeded outpoint another in-flight build has reserved (`add_inputs`
// does not consult the reservation set, while every candidate
// `add_funding` offers is unreserved), and collapse an outpoint both
// supplied to one candidate — coin selection does not deduplicate,
// so a second copy is spent twice and Core rejects the transaction.
let reserved: HashSet<OutPoint> = self
.funding
.iter()
.flat_map(|(reservations, _)| reservations.reserved(self.current_height))
.collect();
let seeded = core::mem::take(&mut self.seeded_inputs);
let mut kept: HashSet<OutPoint> = HashSet::new();
self.inputs.retain(|utxo| {
seeded.contains(&utxo.outpoint)
&& !reserved.contains(&utxo.outpoint)
&& kept.insert(utxo.outpoint)
});
self.seeded_inputs = seeded;
}

// Must match `calculate_base_size`, including the conservative VIN0 routing-script size.
let change_output_size = self.estimated_change_output_size();

Expand Down Expand Up @@ -1982,6 +2046,204 @@ mod tests {
assert!(candidates.contains(&free.outpoint));
}

#[test]
fn use_only_added_inputs_keeps_selection_to_the_seeded_batch() {
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 seeded = Utxo::dummy(0x01, 500_000, 100, false, true);
let other = Utxo::dummy(0x02, 500_000, 100, false, true);
funds.utxos.insert(seeded.outpoint, seeded.clone());
funds.utxos.insert(other.outpoint, other.clone());

let builder = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All)
.use_only_added_inputs()
.add_inputs(vec![seeded.clone()])
.add_funding(&mut funds, &account)
.add_output(&ctx.receive_address, 100_000);

let (tx, _fee, _token) = builder.build_unsigned_reserved().expect("build");
let prevouts: Vec<OutPoint> = tx.input.iter().map(|i| i.previous_output).collect();
assert_eq!(
prevouts,
vec![seeded.outpoint],
"add_funding must contribute nothing of its own, got {prevouts:?}"
);

// Reservation bookkeeping is unchanged: the account that owns the
// seeded input still reserves it.
assert!(funds.reservations().reserved(200).contains(&seeded.outpoint));
}

#[test]
fn add_inputs_after_add_funding_does_not_duplicate_a_candidate() {
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 shared = Utxo::dummy(0x01, 500_000, 100, false, true);
let other = Utxo::dummy(0x02, 500_000, 100, false, true);
funds.utxos.insert(shared.outpoint, shared.clone());
funds.utxos.insert(other.outpoint, other.clone());

// Funding first, then seeding the SAME outpoint: without dedup the pool
// holds it twice, and the opt-in keeps both copies.
let builder = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All)
.add_funding(&mut funds, &account)
.add_inputs(vec![shared.clone()])
.use_only_added_inputs()
.add_output(&ctx.receive_address, 100_000);

let (tx, _fee, _token) = builder.build_unsigned_reserved().expect("build");
let prevouts: Vec<OutPoint> = tx.input.iter().map(|i| i.previous_output).collect();
assert_eq!(
prevouts,
vec![shared.outpoint],
"the shared outpoint must be spent exactly once, got {prevouts:?}"
);
}

#[test]
fn only_added_inputs_is_independent_of_builder_call_order() {
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 seeded = Utxo::dummy(0x01, 500_000, 100, false, true);
let other = Utxo::dummy(0x02, 500_000, 100, false, true);
funds.utxos.insert(seeded.outpoint, seeded.clone());
funds.utxos.insert(other.outpoint, other.clone());

// The opt-in comes AFTER funding, so `add_funding` has already put the
// account's whole unreserved set into the candidate pool.
let builder = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All)
.add_inputs(vec![seeded.clone()])
.add_funding(&mut funds, &account)
.use_only_added_inputs()
.add_output(&ctx.receive_address, 100_000);

let (tx, _fee, _token) = builder.build_unsigned_reserved().expect("build");
let prevouts: Vec<OutPoint> = tx.input.iter().map(|i| i.previous_output).collect();
assert_eq!(
prevouts,
vec![seeded.outpoint],
"candidates an earlier add_funding added must be discarded, got {prevouts:?}"
);
}

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

// Both orders: seeding before the opt-in, and seeding after it — the
// second is what a call-order-sensitive filter would miss.
//
// A fresh account per case: `ReservationSet` has interior mutability, so
// cloning it would share the reservations one build stamps with the next.
for seeded_last in [false, true] {
let mut funds = ManagedCoreFundsAccount::dummy_bip44();
let free = Utxo::dummy(0x01, 500_000, 100, false, true);
let taken = Utxo::dummy(0x02, 500_000, 100, false, true);
funds.utxos.insert(free.outpoint, free.clone());
funds.utxos.insert(taken.outpoint, taken.clone());

// Another in-flight build already holds one of the outpoints the
// caller seeds. `add_inputs` does not consult the reservation set,
// so without the check this build would select it too.
funds.reservations().reserve(&[taken.outpoint], 200, ReservationToken::next());

let builder = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All);
let builder = if seeded_last {
builder
.add_funding(&mut funds, &account)
.use_only_added_inputs()
.add_inputs(vec![free.clone(), taken.clone()])
} else {
builder
.use_only_added_inputs()
.add_inputs(vec![free.clone(), taken.clone()])
.add_funding(&mut funds, &account)
};

let (tx, _fee, _token) = builder
.add_output(&ctx.receive_address, 100_000)
.build_unsigned_reserved()
.expect("build");
let prevouts: Vec<OutPoint> = tx.input.iter().map(|i| i.previous_output).collect();
assert_eq!(
prevouts,
vec![free.outpoint],
"a seeded input reserved by another build must be dropped \
(seeded_last = {seeded_last}), got {prevouts:?}"
);
}
}

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

// An account above MAX_STANDARD_TX_INPUTS, like a heavily mixed
// CoinJoin account: 589 UTXOs was the figure from ticket 32081.
// `Utxo::dummy` only varies the txid, which caps it at 256 distinct
// outpoints — vary the vout to get past the input limit.
let mut funds = ManagedCoreFundsAccount::dummy_bip44();
let unique: Vec<Utxo> = (0..589u32)
.map(|i| {
let mut utxo = Utxo::dummy((i / 256) as u8, 500_000, 100, false, true);
utxo.outpoint.vout = i;
utxo
})
.collect();
for utxo in &unique {
funds.utxos.insert(utxo.outpoint, utxo.clone());
}
assert_eq!(funds.utxos.len(), 589, "the fixture must exceed the cap");
let chunk: Vec<Utxo> = unique.iter().take(MAX_STANDARD_TX_INPUTS).cloned().collect();

// Without the opt-in the whole account is pulled in and the build dies
// on the cap, however small the seeded chunk is.
let unbounded = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All)
.add_inputs(chunk.clone())
.add_funding(&mut funds.clone(), &account)
.add_output(&ctx.receive_address, 100_000)
.build_unsigned_reserved();
assert!(
matches!(unbounded, Err(BuilderError::TooManyInputs { .. })),
"expected the unbounded build to hit the cap, got {unbounded:?}"
);

// With it, the seeded chunk is exactly what gets spent.
let (tx, _fee, _token) = TransactionBuilder::new()
.set_current_height(200)
.set_selection_strategy(SelectionStrategy::All)
.use_only_added_inputs()
.add_inputs(chunk.clone())
.add_funding(&mut funds, &account)
.add_output(&ctx.receive_address, 100_000)
.build_unsigned_reserved()
.expect("chunked drain builds");
assert_eq!(tx.input.len(), chunk.len(), "the chunk is spent whole and alone");
}

/// A UTXO seeded with `add_inputs` and then offered again by `add_funding`
/// must appear ONCE. Additive funding otherwise pushes a second candidate
/// for the same outpoint, and since coin selection does not deduplicate,
Expand Down
Loading