From 652eb5b799c8d2ed9f65ab24c0101af3e09905f8 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 30 Dec 2024 10:28:18 +0000 Subject: [PATCH 1/2] refactor(bitcoind): extract logic to function --- lianad/src/bitcoin/d/mod.rs | 117 ++++++++++++++++++++++++++++++++++-- 1 file changed, 112 insertions(+), 5 deletions(-) diff --git a/lianad/src/bitcoin/d/mod.rs b/lianad/src/bitcoin/d/mod.rs index a7b088a33..aabef152c 100644 --- a/lianad/src/bitcoin/d/mod.rs +++ b/lianad/src/bitcoin/d/mod.rs @@ -1060,11 +1060,7 @@ impl BitcoinD { let current_descs = self.list_descriptors(); for desc in descs { - let present = current_descs - .iter() - .find(|entry| &entry.desc == desc) - .map(|entry| entry.timestamp == timestamp) - .unwrap_or(false); + let present = current_descs_contain_desc_timestamp(¤t_descs, desc, timestamp); if !present { return false; } @@ -1281,6 +1277,19 @@ pub struct ListDescEntry { pub timestamp: u32, } +/// Whether `current_descs` contain the descriptor `desc` at `timestamp`. +fn current_descs_contain_desc_timestamp( + current_descs: &[ListDescEntry], + desc: &String, + timestamp: u32, +) -> bool { + current_descs + .iter() + .find(|entry| &entry.desc == desc) + .map(|entry| entry.timestamp == timestamp) + .unwrap_or(false) +} + /// A 'received' entry in the 'listsinceblock' result. #[derive(Debug, Clone)] pub struct LSBlockEntry { @@ -1592,4 +1601,102 @@ mod tests { 0.9999 ); } + + #[test] + fn test_current_descs_contain_desc_timestamp() { + // For simplicity, I've removed the checksums in the following descriptors as these will change + // depending on whether `h` or `'` is used. + + // Simulate that `listdescriptors` returns an entry for receive and change descriptors, respectively. + // Include one of the descriptors with two different timestamps and another invalid descriptor. + let current_descs = vec![ + ListDescEntry { + desc: "this is not a descriptor and will be ignored".to_string(), + range: Some([0, 999]), + timestamp: 1598918410, + }, + ListDescEntry { + desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(), + range: Some([0, 999]), + timestamp: 1598918400, + }, + ListDescEntry { + desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(), + range: Some([0, 999]), + timestamp: 1598918380, + }, + // same as receive descriptor above but with different timestamp. + ListDescEntry { + desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(), + range: Some([0, 999]), + timestamp: 1598918410, + }, + ]; + + // These are the corresponding descriptors from the Liana wallet, using only `'`. + let recv_desc = "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(); + let change_desc = "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(); + + // For the receive descriptor, we don't get a match unless the timestamp matches the first occurrence. + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &recv_desc, + 1598918399 + )); + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &recv_desc, + 1598918401 + )); + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &change_desc, + 1598918381 + )); + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &recv_desc, + 1598918410 // this is the second timestamp for this descriptor + )); + // We only get a match when we use the first timestamp for each descriptor. + assert!(current_descs_contain_desc_timestamp( + ¤t_descs, + &recv_desc, + 1598918400 + )); + assert!(current_descs_contain_desc_timestamp( + ¤t_descs, + &change_desc, + 1598918380 + )); + + // If the `listdescriptors` response contains a mix of `h` and `'`, then there is no match. + let current_descs = vec![ + ListDescEntry { + desc: "this is not a descriptor and will be ignored".to_string(), + range: Some([0, 999]), + timestamp: 1598918410, + }, + ListDescEntry { + desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(), + range: Some([0, 999]), + timestamp: 1598918400, + }, + ListDescEntry { + desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(), + range: Some([0, 999]), + timestamp: 1598918380, + }, + ]; + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &recv_desc, + 1598918400 + )); + assert!(!current_descs_contain_desc_timestamp( + ¤t_descs, + &change_desc, + 1598918380 + )); + } } From 6b914fd2491841bb3daa41f59641677c10cc880b Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 30 Dec 2024 10:47:20 +0000 Subject: [PATCH 2/2] bitcoind: parse descriptor before comparing strings Parsing the descriptor first will ensure a match is found regardless of whether bitcoind uses `h` or `'`. --- lianad/src/bitcoin/d/mod.rs | 100 ++++++++++++++++++++++++++---------- tests/fixtures.py | 57 ++++++++++++++++++-- tests/test_rpc.py | 8 +++ 3 files changed, 136 insertions(+), 29 deletions(-) diff --git a/lianad/src/bitcoin/d/mod.rs b/lianad/src/bitcoin/d/mod.rs index aabef152c..d73209968 100644 --- a/lianad/src/bitcoin/d/mod.rs +++ b/lianad/src/bitcoin/d/mod.rs @@ -29,7 +29,7 @@ use jsonrpc::{ use miniscript::{ bitcoin::{self, address, hashes::hex::FromHex}, - descriptor, + descriptor::{self, Descriptor, DescriptorPublicKey}, }; use serde_json::Value as Json; @@ -1056,7 +1056,11 @@ impl BitcoinD { // For the given descriptor strings check if they are imported at this timestamp in the // watchonly wallet. - fn check_descs_timestamp(&self, descs: &[String], timestamp: u32) -> bool { + fn check_descs_timestamp( + &self, + descs: &[&Descriptor], + timestamp: u32, + ) -> bool { let current_descs = self.list_descriptors(); for desc in descs { @@ -1108,15 +1112,15 @@ impl BitcoinD { .fold(1_000, |range, entry| { cmp::max(range, entry.range.map(|r| r[1]).unwrap_or(0)) }); - let desc_str = [ - desc.receive_descriptor().to_string(), - desc.change_descriptor().to_string(), + let descs = [ + desc.receive_descriptor().as_descriptor_public_key(), + desc.change_descriptor().as_descriptor_public_key(), ]; - let desc_json: Vec = desc_str + let desc_json: Vec = descs .iter() - .map(|desc_str| { + .map(|desc| { serde_json::json!({ - "desc": desc_str, + "desc": desc.to_string(), "timestamp": timestamp, "active": false, "range": max_range, @@ -1150,7 +1154,7 @@ impl BitcoinD { } i += 1; - if self.check_descs_timestamp(&desc_str, timestamp) { + if self.check_descs_timestamp(&descs, timestamp) { return Ok(()); } else if i >= NUM_RETRIES { return Err(BitcoindError::StartRescan); @@ -1278,15 +1282,25 @@ pub struct ListDescEntry { } /// Whether `current_descs` contain the descriptor `desc` at `timestamp`. +/// +/// Any descriptors in `current_descs` that cannot be parsed as +/// `Descriptor::` will be ignored. fn current_descs_contain_desc_timestamp( current_descs: &[ListDescEntry], - desc: &String, + desc: &Descriptor, timestamp: u32, ) -> bool { current_descs .iter() - .find(|entry| &entry.desc == desc) - .map(|entry| entry.timestamp == timestamp) + .filter_map(|entry| { + if let Ok(entry_desc) = Descriptor::::from_str(&entry.desc) { + Some((entry_desc, entry.timestamp)) + } else { + None + } + }) + .find(|(entry_desc, _)| entry_desc.to_string() == desc.to_string()) + .map(|(_, entry_timestamp)| entry_timestamp == timestamp) .unwrap_or(false) } @@ -1633,44 +1647,49 @@ mod tests { }, ]; - // These are the corresponding descriptors from the Liana wallet, using only `'`. - let recv_desc = "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(); - let change_desc = "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(); + // Create the Liana wallet descriptor: + let desc = LianaDescriptor::from_str("tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/<0;1>/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/<2;3>/*),older(65535)))").unwrap(); + // The receive and change descriptors contain only `'`: + assert_eq!(desc.receive_descriptor().to_string(), "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn".to_string()); + assert_eq!(desc.change_descriptor().to_string(), "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))#6yyu2dsu".to_string()); + + let recv_desc = desc.receive_descriptor().as_descriptor_public_key(); + let change_desc = desc.change_descriptor().as_descriptor_public_key(); // For the receive descriptor, we don't get a match unless the timestamp matches the first occurrence. assert!(!current_descs_contain_desc_timestamp( ¤t_descs, - &recv_desc, + recv_desc, 1598918399 )); assert!(!current_descs_contain_desc_timestamp( ¤t_descs, - &recv_desc, + recv_desc, 1598918401 )); assert!(!current_descs_contain_desc_timestamp( ¤t_descs, - &change_desc, + change_desc, 1598918381 )); assert!(!current_descs_contain_desc_timestamp( ¤t_descs, - &recv_desc, + recv_desc, 1598918410 // this is the second timestamp for this descriptor )); // We only get a match when we use the first timestamp for each descriptor. assert!(current_descs_contain_desc_timestamp( ¤t_descs, - &recv_desc, + recv_desc, 1598918400 )); assert!(current_descs_contain_desc_timestamp( ¤t_descs, - &change_desc, + change_desc, 1598918380 )); - // If the `listdescriptors` response contains a mix of `h` and `'`, then there is no match. + // If the `listdescriptors` response contains a mix of `h` and `'`, then there is still a match. let current_descs = vec![ ListDescEntry { desc: "this is not a descriptor and will be ignored".to_string(), @@ -1688,14 +1707,43 @@ mod tests { timestamp: 1598918380, }, ]; - assert!(!current_descs_contain_desc_timestamp( + assert!(current_descs_contain_desc_timestamp( ¤t_descs, - &recv_desc, + recv_desc, 1598918400 )); - assert!(!current_descs_contain_desc_timestamp( + assert!(current_descs_contain_desc_timestamp( + ¤t_descs, + change_desc, + 1598918380 + )); + + // There is still a match if the checksum is included in the `listdescriptors` response. + let current_descs = vec![ + ListDescEntry { + desc: "this is not a descriptor and will be ignored".to_string(), + range: Some([0, 999]), + timestamp: 1598918410, + }, + ListDescEntry { + desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#2h7g2wme".to_string(), + range: Some([0, 999]), + timestamp: 1598918400, + }, + ListDescEntry { + desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))#kyer0m8k".to_string(), + range: Some([0, 999]), + timestamp: 1598918380, + }, + ]; + assert!(current_descs_contain_desc_timestamp( + ¤t_descs, + recv_desc, + 1598918400 + )); + assert!(current_descs_contain_desc_timestamp( ¤t_descs, - &change_desc, + change_desc, 1598918380 )); } diff --git a/tests/fixtures.py b/tests/fixtures.py index dc2860a88..00e2f6d3f 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -145,11 +145,20 @@ def xpub_fingerprint(hd): return _pubkey_to_fingerprint(hd.pubkey).hex() -def single_key_desc(prim_fg, prim_xpub, reco_fg, reco_xpub, csv_value, is_taproot): +def single_key_desc( + prim_fg, + prim_xpub, + reco_fg, + reco_xpub, + csv_value, + is_taproot, + prim_deriv_path="", + reco_deriv_path="", +): if is_taproot: - return f"tr([{prim_fg}]{prim_xpub}/<0;1>/*,and_v(v:pk([{reco_fg}]{reco_xpub}/<0;1>/*),older({csv_value})))" + return f"tr([{prim_fg}{prim_deriv_path}]{prim_xpub}/<0;1>/*,and_v(v:pk([{reco_fg}{reco_deriv_path}]{reco_xpub}/<0;1>/*),older({csv_value})))" else: - return f"wsh(or_d(pk([{prim_fg}]{prim_xpub}/<0;1>/*),and_v(v:pkh([{reco_fg}]{reco_xpub}/<0;1>/*),older({csv_value}))))" + return f"wsh(or_d(pk([{prim_fg}{prim_deriv_path}]{prim_xpub}/<0;1>/*),and_v(v:pkh([{reco_fg}{reco_deriv_path}]{reco_xpub}/<0;1>/*),older({csv_value}))))" @pytest.fixture @@ -193,6 +202,48 @@ def lianad(bitcoin_backend, directory): lianad.cleanup() +# This can currently only be used with Taproot if no signing is required. +@pytest.fixture +def lianad_with_deriv_paths(bitcoin_backend, directory): + datadir = os.path.join(directory, "lianad") + os.makedirs(datadir, exist_ok=True) + + signer = SingleSigner(is_taproot=USE_TAPROOT) + (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 + main_desc = Descriptor.from_str( + single_key_desc( + prim_fingerprint, + primary_xpub, + reco_fingerprint, + recovery_xpub, + csv_value, + USE_TAPROOT, + "/48h/1h/0h/2h", + "/46h/12h/10h/72h", + ) + ) + + lianad = Lianad( + datadir, + signer, + main_desc, + bitcoin_backend, + ) + + try: + lianad.start() + yield lianad + except Exception: + lianad.cleanup() + raise + + lianad.cleanup() + + def unspendable_internal_xpub(xpubs): """Deterministic, unique, unspendable internal key. See See https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304/21 diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 92084c270..0fd539889 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -619,6 +619,14 @@ def test_broadcast_spend(lianad, bitcoind): lianad.rpc.broadcastspend(txid) +# Use a descriptor that includes hardened derivation paths so that we can check +# there is no problem regarding the use of `h` and `'`. +def test_start_rescan_does_not_error(lianad_with_deriv_paths, bitcoind): + """Test we can successfully start a rescan.""" + tip_timestamp = bitcoind.rpc.getblockheader(bitcoind.rpc.getbestblockhash())["time"] + lianad_with_deriv_paths.rpc.startrescan(tip_timestamp - 1) + + def test_start_rescan(lianad, bitcoind): """Test we successfully retrieve all our transactions after losing state by rescanning.""" initial_timestamp = int(time.time())