diff --git a/src/descriptor/policy.rs b/src/descriptor/policy.rs index abf5024b..a5e5736b 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 d559c784..1f6a3baa 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 02007964..ef049f5f 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 { @@ -109,12 +118,18 @@ 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) + match self + .create_height + .unwrap_or(ConfirmationHeight::Confirmed(0)) + { + // 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()) - .expect("Overflowing addition") + .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 } @@ -170,10 +185,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::{ConfirmationHeight, 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 +294,56 @@ 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(ConfirmationHeight::Confirmed(100)), false); + assert!(>::check_older( + &older, + relative::LockTime::from_height(144) + )); + + 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) + )); + } + + #[test] + fn test_check_older_unreachable_satisfaction_height_is_not_satisfied() { + let older = Older::new( + Some(u32::MAX), + Some(ConfirmationHeight::Confirmed(u32::MAX)), + false, + ); + assert!(!>::check_older( + &older, + 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) + )); + } } diff --git a/tests/wallet.rs b/tests/wallet.rs index 557bd2bb..9041cb03 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 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) + .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();