Merge #742: commands: don't add derivation paths for keys from different path but same signer

2e1c54491e96fda79b3831dc0c079c59250fabe0 qa: test der paths in PSBT when desc has duplicate signer (Antoine Poinsot)
8d213d5e31dd47f0c179475e32f5eb86c3773770 qa: correct create_spend functional test (Antoine Poinsot)
cf33228b0d25e79ec8fa855aaac6ebfa5f10da12 qa: don't assume desc xpubs' der path length in finalizer (Antoine Poinsot)
24edaecbdcfc0b6b53ac3355ad22991a8b5de018 commands: don't add der paths for keys from diff path but same signer (Antoine Poinsot)

Pull request description:

  When creating a PSBT, we were checking whether the key was for this path by checking its origin. This check would return false positive for keys from other paths but same signer (which shares the same fingerprint).

  Instead, check the entire origin for each key to make sure it's actually the one used in the path we are interested about.

  Thanks to Edouard Paris for finding this bug.

ACKs for top commit:
  darosior:
    self-ACK 2e1c54491e96fda79b3831dc0c079c59250fabe0 - only made the test more robust since Edouard's ACK

Tree-SHA512: 3bbd0db9e1be59318f9e0af7b6ebf2d91d919e4946acb3dd02822cd6c0e2b9829a963ff1ecd9b0b0ab780bd46b4f0d120ab80b2f55ee84fbcf848231727b3cac

src/commands/mod.rs

@@ -26,7 +26,7 @@ use std::{
 
 use miniscript::{
     bitcoin::{
-        self, address,
+        self, address, bip32,
         locktime::absolute,
         psbt::{Input as PsbtIn, Output as PsbtOut, PartiallySignedTransaction as Psbt},
     },
@@ -237,6 +237,25 @@ fn serializable_size<T: bitcoin::consensus::Encodable + ?Sized>(t: &T) -> u64 {
     bitcoin::consensus::serialize(t).len().try_into().unwrap()
 }
 
+// Whether a given key was derived from the keys of a specific spending path.
+fn is_key_for_spending_path(
+    pubkey: &descriptors::keys::DerivedPublicKey,
+    path_origins: &HashMap<bip32::Fingerprint, HashSet<bip32::DerivationPath>>,
+) -> bool {
+    // If it comes from a signer used in this path
+    if let Some(der_paths) = path_origins.get(&pubkey.origin.0) {
+        // Get the derivation path of the parent used to derive this key. Note this is fine
+        // to only drop the last derivation step, because the keys in the policy are
+        // normalized (so the derivation path up to the wildcard is part of the origin).
+        if let Some((_, der_path_no_wildcard)) = pubkey.origin.1[..].split_last() {
+            // Now make sure it was actually derived from the xpub used in this derivation
+            // path, and not from the same signer used in another path.
+            return der_paths.contains(&(der_path_no_wildcard.into()));
+        }
+    }
+    false
+}
+
 impl DaemonControl {
     // Get the derived descriptor for this coin
     fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc {
@@ -370,7 +389,7 @@ impl DaemonControl {
             .primary_path()
             .thresh_origins();
         let is_prim_path_key =
-            |pubkey: &keys::DerivedPublicKey| prim_path_origins.contains_key(&pubkey.origin.0);
+            |pubkey: &keys::DerivedPublicKey| is_key_for_spending_path(pubkey, &prim_path_origins);
 
         // Iterate through given outpoints to fetch the coins (hence checking their existence
         // at the same time). We checked there is at least one, therefore after this loop the
@@ -814,7 +833,7 @@ impl DaemonControl {
             .ok_or(CommandError::UnknownRecoveryTimelock(timelock))?
             .thresh_origins();
         let is_key_for_this_path =
-            |pubkey: &keys::DerivedPublicKey| reco_path_origins.contains_key(&pubkey.origin.0);
+            |pubkey: &keys::DerivedPublicKey| is_key_for_spending_path(pubkey, &reco_path_origins);
 
         // Fill-in the transaction inputs and PSBT inputs information. Record the value
         // that is fed to the transaction while doing so, to compute the fees afterward.

tests/fixtures.py

@@ -156,6 +156,41 @@ def lianad(bitcoind, directory):
     lianad.cleanup()
 
 
+@pytest.fixture
+def lianad_same_signer(bitcoind, directory):
+    """A simple lianad but the same signer is used for primary and recovery."""
+    datadir = os.path.join(directory, "lianad")
+    os.makedirs(datadir, exist_ok=True)
+    bitcoind_cookie = os.path.join(bitcoind.bitcoin_dir, "regtest", ".cookie")
+
+    # We use the same xpub for both paths, but /0/<0;1>/* for the primary and /1/<0;1>/*
+    # for the recovery.
+    signer = SingleSigner()
+    fingerprint = xpub_fingerprint(signer.primary_hd)
+    xpub = signer.primary_hd.get_xpub()
+    csv_value = 10
+    main_desc = Descriptor.from_str(
+        f"wsh(or_d(pk([{fingerprint}]{xpub}/0/<0;1>/*),and_v(v:pkh([{fingerprint}]{xpub}/1/<0;1>/*),older({csv_value}))))"
+    )
+
+    lianad = Lianad(
+        datadir,
+        signer,
+        main_desc,
+        bitcoind.rpcport,
+        bitcoind_cookie,
+    )
+
+    try:
+        lianad.start()
+        yield lianad
+    except Exception:
+        lianad.cleanup()
+        raise
+
+    lianad.cleanup()
+
+
 def multi_expression(thresh, keys):
     exp = f"multi({thresh},"
     for i, key in enumerate(keys):

tests/test_framework/lianad.py

@@ -79,14 +79,14 @@ class Lianad(TailableProc):
                 int.from_bytes(raw_der_path[i : i + 4], byteorder="little", signed=True)
                 for i in range(0, len(raw_der_path), 4)
             ]
-            assert len(der_path) == 2
 
             # Create a copy of the descriptor to derive it at the index used in this input.
             # Then create a satisfaction for it using the signature we just created.
+            is_change = der_path[len(der_path) - 2] == 1
             desc = Descriptor.from_str(
-                str(self.receive_desc if der_path[0] == 0 else self.change_desc)
+                str(self.change_desc if is_change else self.receive_desc)
             )
-            desc.derive(der_path[1])
+            desc.derive(der_path[len(der_path) - 1])
             sat_material = SatisfactionMaterial(
                 signatures=psbt_in.map[PSBT_IN_PARTIAL_SIG],
             )

tests/test_rpc.py

@@ -398,14 +398,24 @@ def test_create_spend(lianad, bitcoind):
     res_spend = lianad.rpc.createspend(destinations, outpoints, 1)
     res_reco = lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2)
 
-    # The two PSBTs don't share any BIP32 derivation paths in their input.
+    # The two PSBTs don't share any BIP32 derivation paths in their inputs.
     res_spend_psbt = PSBT.from_base64(res_spend["psbt"])
+    res_spend_keys = set()
+    for i in res_spend_psbt.i:
+        res_spend_keys = res_spend_keys | set(i.map[PSBT_IN_BIP32_DERIVATION])
     res_reco_psbt = PSBT.from_base64(res_reco["psbt"])
-    res_spend_keys = set(res_spend_psbt.i[0].map[PSBT_IN_BIP32_DERIVATION])
-    res_reco_keys = set(res_reco_psbt.i[0].map[PSBT_IN_BIP32_DERIVATION])
+    res_reco_keys = set()
+    for i in res_reco_psbt.i:
+        res_reco_keys = res_reco_keys | set(i.map[PSBT_IN_BIP32_DERIVATION])
     assert res_spend_keys.intersection(res_reco_keys) == set()
 
 
+def test_create_spend_duplicate_signer(lianad_same_signer, bitcoind):
+    """Test spend creation (esp. around what bip32 derivations are added to the PSBT) but
+    with a Liana setup where a single signer is repeated between paths."""
+    test_create_spend(lianad_same_signer, bitcoind)
+
+
 def test_list_spend(lianad, bitcoind):
     # Start by creating two conflicting Spend PSBTs. The first one will have a change
     # output but not the second one.