fix(policy)!: Fix SatisfiableItem id calculation for Thresh nodes

src/descriptor/policy.rs

@@ -171,9 +171,46 @@ impl SatisfiableItem {
     }
 
     /// Returns a unique id for the [`SatisfiableItem`]
+    ///
+    /// The id is computed as a checksum over a canonical string representation of
+    /// the item's *structural* data:
+    ///
+    /// - For **leaf items** (signatures, preimages, timelocks) the representation is the JSON
+    ///   serialization of the item itself.
+    /// - For **[`SatisfiableItem::Thresh`]** the representation is
+    ///   `thresh(<k>,[<child_id0>,<child_id1>,...])`, using only the threshold value and the
+    ///   already-stable ids of the child policies. This deliberately excludes `contribution` and
+    ///   `satisfaction` from child [`Policy`] nodes, which are runtime-dependent (they change based
+    ///   on which signing keys are present), and would otherwise cause the id to vary for the same
+    ///   descriptor structure.
+    ///
+    /// This ensures that the policy id is fixed for a given descriptor structure and
+    /// key set, regardless of whether private or public keys are loaded into the signer.
+    ///
+    /// # Migration note
+    ///
+    /// The id derivation for [`SatisfiableItem::Thresh`] nodes was changed to fix id instability
+    /// caused by runtime key state (see [issue #123]). Any previously persisted
+    /// [`SatisfiableItem::Thresh`] node ids are now stale and must be recomputed from the
+    /// descriptor.
+    ///
+    /// [issue #123]: https://github.com/bitcoindevkit/bdk_wallet/issues/123
     pub fn id(&self) -> String {
-        calc_checksum(&serde_json::to_string(self).expect("Failed to serialize a SatisfiableItem"))
-            .expect("Failed to compute a SatisfiableItem id")
+        let payload = match self {
+            SatisfiableItem::Thresh { items, threshold } => {
+                // Use only structural data: threshold + child policy ids.
+                // Child policy contribution/satisfaction bake in runtime state and must
+                // not influence the id.
+                let child_ids = items
+                    .iter()
+                    .map(|p| p.id.as_str())
+                    .collect::<Vec<_>>()
+                    .join(",");
+                format!("thresh({threshold},[{child_ids}])")
+            }
+            _ => serde_json::to_string(self).expect("Failed to serialize a SatisfiableItem"),
+        };
+        calc_checksum(&payload).expect("Failed to compute a SatisfiableItem id")
     }
 }
 
@@ -1878,6 +1915,79 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_thresh_policy_id_stable_regardless_of_keys() {
+        // Regression: policy node IDs for compound (Thresh) items must not depend on
+        // whether private or public keys are present in the descriptor.
+        //
+        // Previously, `SatisfiableItem::Thresh` was serialized to JSON for the id
+        // calculation. Because `SatisfiableItem::Thresh` contains `Vec<Policy>`, and
+        // `Policy` carries `contribution` / `satisfaction` (runtime state that differs
+        // when private keys are available), the id changed based on the signing context
+        // rather than the descriptor structure.
+        let secp = Secp256k1::new();
+
+        // all public keys — each child policy has contribution: Satisfaction::None
+        let (_prv0a, pub0a, _) = setup_keys(TPRV0_STR, PATH, &secp);
+        let (_prv1a, pub1a, _) = setup_keys(TPRV1_STR, PATH, &secp);
+        let desc = descriptor!(sh(and_v(v: pk(pub0a), pk(pub1a)))).unwrap();
+        let (wallet_desc, keymap) = desc
+            .into_wallet_descriptor(&secp, NetworkKind::Test)
+            .unwrap();
+        let signers = Arc::new(SignersContainer::build(keymap, &wallet_desc, &secp));
+        let id_all_pub = wallet_desc
+            .extract_policy(&signers, BuildSatisfaction::None, &secp)
+            .unwrap()
+            .unwrap()
+            .id;
+
+        // prv0 + pub1 — first child has contribution: Complete, second: None
+        let (prv0b, _pub0b, _) = setup_keys(TPRV0_STR, PATH, &secp);
+        let (_prv1b, pub1b, _) = setup_keys(TPRV1_STR, PATH, &secp);
+        let desc = descriptor!(sh(and_v(v: pk(prv0b), pk(pub1b)))).unwrap();
+        let (wallet_desc, keymap) = desc
+            .into_wallet_descriptor(&secp, NetworkKind::Test)
+            .unwrap();
+        let signers = Arc::new(SignersContainer::build(keymap, &wallet_desc, &secp));
+        let id_mixed = wallet_desc
+            .extract_policy(&signers, BuildSatisfaction::None, &secp)
+            .unwrap()
+            .unwrap()
+            .id;
+
+        // pub0 + prv1 — first child has contribution: None, second: Complete
+        let (_prv0c, pub0c, _) = setup_keys(TPRV0_STR, PATH, &secp);
+        let (prv1c, _pub1c, _) = setup_keys(TPRV1_STR, PATH, &secp);
+        let desc = descriptor!(sh(and_v(v: pk(pub0c), pk(prv1c)))).unwrap();
+        let (wallet_desc, keymap) = desc
+            .into_wallet_descriptor(&secp, NetworkKind::Test)
+            .unwrap();
+        let signers = Arc::new(SignersContainer::build(keymap, &wallet_desc, &secp));
+        let id_mixed_inv = wallet_desc
+            .extract_policy(&signers, BuildSatisfaction::None, &secp)
+            .unwrap()
+            .unwrap()
+            .id;
+
+        // all private keys — both children have contribution: Complete
+        let (prv0d, _pub0d, _) = setup_keys(TPRV0_STR, PATH, &secp);
+        let (prv1d, _pub1d, _) = setup_keys(TPRV1_STR, PATH, &secp);
+        let desc = descriptor!(sh(and_v(v: pk(prv0d), pk(prv1d)))).unwrap();
+        let (wallet_desc, keymap) = desc
+            .into_wallet_descriptor(&secp, NetworkKind::Test)
+            .unwrap();
+        let signers = Arc::new(SignersContainer::build(keymap, &wallet_desc, &secp));
+        let id_all_prv = wallet_desc
+            .extract_policy(&signers, BuildSatisfaction::None, &secp)
+            .unwrap()
+            .unwrap()
+            .id;
+
+        assert_eq!(id_all_pub, id_mixed, "pub+pub vs prv+pub ids differ");
+        assert_eq!(id_all_pub, id_mixed_inv, "pub+pub vs pub+prv ids differ");
+        assert_eq!(id_all_pub, id_all_prv, "pub+pub vs prv+prv ids differ");
+    }
+
     #[test]
     fn test_extract_tr_satisfaction_script_spend() {
         const UNSIGNED_PSBT: &str = "cHNidP8BAFMBAAAAAWZalxaErOL7P3WPIUc8DsjgE68S+ww+uqiqEI2SAwlPAAAAAAD/////AQiWmAAAAAAAF6kU4R3W8CnGzZcSsaovTYu0X8vHt3WHAAAAAAABASuAlpgAAAAAACJRINa6bLPZwp3/CYWoxyI3mLYcSC5f9LInAMUng94nspa2IhXBgiPY+kcolS1Hp0niOK/+7VHz6F+nsz8JVxnzWzkgToYjIHhGyuexxtRVKevRx4YwWR/W0r7LPHt6oS6DLlzyuYQarMAhFnhGyuexxtRVKevRx4YwWR/W0r7LPHt6oS6DLlzyuYQaLQH2onWFc3UR6I9ZhuHVeJCi5LNAf4APVd7mHn4BhdViHRwu7j4AAACAAgAAACEWgiPY+kcolS1Hp0niOK/+7VHz6F+nsz8JVxnzWzkgToYNAMkRfC4AAACAAgAAAAEXIIIj2PpHKJUtR6dJ4jiv/u1R8+hfp7M/CVcZ81s5IE6GARgg9qJ1hXN1EeiPWYbh1XiQouSzQH+AD1Xe5h5+AYXVYh0AAA==";

tests/wallet.rs

@@ -1991,7 +1991,7 @@ fn test_taproot_psbt_populate_tap_key_origins_repeated_key() {
     let (mut wallet, _) = get_funded_wallet(get_test_tr_repeated_key(), get_test_tr_single_sig());
     let addr = wallet.reveal_next_address(KeychainKind::External);
 
-    let path = vec![("rn4nre9c".to_string(), vec![0])]
+    let path = vec![("vj73w7cm".to_string(), vec![0])]
         .into_iter()
         .collect();