Skip to content

TxBuilder should not change order of insertion #261

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
nymius wants to merge 2 commits into from
Closed

TxBuilder should not change order of insertion #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3555ccf
fix(tx_builder): preserve insertion order with TxOrdering::Untouched
nymius Jun 9, 2025
0e97bf5
doc(tx_builder): add info about manually selected UTxOs priority
nymius Jun 9, 2025
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
96 changes: 56 additions & 40 deletions wallet/src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1511,7 +1511,11 @@ impl Wallet {

let (required_utxos, optional_utxos) = {
// NOTE: manual selection overrides unspendable
let mut required: Vec<WeightedUtxo> = params.utxos.values().cloned().collect();
let mut required_to_sort: Vec<(u32, WeightedUtxo)> =
params.utxos.values().cloned().collect();
required_to_sort.sort_by_key(|(k, _v)| *k);
let mut required: Vec<WeightedUtxo> =
required_to_sort.into_iter().map(|(_k, v)| v).collect();
let optional = self.filter_utxos(&params, current_height.to_consensus_u32());

// if drain_wallet is true, all UTxOs are required
Expand Down Expand Up @@ -1720,7 +1724,8 @@ impl Wallet {
let utxos = tx
.input
.drain(..)
.map(|txin| -> Result<_, BuildFeeBumpError> {
.zip(0u32..)
.map(|(txin, order)| -> Result<_, BuildFeeBumpError> {
graph
// Get previous transaction
.get_tx(txin.previous_output.txid)
Expand All @@ -1736,26 +1741,29 @@ impl Wallet {
.get(txin.previous_output.vout as usize)
.ok_or(BuildFeeBumpError::InvalidOutputIndex(txin.previous_output))
.cloned()?;
Ok((prev_tx, prev_txout, chain_position))
Ok((prev_tx, prev_txout, chain_position, order))
})
.map(|(prev_tx, prev_txout, chain_position)| {
.map(|(prev_tx, prev_txout, chain_position, order)| {
match txout_index.index_of_spk(prev_txout.script_pubkey.clone()) {
Some(&(keychain, derivation_index)) => (
txin.previous_output,
WeightedUtxo {
satisfaction_weight: self
.public_descriptor(keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(LocalOutput {
outpoint: txin.previous_output,
txout: prev_txout.clone(),
keychain,
is_spent: true,
derivation_index,
chain_position,
}),
},
(
order,
WeightedUtxo {
satisfaction_weight: self
.public_descriptor(keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(LocalOutput {
outpoint: txin.previous_output,
txout: prev_txout.clone(),
keychain,
is_spent: true,
derivation_index,
chain_position,
}),
},
),
),
None => {
let satisfaction_weight = Weight::from_wu_usize(
Expand All @@ -1765,27 +1773,32 @@ impl Wallet {

(
txin.previous_output,
WeightedUtxo {
utxo: Utxo::Foreign {
outpoint: txin.previous_output,
sequence: txin.sequence,
psbt_input: Box::new(psbt::Input {
witness_utxo: prev_txout
.script_pubkey
.witness_version()
.map(|_| prev_txout.clone()),
non_witness_utxo: Some(prev_tx.as_ref().clone()),
..Default::default()
}),
(
order,
WeightedUtxo {
utxo: Utxo::Foreign {
outpoint: txin.previous_output,
sequence: txin.sequence,
psbt_input: Box::new(psbt::Input {
witness_utxo: prev_txout
.script_pubkey
.witness_version()
.map(|_| prev_txout.clone()),
non_witness_utxo: Some(
prev_tx.as_ref().clone(),
),
..Default::default()
}),
},
satisfaction_weight,
},
satisfaction_weight,
},
),
)
}
}
})
})
.collect::<Result<HashMap<OutPoint, WeightedUtxo>, BuildFeeBumpError>>()?;
.collect::<Result<HashMap<OutPoint, (u32, WeightedUtxo)>, BuildFeeBumpError>>()?;

if tx.output.len() > 1 {
let mut change_index = None;
Expand Down Expand Up @@ -2749,13 +2762,16 @@ mod test {
let output = wallet.get_utxo(OutPoint { txid, vout: 0 }).unwrap();
params.utxos.insert(
output.outpoint,
WeightedUtxo {
satisfaction_weight: wallet
.public_descriptor(output.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(output),
},
(
0,
WeightedUtxo {
satisfaction_weight: wallet
.public_descriptor(output.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(output),
},
),
);
// enforce selection of first output in transaction
let received = wallet.filter_utxos(&params, wallet.latest_checkpoint().block_id().height);
Expand Down
137 changes: 94 additions & 43 deletions wallet/src/wallet/tx_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ pub(crate) struct TxParams {
pub(crate) fee_policy: Option<FeePolicy>,
pub(crate) internal_policy_path: Option<BTreeMap<String, Vec<usize>>>,
pub(crate) external_policy_path: Option<BTreeMap<String, Vec<usize>>>,
pub(crate) utxos: HashMap<OutPoint, WeightedUtxo>,
pub(crate) utxos: HashMap<OutPoint, (u32, WeightedUtxo)>,
pub(crate) unspendable: HashSet<OutPoint>,
pub(crate) manually_selected_only: bool,
pub(crate) sighash: Option<psbt::PsbtSighashType>,
Expand Down Expand Up @@ -276,28 +276,41 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
///
/// These have priority over the "unspendable" utxos, meaning that if a utxo is present both in
/// the "utxos" and the "unspendable" list, it will be spent.
///
/// If a UTxO is inserted multiple times, only the final insertion will take effect.
pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> {
let order_max = self
.params
.utxos
.values()
.map(|(order, _)| order)
.max()
.unwrap_or(&0);
let utxo_batch = outpoints
.iter()
.map(|outpoint| {
.zip(1u32..)
.map(|(outpoint, counter)| {
self.wallet
.get_utxo(*outpoint)
.ok_or(AddUtxoError::UnknownUtxo(*outpoint))
.map(|output| {
(
*outpoint,
WeightedUtxo {
satisfaction_weight: self
.wallet
.public_descriptor(output.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(output),
},
(
order_max + counter,
WeightedUtxo {
satisfaction_weight: self
.wallet
.public_descriptor(output.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(output),
},
),
)
})
})
.collect::<Result<HashMap<OutPoint, WeightedUtxo>, AddUtxoError>>()?;
.collect::<Result<HashMap<OutPoint, (u32, WeightedUtxo)>, AddUtxoError>>()?;
self.params.utxos.extend(utxo_batch);

Ok(self)
Expand All @@ -313,6 +326,10 @@ impl<'a, Cs> TxBuilder<'a, Cs> {

/// Add a foreign UTXO i.e. a UTXO not known by this wallet.
///
/// Foreign UTxOs do not take priority over local UTxOs. If a local UTxO is added to the
/// manually selected list, it will replace any conflicting foreign UTxOs. However, a foreign
/// UTxO cannot replace a conflicting local UTxO.
///
/// There might be cases where the UTxO belongs to the wallet but it doesn't have knowledge of
/// it. This is possible if the wallet is not synced or its not being use to track
/// transactions. In those cases is the responsibility of the user to add any possible local
Expand Down Expand Up @@ -407,23 +424,36 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
}
}

if let Some(WeightedUtxo {
utxo: Utxo::Local { .. },
..
}) = self.params.utxos.get(&outpoint)
if let Some((
_order,
WeightedUtxo {
utxo: Utxo::Local { .. },
..
},
)) = self.params.utxos.get(&outpoint)
{
None
} else {
let order_max = self
.params
.utxos
.values()
.map(|(order, _)| order)
.max()
.unwrap_or(&0);
self.params.utxos.insert(
outpoint,
WeightedUtxo {
satisfaction_weight,
utxo: Utxo::Foreign {
outpoint,
sequence,
psbt_input: Box::new(psbt_input),
(
order_max + 1,
WeightedUtxo {
satisfaction_weight,
utxo: Utxo::Foreign {
outpoint,
sequence,
psbt_input: Box::new(psbt_input),
},
},
},
),
)
};

Expand Down Expand Up @@ -468,6 +498,11 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
}

/// Choose the ordering for inputs and outputs of the transaction
///
/// When [TxBuilder::ordering] is set to [TxOrdering::Untouched], the insertion order of
/// recipients and manually selected UTxOs is preserved and reflected exactly in transaction's
/// output and input vectors respectively. If algorithmically selected UTxOs are included, they
/// will be placed after all the manually selected ones in the transaction's input vector.
pub fn ordering(&mut self, ordering: TxOrdering) -> &mut Self {
self.params.ordering = ordering;
self
Expand Down Expand Up @@ -777,7 +812,11 @@ pub enum TxOrdering {
/// Randomized (default)
#[default]
Shuffle,
/// Unchanged
/// Unchanged insertion order for recipients and for manually added UTxOs. This guarantees all
/// recipients preserve insertion order in the transaction's output vector and manually added
/// UTxOs preserve insertion order in the transaction's input vector, but does not make any
/// guarantees about algorithmically selected UTxOs. However, by design they will always be
/// placed after the manually selected ones.
Untouched,
/// Provide custom comparison functions for sorting
Custom {
Expand Down Expand Up @@ -1142,14 +1181,18 @@ mod test {
satisfaction_weight: Weight::from_wu(0),
utxo: Utxo::Local(test_utxos[0].clone()),
};
for _ in 0..3 {
for order in 0..3 {
params
.utxos
.insert(test_utxos[0].outpoint, fake_weighted_utxo.clone());
.insert(test_utxos[0].outpoint, (order, fake_weighted_utxo.clone()));
}
assert_eq!(
vec![(test_utxos[0].outpoint, fake_weighted_utxo)],
params.utxos.into_iter().collect::<Vec<_>>()
params
.utxos
.into_iter()
.map(|(k, (_order, v))| (k, v))
.collect::<Vec<_>>()
);
}

Expand Down Expand Up @@ -1202,7 +1245,7 @@ mod test {
)
.is_ok());

let foreign_utxo_with_modified_weight =
let (_order, foreign_utxo_with_modified_weight) =
builder.params.utxos.values().collect::<Vec<_>>()[0];

assert_eq!(builder.params.utxos.len(), 1);
Expand Down Expand Up @@ -1250,13 +1293,16 @@ mod test {
// add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet
builder.params.utxos.insert(
utxo1.outpoint,
WeightedUtxo {
satisfaction_weight: wallet1
.public_descriptor(utxo1.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(utxo1.clone()),
},
(
0,
WeightedUtxo {
satisfaction_weight: wallet1
.public_descriptor(utxo1.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(utxo1.clone()),
},
),
);

// add foreign utxo
Expand All @@ -1271,7 +1317,8 @@ mod test {
)
.is_ok());

let utxo_should_still_be_local = builder.params.utxos.values().collect::<Vec<_>>()[0];
let (_order, utxo_should_still_be_local) =
builder.params.utxos.values().collect::<Vec<_>>()[0];

assert_eq!(builder.params.utxos.len(), 1);
assert_eq!(utxo_should_still_be_local.utxo.outpoint(), utxo1.outpoint);
Expand Down Expand Up @@ -1335,16 +1382,20 @@ mod test {
// add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet
builder.params.utxos.insert(
utxo1.outpoint,
WeightedUtxo {
satisfaction_weight: wallet1
.public_descriptor(utxo1.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(utxo1.clone()),
},
(
0,
WeightedUtxo {
satisfaction_weight: wallet1
.public_descriptor(utxo1.keychain)
.max_weight_to_satisfy()
.unwrap(),
utxo: Utxo::Local(utxo1.clone()),
},
),
);

let utxo_should_still_be_local = builder.params.utxos.values().collect::<Vec<_>>()[0];
let (_order, utxo_should_still_be_local) =
builder.params.utxos.values().collect::<Vec<_>>()[0];

assert_eq!(builder.params.utxos.len(), 1);
assert_eq!(utxo_should_still_be_local.utxo.outpoint(), utxo1.outpoint);
Expand Down
Loading
Loading