From 7461a4341dfa53dc0d292573154758f16d28f376 Mon Sep 17 00:00:00 2001 From: trakshan-mishra Date: Wed, 16 Sep 2026 18:38:51 +0530 Subject: [PATCH 1/3] fix(wallet): treat unreachable relative locktimes as unsatisfied `Older::check_older` added the relative locktime to the previous transaction's confirmation height and unwrapped the sum with `expect("Overflowing addition")`. `Wallet::finalize_psbt` maps an unconfirmed previous transaction to a confirmation height of `u32::MAX`, so finalizing a PSBT that spends an unconfirmed UTXO of a descriptor carrying an `older(n)` branch panicked for every `n` greater than zero. The `Older` satisfier is only consulted when the input's `nSequence` does not already satisfy the CSV branch, so this is reachable through ordinary use: spending an unconfirmed output through a branch that is not the `older()` one, which `Wallet::sign` finalizes by default. Return `false` when the satisfaction height does not fit in a `u32`. Such a height can never be reached, so the branch is simply not satisfied, which is the same answer the comparison would give with wider integers. --- src/wallet/utils.rs | 50 +++++++++++++++++++++++++++++++++++++-------- tests/wallet.rs | 48 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 8 deletions(-) diff --git a/src/wallet/utils.rs b/src/wallet/utils.rs index 020079643..076781a6c 100644 --- a/src/wallet/utils.rs +++ b/src/wallet/utils.rs @@ -109,12 +109,19 @@ impl Satisfier for Older { fn check_older(&self, n: relative::LockTime) -> bool { if let Some(current_height) = self.current_height { // TODO: test >= / > - current_height - >= self - .create_height - .unwrap_or(0) - .checked_add(n.to_consensus_u32()) - .expect("Overflowing addition") + match self + .create_height + .unwrap_or(0) + .checked_add(n.to_consensus_u32()) + { + Some(satisfaction_height) => current_height >= satisfaction_height, + // The height at which the relative locktime would be satisfied does not fit in + // a `u32` and can therefore never be reached, so the branch is not satisfied. + // `Wallet::finalize_psbt` maps an unconfirmed previous transaction to a + // `create_height` of `u32::MAX`, which makes this reachable for any `older(n)` + // with `n` greater than zero. + None => false, + } } else { self.assume_height_reached } @@ -170,10 +177,11 @@ mod test { // otherwise it's time-based pub(crate) const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 1 << 22; - use super::{IsDust, check_nsequence_rbf, shuffle_slice}; - use crate::bitcoin::{Address, Network, Sequence}; + use super::{IsDust, Older, check_nsequence_rbf, shuffle_slice}; + use crate::bitcoin::{Address, Network, PublicKey, Sequence, relative}; use alloc::vec::Vec; use core::str::FromStr; + use miniscript::Satisfier; use rand::{SeedableRng, rngs::StdRng, thread_rng}; #[test] @@ -278,4 +286,30 @@ mod test { shuffle_slice(&mut test, &mut rng); assert_eq!(test, &[0, 4, 1, 2, 5]); } + + #[test] + fn test_check_older_compares_against_the_satisfaction_height() { + let older = Older::new(Some(300), Some(100), false); + assert!(>::check_older( + &older, + relative::LockTime::from_height(144) + )); + + let older = Older::new(Some(200), Some(100), false); + assert!(!>::check_older( + &older, + relative::LockTime::from_height(144) + )); + } + + #[test] + fn test_check_older_unreachable_satisfaction_height_is_not_satisfied() { + // `Wallet::finalize_psbt` maps an unconfirmed previous transaction to a `create_height` + // of `u32::MAX`, so adding a relative locktime to it does not fit in a `u32`. + let older = Older::new(Some(100_000), Some(u32::MAX), false); + assert!(!>::check_older( + &older, + relative::LockTime::from_height(144) + )); + } } diff --git a/tests/wallet.rs b/tests/wallet.rs index 557bd2bbf..99d04974e 100644 --- a/tests/wallet.rs +++ b/tests/wallet.rs @@ -2038,6 +2038,54 @@ fn test_try_finalize_psbt_uses_psbt_timelocks() { } } +#[test] +fn test_finalize_psbt_with_unconfirmed_input_and_unused_csv_branch() { + // `wsh(or_d(pk(A),and_v(v:pk(B),older(144))))`. Spending through the `pk(A)` branch leaves + // the `older(144)` branch for the finalizer to evaluate, and `finalize_psbt` maps an + // unconfirmed previous transaction to a confirmation height of `u32::MAX`. Adding the + // relative locktime to that height overflows, which used to panic. + let descriptor = get_test_a_or_b_plus_csv(); + let mut wallet = Wallet::create_single(descriptor) + .network(Network::Regtest) + .create_wallet_no_persist() + .unwrap(); + let addr = wallet.next_unused_address(KeychainKind::External); + + // Fund the wallet with a transaction that stays unconfirmed. + let funding_tx = Transaction { + output: vec![TxOut { + value: Amount::from_sat(50_000), + script_pubkey: addr.script_pubkey(), + }], + ..new_tx(0) + }; + insert_tx(&mut wallet, funding_tx); + + let policy = wallet + .public_descriptor(KeychainKind::External) + .extract_policy( + &SignersContainer::default(), + BuildSatisfaction::None, + wallet.secp_ctx(), + ) + .unwrap() + .expect("descriptor has a spending policy"); + // Child #0 is `pk(A)`, so the `older(144)` branch is not the one being used. + let path = [(policy.id.clone(), vec![0])].into_iter().collect(); + let condition = policy.get_condition(&path).unwrap(); + + let mut builder = wallet.build_tx(); + builder + .add_recipient(addr.script_pubkey(), Amount::from_sat(10_000)) + .set_condition(condition); + let mut psbt = builder.finish().unwrap(); + + let finalized = wallet + .finalize_psbt(&mut psbt, SignOptions::default()) + .unwrap(); + assert!(!finalized, "an unsigned PSBT cannot be finalized"); +} + #[test] fn test_taproot_try_finalize_sign_option() { let descriptor = get_test_tr_with_taptree(); From bc886ff07e982d9498fdc8d2458028c5ac4d8260 Mon Sep 17 00:00:00 2001 From: trakshan-mishra Date: Tue, 29 Sep 2026 19:24:39 +0530 Subject: [PATCH 2/3] refactor(wallet): represent unconfirmed inputs explicitly in `Older` `finalize_psbt` used `u32::MAX` as the confirmation height of an unconfirmed previous transaction, and `check_older` only returned `false` for it because adding the relative locktime overflowed. Add a `ConfirmationHeight` enum with `Confirmed(u32)` and `Unconfirmed` variants and use it for `Older::create_height`. `check_older` now returns `false` for `Unconfirmed` directly. `None` keeps its existing meaning: the previous transaction was not found, and the height is treated as 0. An overflowing `Confirmed` height still returns `false` instead of panicking. --- src/descriptor/policy.rs | 8 ++++-- src/wallet/mod.rs | 12 +++++---- src/wallet/utils.rs | 53 +++++++++++++++++++++++++++------------- tests/wallet.rs | 6 ++--- 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/src/descriptor/policy.rs b/src/descriptor/policy.rs index abf5024b9..a5e5736b4 100644 --- a/src/descriptor/policy.rs +++ b/src/descriptor/policy.rs @@ -64,7 +64,7 @@ use crate::descriptor::ExtractPolicy; use crate::keys::ExtScriptContext; use crate::types::IndexOutOfBoundsError; use crate::wallet::signer::{SignerId, SignersContainer}; -use crate::wallet::utils::{After, Older, SecpCtx}; +use crate::wallet::utils::{After, ConfirmationHeight, Older, SecpCtx}; use super::XKeyUtils; use super::checksum::calc_checksum; @@ -980,7 +980,11 @@ impl ExtractPolicy for Miniscript::check_older(&older, (*value).into()); let inputs_sat = psbt_inputs_sat(psbt).all(|sat| { diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index d559c7846..1f6a3baa1 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -76,7 +76,7 @@ use crate::wallet::{ error::{BuildFeeBumpError, CreateTxError, MiniscriptPsbtError}, signer::{SignOptions, SignerError, SignerOrdering, SignersContainer, TransactionSigner}, tx_builder::{FeePolicy, TxBuilder, TxParams}, - utils::{After, Older, SecpCtx, check_nsequence_rbf}, + utils::{After, ConfirmationHeight, Older, SecpCtx, check_nsequence_rbf}, }; // Unstable bdk-tx imports — only active with --cfg bdk_wallet_unstable and feature = "bdk-tx". #[cfg(all(bdk_wallet_unstable, feature = "bdk-tx", feature = "std"))] @@ -2017,11 +2017,13 @@ impl Wallet { .map(|canon_tx| { let txid = canon_tx.tx_node.txid; match canon_tx.chain_position { - ChainPosition::Confirmed { anchor, .. } => (txid, anchor.block_id.height), - ChainPosition::Unconfirmed { .. } => (txid, u32::MAX), + ChainPosition::Confirmed { anchor, .. } => { + (txid, ConfirmationHeight::Confirmed(anchor.block_id.height)) + } + ChainPosition::Unconfirmed { .. } => (txid, ConfirmationHeight::Unconfirmed), } }) - .collect::>(); + .collect::>(); let current_height = sign_options .assume_height .unwrap_or_else(|| self.chain.tip().height()); @@ -2066,7 +2068,7 @@ impl Wallet { clear_output_derivations: bool, ) -> Result where - F: FnMut(usize, &bitcoin::TxIn) -> Option, + F: FnMut(usize, &bitcoin::TxIn) -> Option, { let tx = &psbt.unsigned_tx; if psbt.inputs.len() < tx.input.len() { diff --git a/src/wallet/utils.rs b/src/wallet/utils.rs index 076781a6c..f85db26d2 100644 --- a/src/wallet/utils.rs +++ b/src/wallet/utils.rs @@ -85,16 +85,25 @@ impl Satisfier for After { } } +/// Chain position of the transaction that created an input, as seen by [`Older`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConfirmationHeight { + /// The transaction is confirmed at this height. + Confirmed(u32), + /// The transaction is not confirmed yet. + Unconfirmed, +} + pub struct Older { pub current_height: Option, - pub create_height: Option, + pub create_height: Option, pub assume_height_reached: bool, } impl Older { pub(crate) fn new( current_height: Option, - create_height: Option, + create_height: Option, assume_height_reached: bool, ) -> Older { Older { @@ -111,16 +120,15 @@ impl Satisfier for Older { // TODO: test >= / > match self .create_height - .unwrap_or(0) - .checked_add(n.to_consensus_u32()) + .unwrap_or(ConfirmationHeight::Confirmed(0)) { - Some(satisfaction_height) => current_height >= satisfaction_height, - // The height at which the relative locktime would be satisfied does not fit in - // a `u32` and can therefore never be reached, so the branch is not satisfied. - // `Wallet::finalize_psbt` maps an unconfirmed previous transaction to a - // `create_height` of `u32::MAX`, which makes this reachable for any `older(n)` - // with `n` greater than zero. - None => false, + // A height that does not fit in a `u32` can never be reached. + ConfirmationHeight::Confirmed(create_height) => create_height + .checked_add(n.to_consensus_u32()) + .is_some_and(|satisfaction_height| current_height >= satisfaction_height), + // A relative locktime counts from the block that confirms the previous + // transaction, so it cannot be met while that transaction is unconfirmed. + ConfirmationHeight::Unconfirmed => false, } } else { self.assume_height_reached @@ -177,7 +185,7 @@ mod test { // otherwise it's time-based pub(crate) const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 1 << 22; - use super::{IsDust, Older, check_nsequence_rbf, shuffle_slice}; + use super::{ConfirmationHeight, IsDust, Older, check_nsequence_rbf, shuffle_slice}; use crate::bitcoin::{Address, Network, PublicKey, Sequence, relative}; use alloc::vec::Vec; use core::str::FromStr; @@ -289,13 +297,22 @@ mod test { #[test] fn test_check_older_compares_against_the_satisfaction_height() { - let older = Older::new(Some(300), Some(100), false); + let older = Older::new(Some(300), Some(ConfirmationHeight::Confirmed(100)), false); assert!(>::check_older( &older, relative::LockTime::from_height(144) )); - let older = Older::new(Some(200), Some(100), false); + let older = Older::new(Some(200), Some(ConfirmationHeight::Confirmed(100)), false); + assert!(!>::check_older( + &older, + relative::LockTime::from_height(144) + )); + } + + #[test] + fn test_check_older_unconfirmed_is_not_satisfied() { + let older = Older::new(Some(u32::MAX), Some(ConfirmationHeight::Unconfirmed), false); assert!(!>::check_older( &older, relative::LockTime::from_height(144) @@ -304,9 +321,11 @@ mod test { #[test] fn test_check_older_unreachable_satisfaction_height_is_not_satisfied() { - // `Wallet::finalize_psbt` maps an unconfirmed previous transaction to a `create_height` - // of `u32::MAX`, so adding a relative locktime to it does not fit in a `u32`. - let older = Older::new(Some(100_000), Some(u32::MAX), false); + let older = Older::new( + Some(u32::MAX), + Some(ConfirmationHeight::Confirmed(u32::MAX)), + false, + ); assert!(!>::check_older( &older, relative::LockTime::from_height(144) diff --git a/tests/wallet.rs b/tests/wallet.rs index 99d04974e..9041cb030 100644 --- a/tests/wallet.rs +++ b/tests/wallet.rs @@ -2041,9 +2041,9 @@ fn test_try_finalize_psbt_uses_psbt_timelocks() { #[test] fn test_finalize_psbt_with_unconfirmed_input_and_unused_csv_branch() { // `wsh(or_d(pk(A),and_v(v:pk(B),older(144))))`. Spending through the `pk(A)` branch leaves - // the `older(144)` branch for the finalizer to evaluate, and `finalize_psbt` maps an - // unconfirmed previous transaction to a confirmation height of `u32::MAX`. Adding the - // relative locktime to that height overflows, which used to panic. + // the `older(144)` branch for the finalizer to evaluate while the previous transaction is + // still unconfirmed. `finalize_psbt` used to stand in `u32::MAX` for the confirmation height + // of an unconfirmed transaction, and adding the relative locktime to it panicked. let descriptor = get_test_a_or_b_plus_csv(); let mut wallet = Wallet::create_single(descriptor) .network(Network::Regtest) From b806e04f8e25700b6361a18cf16c910ec604f2ec Mon Sep 17 00:00:00 2001 From: trakshan-mishra Date: Tue, 29 Sep 2026 22:23:30 +0530 Subject: [PATCH 3/3] test(wallet): cover `check_older` when `create_height` is `None` `finalize_psbt` passes `None` when the previous transaction is not among the wallet's canonical transactions, and `check_older` then counts the relative locktime from height 0. Test both sides of that boundary. --- src/wallet/utils.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/wallet/utils.rs b/src/wallet/utils.rs index f85db26d2..ef049f5fc 100644 --- a/src/wallet/utils.rs +++ b/src/wallet/utils.rs @@ -331,4 +331,19 @@ mod test { relative::LockTime::from_height(144) )); } + + #[test] + fn test_check_older_not_found_counts_from_height_zero() { + let older = Older::new(Some(144), None, false); + assert!(>::check_older( + &older, + relative::LockTime::from_height(144) + )); + + let older = Older::new(Some(143), None, false); + assert!(!>::check_older( + &older, + relative::LockTime::from_height(144) + )); + } }