fix(wallet): treat unreachable relative locktimes as unsatisfied

src/wallet/utils.rs

@@ -109,12 +109,19 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk> 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!(<Older as Satisfier<PublicKey>>::check_older(
+            &older,
+            relative::LockTime::from_height(144)
+        ));
+
+        let older = Older::new(Some(200), Some(100), false);
+        assert!(!<Older as Satisfier<PublicKey>>::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!(!<Older as Satisfier<PublicKey>>::check_older(
+            &older,
+            relative::LockTime::from_height(144)
+        ));
+    }
 }

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();