Skip to content

fix(P2-01): enforce do_not_spend on explicit UTXO selections #120

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

Merged
icota merged 1 commit into from
May 20, 2026
Merged

fix(P2-01): enforce do_not_spend on explicit UTXO selections #120

Show file tree
Hide file tree
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
16 changes: 10 additions & 6 deletions src/rbf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::account::NgAccount;
#[cfg(feature = "envoy")]
use crate::fee_rate::FeeRateSatPerKvb;
use crate::fee_rate::FeeRateSatPerKwu;
use crate::ngwallet::NgWallet;
use crate::rbf::BumpFeeError::ComposeTxError;
use crate::send::DraftTransaction;
use crate::fee_rate::FeeRateSatPerKwu;
#[cfg(feature = "envoy")]
use crate::fee_rate::FeeRateSatPerKvb;
#[cfg(feature = "envoy")]
use crate::send::TransactionFeeResult;
use crate::transaction::{BitcoinTransaction, Input, KeyChain, Output};
Expand Down Expand Up @@ -44,6 +44,7 @@ pub enum BumpFeeError {
FeeRateUnavailable,
UnableToAccessWallet,
UnableToAddForeignUtxo(AddForeignUtxoError),
LockedUtxoSelected(Vec<String>),
}

// TODO: chore: cleanup duplicate code
Expand Down Expand Up @@ -155,7 +156,8 @@ impl<P: WalletPersister> NgAccount<P> {
max_fee = Some(required.to_sat());
}
CreateTxError::FeeRateTooLow { required } => {
max_fee_rate = FeeRateSatPerKwu::from_bdk(required) + FeeRateSatPerKwu::from_sat_per_vb(1);
max_fee_rate = FeeRateSatPerKwu::from_bdk(required)
+ FeeRateSatPerKwu::from_sat_per_vb(1);
max_fee = None;
}
CoinSelection(error) => {
Expand Down Expand Up @@ -200,7 +202,8 @@ impl<P: WalletPersister> NgAccount<P> {
max_fee = Some(required.to_sat());
}
CreateTxError::FeeRateTooLow { required } => {
max_fee_rate = FeeRateSatPerKwu::from_bdk(required) + FeeRateSatPerKwu::from_sat_per_vb(1);
max_fee_rate = FeeRateSatPerKwu::from_bdk(required)
+ FeeRateSatPerKwu::from_sat_per_vb(1);
max_fee = None;
}
CoinSelection(error) => {
Expand Down Expand Up @@ -439,7 +442,8 @@ impl<P: WalletPersister> NgAccount<P> {
mature_utxos.clone(),
&mut do_not_spend_utxos,
&mut spendables,
);
)
.map_err(BumpFeeError::LockedUtxoSelected)?;

for do_not_spend_utxo in do_not_spend_utxos.clone() {
tx_builder.add_unspendable(do_not_spend_utxo.get_outpoint());
Expand Down
73 changes: 52 additions & 21 deletions src/send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ pub enum TransactionComposeError {
CreateTxError(CreateTxError),
WalletError(String),
Error(String),
LockedUtxoSelected(Vec<String>),
}

impl fmt::Display for TransactionComposeError {
Expand All @@ -76,6 +77,9 @@ impl fmt::Display for TransactionComposeError {
TransactionComposeError::CreateTxError(e) => write!(f, "CreateTxError: {e}"),
TransactionComposeError::WalletError(e) => write!(f, "WalletError: {e}"),
TransactionComposeError::Error(e) => write!(f, "Error: {e}"),
TransactionComposeError::LockedUtxoSelected(ids) => {
write!(f, "LockedUtxoSelected: {}", ids.join(", "))
}
}
}
}
Expand Down Expand Up @@ -115,7 +119,8 @@ impl<P: WalletPersister> NgAccount<P> {
utxos.clone(),
&mut do_not_spend_utxos,
&mut spendables,
);
)
.map_err(TransactionComposeError::LockedUtxoSelected)?;

if spendables.is_empty() {
return Err(TransactionComposeError::Error(
Expand Down Expand Up @@ -201,7 +206,8 @@ impl<P: WalletPersister> NgAccount<P> {
match psbt.clone().extract_tx_fee_rate_limit() {
Ok(..) => {
max_fee_rate = FeeRateSatPerKwu::from_bdk(
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
psbt.fee_rate()
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
);
if max_fee_rate < FeeRateSatPerKwu::from_sat_per_vb(1) {
max_fee_rate = FeeRateSatPerKwu::from_sat_per_vb(1);
Expand All @@ -215,13 +221,15 @@ impl<P: WalletPersister> NgAccount<P> {
}
ExtractTxError::MissingInputValue { .. } => {
max_fee_rate = FeeRateSatPerKwu::from_bdk(
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
psbt.fee_rate()
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
);
break;
}
ExtractTxError::SendingTooMuch { psbt } => {
max_fee_rate = FeeRateSatPerKwu::from_bdk(
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
psbt.fee_rate()
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
);
break;
}
Expand Down Expand Up @@ -335,7 +343,8 @@ impl<P: WalletPersister> NgAccount<P> {
utxos.clone(),
&mut do_not_spend_utxos,
&mut spendables,
);
)
.map_err(TransactionComposeError::LockedUtxoSelected)?;

let mut do_not_spend_amount = 0;

Expand Down Expand Up @@ -429,24 +438,44 @@ impl<P: WalletPersister> NgAccount<P> {
utxos: Vec<Output>,
do_not_spend_utxos: &mut Vec<Output>,
spendables: &mut Vec<Output>,
) {
for output in utxos {
//choose all output that are not selected by the user,
//this will create a pool of available utxo for tx builder.
for selected_outputs in selected_outputs.clone() {
if output.get_id() == selected_outputs.get_id() {
spendables.push(output.clone());
break;
}
}
if selected_outputs.is_empty() && !output.do_not_spend {
spendables.push(output.clone());
) -> Result<(), Vec<String>> {
let locked_utxo_ids: HashSet<String> = utxos
.iter()
.filter(|o| o.do_not_spend)
.map(|o| o.get_id())
.collect();

let mut locked_selected: Vec<String> = Vec::new();
for selected in &selected_outputs {
let id = selected.get_id();
if (selected.do_not_spend || locked_utxo_ids.contains(&id))
&& !locked_selected.contains(&id)
{
locked_selected.push(id);
}
}
if !locked_selected.is_empty() {
return Err(locked_selected);
}

let selected_ids: HashSet<String> = selected_outputs.iter().map(|o| o.get_id()).collect();
let has_explicit_selection = !selected_ids.is_empty();

if !spendables.contains(&output) {
do_not_spend_utxos.push(output.clone());
for output in utxos {
let is_selected = selected_ids.contains(&output.get_id());
if has_explicit_selection {
if is_selected {
spendables.push(output);
} else {
do_not_spend_utxos.push(output);
}
} else if output.do_not_spend {
do_not_spend_utxos.push(output);
} else {
spendables.push(output);
}
}
Ok(())
}

pub(crate) fn transform_psbt_to_bitcointx(
Expand Down Expand Up @@ -478,7 +507,8 @@ impl<P: WalletPersister> NgAccount<P> {
is_confirmed: false,
fee: psbt.fee().unwrap_or(Amount::from_sat(0)).to_sat(),
fee_rate: FeeRateSatPerKvb::from_bdk(
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
psbt.fee_rate()
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
),
amount,
inputs,
Expand Down Expand Up @@ -806,7 +836,8 @@ impl<P: WalletPersister> NgAccount<P> {
is_confirmed: false,
fee: psbt.fee().unwrap_or(Amount::from_sat(0)).to_sat(),
fee_rate: FeeRateSatPerKvb::from_bdk(
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
psbt.fee_rate()
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
),
amount: amount as i64,
inputs: vec![],
Expand Down
158 changes: 156 additions & 2 deletions tests/spend_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ mod spend_tests {
use crate::utils::tests_util;
use bdk_wallet::rusqlite::Connection;
use ngwallet::account::NgAccount;
use ngwallet::send::{DraftTransaction, FeeRateSatPerKvb, TransactionParams};
use ngwallet::rbf::BumpFeeError;
use ngwallet::send::{
DraftTransaction, FeeRateSatPerKvb, TransactionComposeError, TransactionParams,
};

use crate::utils::tests_util::get_ng_hot_wallet;

Expand All @@ -25,7 +28,7 @@ mod spend_tests {
};
let draft = account.get_max_fee(params.clone()).unwrap();
assert_eq!(draft.max_fee_rate, FeeRateSatPerKvb(553_828)); // 138_457 sat/kwu * 4 = sat/kvB
assert_eq!(draft.min_fee_rate, FeeRateSatPerKvb(1000)); // 1 sat/vB in sat/kvB
assert_eq!(draft.min_fee_rate, FeeRateSatPerKvb(1000)); // 1 sat/vB in sat/kvB
check_draft_tx_match_params(draft.draft_transaction.clone(), params.clone());
}

Expand Down Expand Up @@ -147,6 +150,157 @@ mod spend_tests {
assert_eq!(transaction.note, params.note);
assert_eq!(transaction.get_change_tag(), params.tag);
}

// Audit P2-01: a UTXO marked do_not_spend must never be spent, even when
// the caller passes it explicitly in `selected_outputs`. Both the send
// and RBF paths share this policy and must surface a dedicated error.

#[test]
fn test_compose_psbt_rejects_locked_selected_utxo() {
let mut account = get_ng_hot_wallet();
tests_util::add_funds_to_wallet(&mut account);

let locked = account.utxos().unwrap()[0].clone();
account
.set_do_not_spend(locked.get_id().as_str(), true)
.unwrap();
let locked_live = account
.utxos()
.unwrap()
.into_iter()
.find(|o| o.get_id() == locked.get_id())
.expect("locked utxo should still exist");
assert!(locked_live.do_not_spend);

let params = TransactionParams {
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
amount: 4000,
fee_rate: FeeRateSatPerKvb(2000),
selected_outputs: vec![locked_live.clone()],
note: None,
tag: None,
do_not_spend_change: false,
};
match account.compose_psbt(params) {
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
assert_eq!(ids, vec![locked_live.get_id()]);
}
other => panic!("expected LockedUtxoSelected, got {other:?}"),
}
}

// A caller cannot bypass the lock by zeroing `do_not_spend` on the
// Output values it passes in `selected_outputs`. The wallet's live UTXO
// set is the source of truth.
#[test]
fn test_compose_psbt_rejects_stale_unlocked_selected_utxo() {
let mut account = get_ng_hot_wallet();
tests_util::add_funds_to_wallet(&mut account);

let locked_id = {
let utxo = account.utxos().unwrap()[0].clone();
account
.set_do_not_spend(utxo.get_id().as_str(), true)
.unwrap();
utxo.get_id()
};

let mut stale = account
.utxos()
.unwrap()
.into_iter()
.find(|o| o.get_id() == locked_id)
.unwrap();
// Simulate a compromised caller forging do_not_spend=false.
stale.do_not_spend = false;

let params = TransactionParams {
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
amount: 4000,
fee_rate: FeeRateSatPerKvb(2000),
selected_outputs: vec![stale],
note: None,
tag: None,
do_not_spend_change: false,
};
match account.compose_psbt(params) {
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
assert_eq!(ids, vec![locked_id]);
}
other => panic!("expected LockedUtxoSelected, got {other:?}"),
}
}

#[test]
fn test_get_max_fee_rejects_locked_selected_utxo() {
let mut account = get_ng_hot_wallet();
tests_util::add_funds_to_wallet(&mut account);

let locked = account.utxos().unwrap()[0].clone();
account
.set_do_not_spend(locked.get_id().as_str(), true)
.unwrap();
let locked_live = account
.utxos()
.unwrap()
.into_iter()
.find(|o| o.get_id() == locked.get_id())
.unwrap();

let params = TransactionParams {
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
amount: 2003,
fee_rate: FeeRateSatPerKvb(1000),
selected_outputs: vec![locked_live.clone()],
note: None,
tag: None,
do_not_spend_change: false,
};
match account.get_max_fee(params) {
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
assert_eq!(ids, vec![locked_live.get_id()]);
}
other => panic!("expected LockedUtxoSelected, got {other:?}"),
}
}

#[test]
fn test_rbf_rejects_locked_selected_utxo() {
let mut account = get_ng_hot_wallet();
tests_util::add_funds_wallet_with_unconfirmed(&mut account);

// Lock a confirmed mature UTXO (one of the funding outputs) so the
// RBF builder will see it as a candidate input.
let mature = account
.utxos()
.unwrap()
.into_iter()
.find(|o| o.is_confirmed)
.expect("at least one confirmed utxo");
account
.set_do_not_spend(mature.get_id().as_str(), true)
.unwrap();
let locked_live = account
.utxos()
.unwrap()
.into_iter()
.find(|o| o.get_id() == mature.get_id())
.unwrap();

let unconfirmed_tx = account
.transactions()
.unwrap()
.into_iter()
.find(|tx| tx.confirmations == 0)
.expect("expected an unconfirmed tx for RBF");

match account.get_max_bump_fee(vec![locked_live.clone()], unconfirmed_tx) {
Err(BumpFeeError::LockedUtxoSelected(ids)) => {
assert_eq!(ids, vec![locked_live.get_id()]);
}
other => panic!("expected LockedUtxoSelected, got {other:?}"),
}
}
}

// fn pretty_print<T: serde::Serialize>(value: &T) -> String {
Expand Down
Loading