From 24edaecbdcfc0b6b53ac3355ad22991a8b5de018 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 24 Oct 2023 15:44:40 +0200 Subject: [PATCH 1/4] commands: don't add der paths for keys from diff path but same signer 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. --- src/commands/mod.rs | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 06a998ca..cea453e1 100644 --- a/src/commands/mod.rs +++ b/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: &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>, +) -> 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. From cf33228b0d25e79ec8fa855aaac6ebfa5f10da12 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 24 Oct 2023 16:14:41 +0200 Subject: [PATCH 2/4] qa: don't assume desc xpubs' der path length in finalizer It's always 2 for now, but we are going to break this invariant. Make this code more robust. --- tests/test_framework/lianad.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_framework/lianad.py b/tests/test_framework/lianad.py index dfd81cd9..0061ba5c 100644 --- a/tests/test_framework/lianad.py +++ b/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], ) From 8d213d5e31dd47f0c179475e32f5eb86c3773770 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 24 Oct 2023 16:38:35 +0200 Subject: [PATCH 3/4] qa: correct create_spend functional test Both transactions may not spend the same coins. We need to compare the set of all der paths across inputs. --- tests/test_rpc.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index a2dbc614..3bb82c3a 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -398,11 +398,15 @@ 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() From 2e1c54491e96fda79b3831dc0c079c59250fabe0 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 24 Oct 2023 16:15:52 +0200 Subject: [PATCH 4/4] qa: test der paths in PSBT when desc has duplicate signer --- tests/fixtures.py | 35 +++++++++++++++++++++++++++++++++++ tests/test_rpc.py | 6 ++++++ 2 files changed, 41 insertions(+) diff --git a/tests/fixtures.py b/tests/fixtures.py index ee8d5b7c..8827abf6 100644 --- a/tests/fixtures.py +++ b/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): diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 3bb82c3a..4e420ca9 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -410,6 +410,12 @@ def test_create_spend(lianad, bitcoind): 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.