Merge #584: descriptors: fix analysis of missing signatures post #575

10acc482e15c0f5832da576baddd10ce44dc85df descriptors: fix the signatures count analysis (Antoine Poinsot)
7a33040b83796b0517a4955ac69f8766577b8442 descriptors: make signed_pubkeys a mapping from fingerprint (Antoine Poinsot)

Pull request description:

  Needs more testing but that should do the trick: we need to take into account the derivation path appended to the xpub in addition to the one from the origin!

ACKs for top commit:
  darosior:
    ACK 10acc482e15c0f5832da576baddd10ce44dc85df -- Edouard ACK'd the child PR at https://github.com/wizardsardine/liana/pull/599#pullrequestreview-1571990532

Tree-SHA512: 912f482c5e10ece4127ffc86620f8ec581d0a209f8e1d407fd9885a7b6d1dfb13e7d5898c75da782282b9d40f69a80ad835e5a7648ceddcfc809445cdf246efb

src/descriptors/analysis.rs

@@ -130,13 +130,27 @@ fn csv_check(csv_value: u32) -> Result<u16, LianaPolicyError> {
     }
 }
 
-// Get the origin of a key in a multipath descriptors.
+// Get the fingerprint and the full derivation paths (path from the master fingerprint in the
-// Returns None if the given key isn't a multixpub.
+// origin, with the xpub derivation path appended) for a multipath xpub.
-fn key_origin(
+fn key_origins(
     key: &descriptor::DescriptorPublicKey,
-) -> Option<&(bip32::Fingerprint, bip32::DerivationPath)> {
+) -> Option<(bip32::Fingerprint, HashSet<bip32::DerivationPath>)> {
     match key {
-        descriptor::DescriptorPublicKey::MultiXPub(ref xpub) => xpub.origin.as_ref(),
+        descriptor::DescriptorPublicKey::MultiXPub(ref xpub) => {
+            xpub.origin.as_ref().map(|(fg, orig_path)| {
+                let mut der_paths = HashSet::with_capacity(xpub.derivation_paths.paths().len());
+                for der_path in xpub.derivation_paths.paths() {
+                    der_paths.insert(
+                        orig_path
+                            .into_iter()
+                            .chain(der_path.into_iter())
+                            .copied()
+                            .collect(),
+                    );
+                }
+                (*fg, der_paths)
+            })
+        }
         _ => None,
     }
 }
@@ -241,28 +255,36 @@ impl PathInfo {
     }
 
     /// Get the required number of keys for spending through this path, and the set of keys
-    /// that can be used to provide a signature for this path.
+    /// that can be used to provide a signature for this path. The set of keys is represented as a
-    pub fn thresh_origins(&self) -> (usize, HashSet<(bip32::Fingerprint, bip32::DerivationPath)>) {
+    /// mapping from a master extended key fingerprint, to a set of derivation paths. This is
+    /// because we are using multipath descriptors. The derivation paths included the xpub's
+    /// derivation path appended to the origin's derivation path (without the wildcard step).
+    pub fn thresh_origins(
+        &self,
+    ) -> (
+        usize,
+        HashMap<bip32::Fingerprint, HashSet<bip32::DerivationPath>>,
+    ) {
         match self {
             PathInfo::Single(key) => {
-                let mut origins = HashSet::with_capacity(1);
+                let mut all_origins = HashMap::with_capacity(1);
-                origins.insert(
+                let (fg, der_path) = key_origins(key).expect("Must be a multixpub with an origin.");
-                    key_origin(key)
+                all_origins.insert(fg, der_path);
-                        .expect("Must be a multixpub with an origin.")
+                (1, all_origins)
-                        .clone(),
+            }
-                );
+            PathInfo::Multi(k, keys) => {
-                (1, origins)
+                let mut all_origins: HashMap<_, HashSet<_>> = HashMap::with_capacity(keys.len());
+                for key in keys {
+                    let (fg, der_paths) =
+                        key_origins(key).expect("Must be a multixpub with an origin.");
+                    if let Some(existing_der_paths) = all_origins.get_mut(&fg) {
+                        existing_der_paths.extend(der_paths)
+                    } else {
+                        all_origins.insert(fg, der_paths);
+                    }
+                }
+                (*k, all_origins)
             }
-            PathInfo::Multi(k, keys) => (
-                *k,
-                keys.iter()
-                    .map(|key| {
-                        key_origin(key)
-                            .expect("Must be a multixpub with an origin.")
-                            .clone()
-                    })
-                    .collect(),
-            ),
         }
     }
 
@@ -278,24 +300,28 @@ impl PathInfo {
 
         // For all existing signatures, pick those that are from one of our pubkeys.
         for (fg, der_path) in all_pubkeys_signed {
-            // For all xpubs in the descriptor, we derive at /0/* or /1/*, so the xpub's origin's
+            // All xpubs in the descriptor must be wildcard, and therefore have a derivation with
-            // derivation path is the key's one without the last two derivation indexes.
+            // at least one step. (In practice there is at least two, for `/<0;1>/*`.)
-            if der_path.len() < 2 {
+            if der_path.is_empty() {
                 continue;
             }
-            let parent_der_path: bip32::DerivationPath = der_path[..der_path.len() - 2].into();
-            let parent_origin = (*fg, parent_der_path);
 
-            // Now if the origin of this key without the two final derivation indexes is part of
+            // Now check if this signature is for a public key derived from the fingerprint of one
-            // the set of our keys, count it as a signature for it. Note it won't mixup keys
+            // of our known master xpubs.
-            // between spending paths, since we can't have duplicate xpubs in the descriptor and
+            if let Some(parent_der_paths) = origins.get(fg) {
-            // the (fingerprint, der_path) tuple is a UID for an xpub.
+                // If it is, make sure it's for one of the xpubs included in the descriptor. Remove
-            if origins.contains(&parent_origin) {
+                // the wilcard step and check if it's in the set of the derivation paths.
-                sigs_count += 1;
+                let der_path_wo_wc: bip32::DerivationPath = der_path[..der_path.len() - 1].into();
-                if let Some(count) = signed_pubkeys.get_mut(&parent_origin) {
+                if parent_der_paths.contains(&der_path_wo_wc) {
-                    *count += 1;
+                    // If the origin of the key without the wilcard step is part of our keys, count
-                } else {
+                    // it as a signature. Also record how many times this master extended key
-                    signed_pubkeys.insert(parent_origin, 1);
+                    // signed.
+                    sigs_count += 1;
+                    if let Some(count) = signed_pubkeys.get_mut(fg) {
+                        *count += 1;
+                    } else {
+                        signed_pubkeys.insert(*fg, 1);
+                    }
                 }
             }
         }
@@ -516,7 +542,7 @@ pub struct PathSpendInfo {
     pub sigs_count: usize,
     /// The keys for which a signature was provided and the number (always >=1) of
     /// signatures provided for this key.
-    pub signed_pubkeys: HashMap<(bip32::Fingerprint, bip32::DerivationPath), usize>,
+    pub signed_pubkeys: HashMap<bip32::Fingerprint, usize>,
 }
 
 /// Information about a partial spend of Liana coins

src/descriptors/mod.rs

@@ -762,10 +762,7 @@ mod tests {
         // A simple descriptor with 1 keys as primary path and 1 recovery key.
         let desc = LianaDescriptor::from_str("wsh(or_d(pk([f5acc2fd]tpubD6NzVbkrYhZ4YgUx2ZLNt2rLYAMTdYysCRzKoLu2BeSHKvzqPaBDvf17GeBPnExUVPkuBpx4kniP964e2MxyzzazcXLptxLXModSVCVEV1T/<0;1>/*),and_v(v:pkh([8a64f2a9]tpubD6NzVbkrYhZ4WmzFjvQrp7sDa4ECUxTi9oby8K4FZkd3XCBtEdKwUiQyYJaxiJo5y42gyDWEczrFpozEjeLxMPxjf2WtkfcbpUdfvNnozWF/<0;1>/*),older(10))))#d72le4dr").unwrap();
         let desc_info = desc.policy();
-        let prim_key_origin = (
+        let prim_key_fg = bip32::Fingerprint::from_str("f5acc2fd").unwrap();
-            bip32::Fingerprint::from_str("f5acc2fd").unwrap(),
-            Vec::new().into(),
-        );
         let recov_key_origin: (_, bip32::DerivationPath) = (
             bip32::Fingerprint::from_str("8a64f2a9").unwrap(),
             Vec::new().into(),
@@ -806,12 +803,10 @@ mod tests {
         assert_eq!(signed_single_psbt.inputs[0].partial_sigs.len(), 1);
         assert_eq!(info.primary_path.threshold, 1);
         assert_eq!(info.primary_path.sigs_count, 1);
+        dbg!(&info.primary_path.signed_pubkeys);
         assert!(
             info.primary_path.signed_pubkeys.len() == 1
-                && info
+                && info.primary_path.signed_pubkeys.contains_key(&prim_key_fg)
-                    .primary_path
-                    .signed_pubkeys
-                    .contains_key(&prim_key_origin)
         );
         assert!(info.recovery_paths.is_empty());
 
@@ -852,7 +847,7 @@ mod tests {
         assert_eq!(recov_info.sigs_count, 1);
         assert!(
             recov_info.signed_pubkeys.len() == 1
-                && recov_info.signed_pubkeys.contains_key(&recov_key_origin)
+                && recov_info.signed_pubkeys.contains_key(&recov_key_origin.0)
         );
 
         // A PSBT with multiple inputs, all signed for the primary path.
@@ -866,10 +861,7 @@ mod tests {
         assert_eq!(info.primary_path.sigs_count, 1);
         assert!(
             info.primary_path.signed_pubkeys.len() == 1
-                && info
+                && info.primary_path.signed_pubkeys.contains_key(&prim_key_fg)
-                    .primary_path
-                    .signed_pubkeys
-                    .contains_key(&prim_key_origin)
         );
         assert!(info.recovery_paths.is_empty());
 
@@ -887,10 +879,7 @@ mod tests {
         assert_eq!(info.primary_path.sigs_count, 1);
         assert!(
             info.primary_path.signed_pubkeys.len() == 1
-                && info
+                && info.primary_path.signed_pubkeys.contains_key(&prim_key_fg)
-                    .primary_path
-                    .signed_pubkeys
-                    .contains_key(&prim_key_origin)
         );
         let recov_info = info.recovery_paths.get(&timelock).unwrap();
         assert_eq!(recov_info.threshold, 1);
@@ -927,10 +916,7 @@ mod tests {
         assert_eq!(info.primary_path.sigs_count, 1);
         assert!(
             info.primary_path.signed_pubkeys.len() == 1
-                && info
+                && info.primary_path.signed_pubkeys.contains_key(&prim_key_fg)
-                    .primary_path
-                    .signed_pubkeys
-                    .contains_key(&prim_key_origin)
         );
         assert!(info.recovery_paths.is_empty());