From a81d39c81a89cdf5e70b9888cbeb3abaf290a365 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 6 Oct 2023 17:57:17 +0200 Subject: [PATCH 1/3] commands: do not include BIP32 derivations for other spending paths When creating a PSBT, only include the BIP32 derivations in each input for the spending path this PSBT was created for. This is to workaround Bitbox only providing a single signature per input. Most likely other signing devices will have this behaviour too in the future. See https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808. --- doc/API.md | 12 +++++++-- src/commands/mod.rs | 58 ++++++++++++++++++++++++++++++++++++++---- src/descriptors/mod.rs | 16 +++++++++--- src/jsonrpc/mod.rs | 1 + src/signer.rs | 4 +-- 5 files changed, 79 insertions(+), 12 deletions(-) diff --git a/doc/API.md b/doc/API.md index 501562c1..35db17d1 100644 --- a/doc/API.md +++ b/doc/API.md @@ -287,8 +287,8 @@ Create a transaction that sweeps all coins for which a timelocked recovery path currently available to a provided address with the provided feerate. The `timelock` parameter can be used to specify which recovery path to use. By default, -we'll use the first recovery path available. If created for a later timelock a recovery -transaction may be satisfied using an earlier timelock but not the opposite. +we'll use the first recovery path available. The provided timelock must match the value of one +of the recovery path. Due to the fact coins are generally received at different block heights, not all coins may be spendable through a single recovery path at the same time. @@ -296,6 +296,14 @@ spendable through a single recovery path at the same time. This command will error if no such coins are available or the sum of their value is not enough to cover the requested feerate. +Note that although a transaction created for a later available timelock can be satisfied using the +keys for an earlier timelock, a PSBT created with this command will only be usable for a single +recovery path. If the user creates a PSBT for recovery path "B" and then ends up willing to satisfy +it using signatures for earlier recovery path "A", they will have to create a new recovery PSBT for +path "A" using `createrecovery`. This is because we do not include the information necessary to sign +using other recovery paths to workaround some signing devices' surprising behaviour. (See +https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for more details.) + #### Request | Field | Type | Description | diff --git a/src/commands/mod.rs b/src/commands/mod.rs index fb2c2b22..ff1f5345 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -7,7 +7,8 @@ mod utils; use crate::{ bitcoin::BitcoinInterface, database::{Coin, DatabaseInterface}, - descriptors, DaemonControl, VERSION, + descriptors::{self, keys}, + DaemonControl, VERSION, }; pub use crate::database::{CoinStatus, LabelItem}; @@ -71,6 +72,7 @@ pub enum CommandError { InsaneRescanTimestamp(u32), /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), + UnknownRecoveryTimelock(u16), RecoveryNotAvailable, } @@ -132,6 +134,7 @@ impl fmt::Display for CommandError { ), Self::InsaneRescanTimestamp(t) => write!(f, "Insane timestamp '{}'.", t), Self::RescanTrigger(s) => write!(f, "Error while starting rescan: '{}'", s), + Self::UnknownRecoveryTimelock(tl) => write!(f, "Provided timelock does not correspond to any recovery path: '{}'.", tl), Self::RecoveryNotAvailable => write!( f, "No coin currently spendable through this timelocked recovery path." @@ -345,6 +348,23 @@ impl DaemonControl { } let mut db_conn = self.db.connection(); + // Some signing devices (such as the Bitbox02) would not sign for all keys present in a + // script. This can lead to disruptions. For instance it could not provide a signature for + // the same key across a PSBT inputs. (See + // https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for details.) + // Therefore, only include in the PSBT_IN_BIP32_DERIVATION field the keys for the spending + // path we are interested in. Here the primary path. This guides the signer to sign for + // what we expect, and as long as the same signer isn't reused within the same spending + // path the signatures it will return will be consistent across inputs. + let (_, prim_path_origins) = self + .config + .main_descriptor + .policy() + .primary_path() + .thresh_origins(); + let is_prim_path_key = + |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 // list of coins is not empty. @@ -393,7 +413,7 @@ impl DaemonControl { script_pubkey: coin_desc.script_pubkey(), }); let non_witness_utxo = spent_txs.get(op).cloned(); - let bip32_derivation = coin_desc.bip32_derivations(); + let bip32_derivation = coin_desc.bip32_derivations(is_prim_path_key); psbt_ins.push(PsbtIn { witness_script, witness_utxo, @@ -428,7 +448,9 @@ impl DaemonControl { } else { self.config.main_descriptor.receive_descriptor() }; - desc.derive(index, &self.secp).bip32_derivations() + // NOTE: include all derivations for change outputs, to make sure signers are + // able to re-compute change addresses. + desc.derive(index, &self.secp).bip32_derivations(|_| true) } else { Default::default() }; @@ -506,8 +528,10 @@ impl DaemonControl { // TODO: shuffle once we have Taproot change_txo.value = change_amount.to_sat(); tx.output.push(change_txo); + // NOTE: include all derivations for change outputs, to make sure signers are + // able to re-compute change addresses. psbt_outs.push(PsbtOut { - bip32_derivation: change_desc.bip32_derivations(), + bip32_derivation: change_desc.bip32_derivations(|_| true), ..PsbtOut::default() }); } else if is_self_send { @@ -761,6 +785,30 @@ impl DaemonControl { .unwrap_or(false) }); + // Some signing devices (such as the Bitbox02) would not sign for all keys present in a + // script. This can lead to disruptions. For instance it could not provide a signature for + // the same key across a PSBT inputs. (See + // https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for details.) + // Therefore, only include in the PSBT_IN_BIP32_DERIVATION field the keys for the requested + // recovery path. This guides the signer toward signing only for this path, and as long as + // the same signer isn't reused within the same spending path the signatures it will return + // will be consistent across inputs. + // Note it's particularly unfortunate for a recovery PSBT. This is because absent this + // restriction a (set of) user(s) could satisfy any previously available paths (the primary + // path and any timelocked path which necessitates a lower sequence) to finalize a PSBT. + // Now they can only create and pass around a PSBT that can be used to sign for a single + // recovery path. + let (_, reco_path_origins) = self + .config + .main_descriptor + .policy() + .recovery_paths() + .get(&timelock) + .ok_or(CommandError::UnknownRecoveryTimelock(timelock))? + .thresh_origins(); + let is_key_for_this_path = + |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. let mut in_value = bitcoin::Amount::from_sat(0); @@ -793,7 +841,7 @@ impl DaemonControl { script_pubkey: coin_desc.script_pubkey(), }); let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); - let bip32_derivation = coin_desc.bip32_derivations(); + let bip32_derivation = coin_desc.bip32_derivations(is_key_for_this_path); psbt.inputs.push(PsbtIn { witness_script, witness_utxo, diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs index 574b153c..c5c50f50 100644 --- a/src/descriptors/mod.rs +++ b/src/descriptors/mod.rs @@ -378,7 +378,11 @@ impl DerivedSinglePathLianaDesc { self.0.explicit_script().expect("Not a Taproot descriptor") } - pub fn bip32_derivations(&self) -> Bip32Deriv { + /// Get the BIP32 derivations for this derived descriptor, filtered by the given predicate. + pub fn bip32_derivations

(&self, mut predicate: P) -> Bip32Deriv + where + P: FnMut(&DerivedPublicKey) -> bool, + { let ms = match self.0 { descriptor::Descriptor::Wsh(ref wsh) => match wsh.as_inner() { descriptor::WshInner::Ms(ms) => ms, @@ -391,7 +395,13 @@ impl DerivedSinglePathLianaDesc { // For DerivedPublicKey, Pk::Hash == Self. ms.iter_pk() - .map(|k| (k.key.inner, (k.origin.0, k.origin.1))) + .filter_map(|k| { + if predicate(&k) { + Some((k.key.inner, (k.origin.0, k.origin.1))) + } else { + None + } + }) .collect() } } @@ -630,7 +640,7 @@ mod tests { // Sanity check we can call the methods on the derived desc der_desc.script_pubkey(); der_desc.witness_script(); - assert!(!der_desc.bip32_derivations().is_empty()); + assert!(!der_desc.bip32_derivations(|_| true).is_empty()); } #[test] diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 6af7b9e8..0293113e 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -164,6 +164,7 @@ impl From for Error { | commands::CommandError::SpendFinalization(..) | commands::CommandError::InsaneRescanTimestamp(..) | commands::CommandError::AlreadyRescanning + | commands::CommandError::UnknownRecoveryTimelock(..) | commands::CommandError::RecoveryNotAvailable => { Error::new(ErrorCode::InvalidParams, e.to_string()) } diff --git a/src/signer.rs b/src/signer.rs index ebce7510..cc1f672b 100644 --- a/src/signer.rs +++ b/src/signer.rs @@ -456,7 +456,7 @@ mod tests { unknown: BTreeMap::new(), inputs: vec![PsbtIn { witness_script: Some(spent_coin_desc.witness_script()), - bip32_derivation: spent_coin_desc.bip32_derivations(), + bip32_derivation: spent_coin_desc.bip32_derivations(|_| true), witness_utxo: Some(bitcoin::TxOut { value: 19_000, script_pubkey: spent_coin_desc.script_pubkey(), @@ -499,7 +499,7 @@ mod tests { let other_spent_coin_desc = desc.receive_descriptor().derive(84.into(), &secp); dummy_psbt.inputs.push(PsbtIn { witness_script: Some(other_spent_coin_desc.witness_script()), - bip32_derivation: other_spent_coin_desc.bip32_derivations(), + bip32_derivation: other_spent_coin_desc.bip32_derivations(|_| true), witness_utxo: Some(bitcoin::TxOut { value: 19_000, script_pubkey: other_spent_coin_desc.script_pubkey(), From b71bd693d6ea4c1c3567194fc82be43fb70c05bb Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 6 Oct 2023 18:17:45 +0200 Subject: [PATCH 2/3] qa: don't use a static dummy origin for descriptor xpubs --- tests/fixtures.py | 20 +++++++++--- tests/test_misc.py | 77 +++++++++++++++++++++++++--------------------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index 1c4c8624..ee8d5b7c 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -1,4 +1,5 @@ from bip32 import BIP32 +from bip32.utils import _pubkey_to_fingerprint from bip380.descriptors import Descriptor from concurrent import futures from test_framework.bitcoind import Bitcoind @@ -115,6 +116,10 @@ def bitcoind(directory): bitcoind.cleanup() +def xpub_fingerprint(hd): + return _pubkey_to_fingerprint(hd.pubkey).hex() + + @pytest.fixture def lianad(bitcoind, directory): datadir = os.path.join(directory, "lianad") @@ -122,13 +127,15 @@ def lianad(bitcoind, directory): bitcoind_cookie = os.path.join(bitcoind.bitcoin_dir, "regtest", ".cookie") signer = SingleSigner() - primary_xpub, recovery_xpub = ( - signer.primary_hd.get_xpub(), - signer.recovery_hd.get_xpub(), + (prim_fingerprint, primary_xpub), (reco_fingerprint, recovery_xpub) = ( + (xpub_fingerprint(signer.primary_hd), signer.primary_hd.get_xpub()), + (xpub_fingerprint(signer.recovery_hd), signer.recovery_hd.get_xpub()), ) csv_value = 10 + # NOTE: origins are the actual xpub themselves which is incorrect but make it + # possible to differentiate them. main_desc = Descriptor.from_str( - f"wsh(or_d(pk([aabbccdd]{primary_xpub}/<0;1>/*),and_v(v:pkh([aabbccdd]{recovery_xpub}/<0;1>/*),older({csv_value}))))" + f"wsh(or_d(pk([{prim_fingerprint}]{primary_xpub}/<0;1>/*),and_v(v:pkh([{reco_fingerprint}]{recovery_xpub}/<0;1>/*),older({csv_value}))))" ) lianad = Lianad( @@ -152,7 +159,10 @@ def lianad(bitcoind, directory): def multi_expression(thresh, keys): exp = f"multi({thresh}," for i, key in enumerate(keys): - exp += f"[aabbccdd]{key.get_xpub()}/<0;1>/*" + # NOTE: origins are the actual xpub themselves which is incorrect but make it + # possible to differentiate them. + fingerprint = xpub_fingerprint(key) + exp += f"[{fingerprint}]{key.get_xpub()}/<0;1>/*" if i != len(keys) - 1: exp += "," return exp + ")" diff --git a/tests/test_misc.py b/tests/test_misc.py index 55841759..a1b23e6b 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -127,11 +127,16 @@ def test_multipath(lianad_multipath, bitcoind): reco_psbt = PSBT.from_base64(res["psbt"]) txid = reco_psbt.tx.txid().hex() + # NOTE: this test was commented out due to the introduced restriction to not include + # the BIP32 derivations for other spending paths in PSBT inputs to support the Bitbox2 + # signing device (and most likely others). + # TODO: reintroduce these tests once we get rid of this restriction. + # Try to sign with the keys for the next recovery spending path, it'll fail. - signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {20: range(3)}) - lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) - with pytest.raises(RpcError, match="Failed to finalize"): - lianad_multipath.rpc.broadcastspend(txid) + # signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {20: range(3)}) + # lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) + # with pytest.raises(RpcError, match="Failed to finalize"): + # lianad_multipath.rpc.broadcastspend(txid) # Try to sign with the right keys but only two of them, it'll fail. signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {10: range(2)}) @@ -144,52 +149,54 @@ def test_multipath(lianad_multipath, bitcoind): lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) lianad_multipath.rpc.broadcastspend(txid) + # NOTE: commented out for the same reason as above. + # Receive 3 more coins and make the second recovery path (20 blocks) available. - txids = [] - for _ in range(3): - addr = lianad_multipath.rpc.getnewaddress()["address"] - txids.append(bitcoind.rpc.sendtoaddress(addr, 0.42)) - bitcoind.generate_block(20, wait_for_mempool=txids) - wait_for( - lambda: lianad_multipath.rpc.getinfo()["block_height"] - == bitcoind.rpc.getblockcount() - ) + # txids = [] + # for _ in range(3): + # addr = lianad_multipath.rpc.getnewaddress()["address"] + # txids.append(bitcoind.rpc.sendtoaddress(addr, 0.42)) + # bitcoind.generate_block(20, wait_for_mempool=txids) + # wait_for( + # lambda: lianad_multipath.rpc.getinfo()["block_height"] + # == bitcoind.rpc.getblockcount() + # ) # We can create a recovery transaction for an earlier timelock. - lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2) + # lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2) # Sweep all coins through the second recovery path (that is available after 20 blocks). # It needs 3 signatures out of 5 keys. - res = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 20) - reco_psbt = PSBT.from_base64(res["psbt"]) - txid = reco_psbt.tx.txid().hex() + # res = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 20) + # reco_psbt = PSBT.from_base64(res["psbt"]) + # txid = reco_psbt.tx.txid().hex() # We can sign with any keys for the second recovery path (we need only 1 out of 10) - signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {20: [8]}) - lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) - lianad_multipath.rpc.broadcastspend(txid) + # signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {20: [8]}) + # lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) + # lianad_multipath.rpc.broadcastspend(txid) # Now do this again but with signing using keys for the first recovery path. # Receive 3 more coins and make the second recovery path (20 blocks) available. Note this # is possible since the CSV checks the nSequence is >= to the value, not ==. - txids = [] - for _ in range(3): - addr = lianad_multipath.rpc.getnewaddress()["address"] - txids.append(bitcoind.rpc.sendtoaddress(addr, 0.398)) - bitcoind.generate_block(20, wait_for_mempool=txids) - wait_for( - lambda: lianad_multipath.rpc.getinfo()["block_height"] - == bitcoind.rpc.getblockcount() - ) + # txids = [] + # for _ in range(3): + # addr = lianad_multipath.rpc.getnewaddress()["address"] + # txids.append(bitcoind.rpc.sendtoaddress(addr, 0.398)) + # bitcoind.generate_block(20, wait_for_mempool=txids) + # wait_for( + # lambda: lianad_multipath.rpc.getinfo()["block_height"] + # == bitcoind.rpc.getblockcount() + # ) # Sweep all coins through the second recovery path (that is available after 20 blocks). # It needs 3 signatures out of 5 keys. - res = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 20) - reco_psbt = PSBT.from_base64(res["psbt"]) - txid = reco_psbt.tx.txid().hex() + # res = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 20) + # reco_psbt = PSBT.from_base64(res["psbt"]) + # txid = reco_psbt.tx.txid().hex() # We can sign with keys for the first recovery path (we need 3 out of 5) - signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {10: range(2, 5)}) - lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) - lianad_multipath.rpc.broadcastspend(txid) + # signed_psbt = lianad_multipath.signer.sign_psbt(reco_psbt, {10: range(2, 5)}) + # lianad_multipath.rpc.updatespend(signed_psbt.to_base64()) + # lianad_multipath.rpc.broadcastspend(txid) def test_coinbase_deposit(lianad, bitcoind): From 7f3b0b021858cfb2fe914f3ba6b30a39e3ae05ff Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 6 Oct 2023 18:20:42 +0200 Subject: [PATCH 3/3] qa: test a PSBT has only the BIP32 derivations for a single spending path --- tests/test_rpc.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 3a2dcc2e..193088fa 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -6,6 +6,7 @@ import time from fixtures import * from test_framework.serializations import ( PSBT, + PSBT_IN_BIP32_DERIVATION, PSBT_IN_PARTIAL_SIG, PSBT_IN_NON_WITNESS_UTXO, ) @@ -385,6 +386,25 @@ def test_create_spend(lianad, bitcoind): with pytest.raises(RpcError, match=".*is from an immature coinbase transaction."): lianad.rpc.createspend(destinations, [imma_coin["outpoint"]], 1) + # Receive a coin and make it immediately available for the recovery path. + txid = bitcoind.rpc.sendtoaddress(lianad.rpc.getnewaddress()["address"], 1) + bitcoind.generate_block(10, wait_for_mempool=txid) + wait_for( + lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() + ) + + # Create both a spend transaction and recovery transaction spending this coin. + outpoints = [c["outpoint"] for c in lianad.rpc.listcoins(["confirmed"])["coins"]] + 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. + res_spend_psbt = PSBT.from_base64(res_spend["psbt"]) + 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]) + assert res_spend_keys.intersection(res_reco_keys) == set() + def test_list_spend(lianad, bitcoind): # Start by creating two conflicting Spend PSBTs. The first one will have a change @@ -841,6 +861,39 @@ def test_create_recovery(lianad, bitcoind): sign_and_broadcast(lianad, bitcoind, reco_psbt, recovery=True) +def test_create_recovery_specific_paths(lianad_multipath, bitcoind): + """Test creating recovery PSBTs for specific recovery paths.""" + # We can't create a recovery for a specific recovery path without specifying the precise + # timelock value of this recovery path. + with pytest.raises( + RpcError, + match="Provided timelock does not correspond to any recovery path: '42424'", + ): + lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 42424) + + # Receive a coin and make it immediately available for both reco paths. + txid = bitcoind.rpc.sendtoaddress( + lianad_multipath.rpc.getnewaddress()["address"], 1 + ) + bitcoind.generate_block(20, wait_for_mempool=txid) + wait_for( + lambda: lianad_multipath.rpc.getinfo()["block_height"] + == bitcoind.rpc.getblockcount() + ) + + # But we can create one for both existing recovery paths. + res_10 = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 10) + res_20 = lianad_multipath.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 20) + + # Both don't have the same BIP32 derivations set in their input, unfortunately. This + # is because we only set them for the keys from a specific spending path. + res_10_psbt = PSBT.from_base64(res_10["psbt"]) + res_20_psbt = PSBT.from_base64(res_20["psbt"]) + res_10_keys = set(res_10_psbt.i[0].map[PSBT_IN_BIP32_DERIVATION]) + res_20_keys = set(res_20_psbt.i[0].map[PSBT_IN_BIP32_DERIVATION]) + assert res_10_keys.intersection(res_20_keys) == set() + + def test_labels(lianad, bitcoind): """Test the creation and updating of labels.""" # We can set a label for an address.