From ebd6103e6597e0a66470541d2fcd25709ea77866 Mon Sep 17 00:00:00 2001
From: Alekos Filini <alekos.filini@gmail.com>
Date: Fri, 31 Mar 2023 20:08:23 +0200
Subject: [PATCH] Fix policy condition calculation

When constructing the `Condition` struct we recursively call
`get_condition` on all the items in a threshold and short-circuit if
there's an error somewhere (for example, because the policy-path hasn't
been provided for a specific threshold).

This can cause issues when the user doesn't care about a subtree, because
we still try to call `get_condition` on all the items and fail if
something is missing, even if the specific subtree isn't selected and
won't be used later on.

This commit changes the logic so that we first filter only the `selected`
items, and then unwrap the error using the question mark. If errors
happened somewhere else they will be ignored, as it should.
---
 crates/bdk/src/descriptor/policy.rs | 41 ++++++++++++++++-------------
 crates/bdk/tests/wallet.rs          | 19 +++++++++++++
 2 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/crates/bdk/src/descriptor/policy.rs b/crates/bdk/src/descriptor/policy.rs
index af3e4a3b..5de20ae7 100644
--- a/crates/bdk/src/descriptor/policy.rs
+++ b/crates/bdk/src/descriptor/policy.rs
@@ -662,11 +662,11 @@ impl Policy {
                 (0..*threshold).collect()
             }
             SatisfiableItem::Multisig { keys, .. } => (0..keys.len()).collect(),
-            _ => vec![],
+            _ => HashSet::new(),
         };
-        let selected = match path.get(&self.id) {
-            Some(arr) => arr,
-            _ => &default,
+        let selected: HashSet<_> = match path.get(&self.id) {
+            Some(arr) => arr.iter().copied().collect(),
+            _ => default,
         };
 
         match &self.item {
@@ -674,14 +674,24 @@ impl Policy {
                 let mapped_req = items
                     .iter()
                     .map(|i| i.get_condition(path))
-                    .collect::<Result<Vec<_>, _>>()?;
+                    .collect::<Vec<_>>();
 
                 // if all the requirements are null we don't care about `selected` because there
                 // are no requirements
-                if mapped_req.iter().all(Condition::is_null) {
+                if mapped_req
+                    .iter()
+                    .all(|cond| matches!(cond, Ok(c) if c.is_null()))
+                {
                     return Ok(Condition::default());
                 }
 
+                // make sure all the indexes in the `selected` list are within range
+                for index in &selected {
+                    if *index >= items.len() {
+                        return Err(PolicyError::IndexOutOfRange(*index));
+                    }
+                }
+
                 // if we have something, make sure we have enough items. note that the user can set
                 // an empty value for this step in case of n-of-n, because `selected` is set to all
                 // the elements above
@@ -690,23 +700,18 @@ impl Policy {
                 }
 
                 // check the selected items, see if there are conflicting requirements
-                let mut requirements = Condition::default();
-                for item_index in selected {
-                    requirements = requirements.merge(
-                        mapped_req
-                            .get(*item_index)
-                            .ok_or(PolicyError::IndexOutOfRange(*item_index))?,
-                    )?;
-                }
-
-                Ok(requirements)
+                mapped_req
+                    .into_iter()
+                    .enumerate()
+                    .filter(|(index, _)| selected.contains(index))
+                    .try_fold(Condition::default(), |acc, (_, cond)| acc.merge(&cond?))
             }
             SatisfiableItem::Multisig { keys, threshold } => {
                 if selected.len() < *threshold {
                     return Err(PolicyError::NotEnoughItemsSelected(self.id.clone()));
                 }
-                if let Some(item) = selected.iter().find(|i| **i >= keys.len()) {
-                    return Err(PolicyError::IndexOutOfRange(*item));
+                if let Some(item) = selected.into_iter().find(|&i| i >= keys.len()) {
+                    return Err(PolicyError::IndexOutOfRange(item));
                 }
 
                 Ok(Condition::default())
diff --git a/crates/bdk/tests/wallet.rs b/crates/bdk/tests/wallet.rs
index 9b25223e..0ada20d3 100644
--- a/crates/bdk/tests/wallet.rs
+++ b/crates/bdk/tests/wallet.rs
@@ -925,6 +925,25 @@ fn test_create_tx_policy_path_use_csv() {
     assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(144));
 }
 
+#[test]
+fn test_create_tx_policy_path_ignored_subtree_with_csv() {
+    let (mut wallet, _) = get_funded_wallet("wsh(or_d(pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu),or_i(and_v(v:pkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),older(30)),and_v(v:pkh(cMnkdebixpXMPfkcNEjjGin7s94hiehAH4mLbYkZoh9KSiNNmqC8),older(90)))))");
+
+    let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap();
+    let root_id = external_policy.id;
+    // child #0 is pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu)
+    let path = vec![(root_id, vec![0])].into_iter().collect();
+
+    let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap();
+    let mut builder = wallet.build_tx();
+    builder
+        .add_recipient(addr.script_pubkey(), 30_000)
+        .policy_path(path, KeychainKind::External);
+    let (psbt, _) = builder.finish().unwrap();
+
+    assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(0xFFFFFFFE));
+}
+
 #[test]
 fn test_create_tx_global_xpubs_with_origin() {
     use bitcoin::hashes::hex::FromHex;