From 6f5b053ea5af4737885a25573c82d627c299236e Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 27 Oct 2023 15:43:52 +0200 Subject: [PATCH] Revert "Merge #742: commands: don't add derivation paths for keys from different path but same signer" This reverts commit ec0c2426aa5fa6cee2efabd3ee6f175b41c35f64, reversing changes made to 26d750d09c84734f56c2dc18cb332a232e24fb6d. This reverts the fixes to the pruning of BIP32 derivation paths when creating a PSBT, in preparation of reverting the merge of this feature altogether. This is because always creating PSBT with only the BIP32 derivations for a single path broke the Ledger support. --- src/commands/mod.rs | 25 +++--------------------- tests/fixtures.py | 35 ---------------------------------- tests/test_framework/lianad.py | 6 +++--- tests/test_rpc.py | 16 +++------------- 4 files changed, 9 insertions(+), 73 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index cea453e1..06a998ca 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -26,7 +26,7 @@ use std::{ use miniscript::{ bitcoin::{ - self, address, bip32, + self, address, locktime::absolute, psbt::{Input as PsbtIn, Output as PsbtOut, PartiallySignedTransaction as Psbt}, }, @@ -237,25 +237,6 @@ 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 { @@ -389,7 +370,7 @@ impl DaemonControl { .primary_path() .thresh_origins(); let is_prim_path_key = - |pubkey: &keys::DerivedPublicKey| is_key_for_spending_path(pubkey, &prim_path_origins); + |pubkey: &keys::DerivedPublicKey| prim_path_origins.contains_key(&pubkey.origin.0); // 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 @@ -833,7 +814,7 @@ impl DaemonControl { .ok_or(CommandError::UnknownRecoveryTimelock(timelock))? .thresh_origins(); let is_key_for_this_path = - |pubkey: &keys::DerivedPublicKey| is_key_for_spending_path(pubkey, &reco_path_origins); + |pubkey: &keys::DerivedPublicKey| reco_path_origins.contains_key(&pubkey.origin.0); // 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. diff --git a/tests/fixtures.py b/tests/fixtures.py index 8827abf6..ee8d5b7c 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -156,41 +156,6 @@ 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_framework/lianad.py b/tests/test_framework/lianad.py index 0061ba5c..dfd81cd9 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.change_desc if is_change else self.receive_desc) + str(self.receive_desc if der_path[0] == 0 else self.change_desc) ) - desc.derive(der_path[len(der_path) - 1]) + desc.derive(der_path[1]) sat_material = SatisfactionMaterial( signatures=psbt_in.map[PSBT_IN_PARTIAL_SIG], ) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 4e420ca9..a2dbc614 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -398,24 +398,14 @@ 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 inputs. + # The two PSBTs don't share any BIP32 derivation paths in their input. 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_reco_keys = set() - for i in res_reco_psbt.i: - res_reco_keys = res_reco_keys | set(i.map[PSBT_IN_BIP32_DERIVATION]) + 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]) 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.