From 3105b86a28444097e20e14261ffbfd5448b2854b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Oct 2022 18:33:09 +0200 Subject: [PATCH 01/14] Use my own fork of rust-miniscript It supports multipath descriptors. We'll need to find a solution for the release --- Cargo.lock | 3 +-- Cargo.toml | 3 ++- src/descriptors.rs | 16 +++++++++++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dea18ca3..12079160 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -256,8 +256,7 @@ dependencies = [ [[package]] name = "miniscript" version = "8.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f4975078076f0b7b914a3044ad7432d2a7fcec38edb855afdc672e24ca35b69" +source = "git+https://github.com/darosior/rust-miniscript?branch=multipath_descriptors_on_8.0#7d756f2ab066d85d299f711f953ebda15f14e832" dependencies = [ "bitcoin", "serde", diff --git a/Cargo.toml b/Cargo.toml index 35a61ff4..36033c17 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,8 @@ jsonrpc_server = [] [dependencies] # For managing transactions (it re-exports the bitcoin crate) -miniscript = { version = "8.0", features = ["serde"] } +# TODO: don't use my fork for a real release.. +miniscript = { git = "https://github.com/darosior/rust-miniscript", branch = "multipath_descriptors_on_8.0", features = ["serde"] } # Don't reinvent the wheel dirs = "3.0" diff --git a/src/descriptors.rs b/src/descriptors.rs index 4336903c..8a595f46 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -125,6 +125,10 @@ impl MiniscriptKey for DerivedPublicKey { fn is_x_only_key(&self) -> bool { false } + + fn num_der_paths(&self) -> usize { + 0 + } } impl ToPublicKey for DerivedPublicKey { @@ -164,7 +168,8 @@ fn csv_check(csv_value: u32) -> Result<(), DescCreationError> { fn is_unhardened_deriv(key: &descriptor::DescriptorPublicKey) -> bool { match *key { - descriptor::DescriptorPublicKey::Single(..) => false, + descriptor::DescriptorPublicKey::Single(..) + | descriptor::DescriptorPublicKey::MultiXPub(..) => false, descriptor::DescriptorPublicKey::XPub(ref xpub) => { xpub.wildcard == descriptor::Wildcard::Unhardened } @@ -342,10 +347,15 @@ impl InheritanceDescriptor { &mut self, pk: &descriptor::DescriptorPublicKey, ) -> Result { - let definite_key = pk.clone().at_derivation_index(self.0); + let definite_key = pk + .clone() + .at_derivation_index(self.0) + .expect("We disallow multipath keys."); let origin = ( definite_key.master_fingerprint(), - definite_key.full_derivation_path(), + definite_key + .full_derivation_path() + .expect("We disallow multipath keys."), ); let key = definite_key.derive_public_key(self.1)?; Ok(DerivedPublicKey { origin, key }) From 846d924792089e41e530e414b26f0823afc151eb Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 14:59:59 +0200 Subject: [PATCH 02/14] qa: upgrade python-bip380 to latest master For multipath descriptors support --- tests/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/requirements.txt b/tests/requirements.txt index 34e9dc94..ce153082 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -4,4 +4,4 @@ pytest-timeout==1.3.4 ephemeral_port_reserve==1.1.1 bip32~=3.0 -bip380==0.0.3 +https://github.com/darosior/python-bip380/archive/f25eb2add9a5d461e382635231a5f971652fc8e1.zip From f985fd787917e344b17ae90edd7b99bc1c9f3a7c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 21 Oct 2022 13:12:23 +0200 Subject: [PATCH 03/14] descriptors: remove as_inner method We'll change the semantic of the descriptor, so we need to make sure nothing accesses it with the old semantic. --- src/config.rs | 11 ++--------- src/descriptors.rs | 14 +++++++++++--- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/config.rs b/src/config.rs index f4ef1ca3..a873abf4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,7 +2,7 @@ use crate::descriptors::InheritanceDescriptor; use std::{net::SocketAddr, path::PathBuf, str::FromStr, time::Duration}; -use miniscript::{bitcoin::Network, DescriptorPublicKey, ForEachKey}; +use miniscript::bitcoin::Network; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; @@ -203,14 +203,7 @@ impl Config { Network::Bitcoin => Network::Bitcoin, _ => Network::Testnet, }; - let unexpected_net = self.main_descriptor.as_inner().for_each_key(|xpub| { - if let DescriptorPublicKey::XPub(xpub) = xpub { - xpub.xkey.network != expected_network - } else { - false - } - }); - if unexpected_net { + if !self.main_descriptor.all_xpubs_net_is(expected_network) { return Err(ConfigError::Unexpected(format!( "Our bitcoin network is {} but one xpub is not for network {}", self.bitcoin_config.network, expected_network diff --git a/src/descriptors.rs b/src/descriptors.rs index 8a595f46..a39ed120 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -9,7 +9,8 @@ use miniscript::{ descriptor, hash256, miniscript::{decode::Terminal, Miniscript}, policy::{Liftable, Semantic as SemanticPolicy}, - translate_hash_clone, MiniscriptKey, ScriptContext, ToPublicKey, TranslatePk, Translator, + translate_hash_clone, ForEachKey, MiniscriptKey, ScriptContext, ToPublicKey, TranslatePk, + Translator, }; use std::{collections::BTreeMap, convert::TryFrom, error, fmt, str, sync}; @@ -315,8 +316,15 @@ impl InheritanceDescriptor { ))) } - pub fn as_inner(&self) -> &descriptor::Descriptor { - &self.0 + /// Whether all xpubs contained in this descriptor are for the passed expected network. + pub fn all_xpubs_net_is(&self, expected_net: bitcoin::Network) -> bool { + self.0.for_each_key(|xpub| { + if let descriptor::DescriptorPublicKey::XPub(xpub) = xpub { + xpub.xkey.network == expected_net + } else { + false + } + }) } /// Derive this descriptor at a given index. From caaca1fd1a721acae150b8bd9212e51b8e378c99 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 21 Oct 2022 13:53:44 +0200 Subject: [PATCH 04/14] descriptors: rename derive into derive_received We'll be able to derive change addresses too --- src/commands/mod.rs | 8 ++++---- src/database/sqlite/mod.rs | 8 ++++---- src/database/sqlite/utils.rs | 2 +- src/descriptors.rs | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index db563993..9805331a 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -172,7 +172,9 @@ fn serializable_size(t: &T) -> u64 { impl DaemonControl { // Get the descriptor at this derivation index fn derived_desc(&self, index: bip32::ChildNumber) -> descriptors::DerivedInheritanceDescriptor { - self.config.main_descriptor.derive(index, &self.secp) + self.config + .main_descriptor + .derive_receive(index, &self.secp) } } @@ -201,9 +203,7 @@ impl DaemonControl { // TODO: should we wrap around instead of failing? db_conn.increment_derivation_index(&self.secp); let address = self - .config - .main_descriptor - .derive(index, &self.secp) + .derived_desc(index) .address(self.config.bitcoin_config.network); GetAddressResult { address } } diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index d9489d59..c0178fd2 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -240,7 +240,7 @@ impl SqliteConn { let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; let next_la_address = db_wallet .main_descriptor - .derive(next_la_index.into(), secp) + .derive_receive(next_la_index.into(), secp) .address(network); db_tx .execute( @@ -714,7 +714,7 @@ mod tests { // There is the index for the first index let addr = options .main_descriptor - .derive(0.into(), &secp) + .derive_receive(0.into(), &secp) .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 0.into()); @@ -722,7 +722,7 @@ mod tests { // There is the index for the 199th index (look-ahead limit) let addr = options .main_descriptor - .derive(199.into(), &secp) + .derive_receive(199.into(), &secp) .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 199.into()); @@ -730,7 +730,7 @@ mod tests { // And not for the 200th one. let addr = options .main_descriptor - .derive(200.into(), &secp) + .derive_receive(200.into(), &secp) .address(options.bitcoind_network); assert!(conn.db_address(&addr).is_none()); diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index b7cb6218..c9c0cd0a 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -98,7 +98,7 @@ pub fn create_fresh_db( // TODO: have this as a helper in descriptors.rs let address = options .main_descriptor - .derive(index.into(), secp) + .derive_receive(index.into(), secp) .address(options.bitcoind_network); query += &format!( "INSERT INTO addresses (address, derivation_index) VALUES (\"{}\", {});\n", diff --git a/src/descriptors.rs b/src/descriptors.rs index a39ed120..05974816 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -327,11 +327,11 @@ impl InheritanceDescriptor { }) } - /// Derive this descriptor at a given index. + /// Derive this descriptor at a given index for a receiving address. /// /// # Panics /// - If the given index is hardened. - pub fn derive( + pub fn derive_receive( &self, index: bip32::ChildNumber, secp: &secp256k1::Secp256k1, @@ -504,7 +504,7 @@ mod tests { fn inheritance_descriptor_derivation() { let secp = secp256k1::Secp256k1::verification_only(); let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2d").unwrap(); - let der_desc = desc.derive(11.into(), &secp); + let der_desc = desc.derive_receive(11.into(), &secp); assert_eq!( "bc1qvjzcg25nsxmfccct0txjvljxjwn68htkrw57jqmjhfzvhyd2z4msc74w65", der_desc.address(bitcoin::Network::Bitcoin).to_string() From ba4c1e0383e302d2a822176d93b3b9fadc174b4b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 21 Oct 2022 14:50:53 +0200 Subject: [PATCH 05/14] bitcoind: include change outputs in listsinceblock --- src/bitcoin/d/mod.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index c3db160e..4de7a4bb 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -528,7 +528,13 @@ impl BitcoinD { pub fn list_since_block(&self, block_hash: &bitcoin::BlockHash) -> LSBlockRes { self.make_wallet_request( "listsinceblock", - ¶ms!(Json::String(block_hash.to_string()),), + ¶ms!( + Json::String(block_hash.to_string()), + Json::Number(1.into()), // Default for min_confirmations for the returned + Json::Bool(true), // Whether to include watchonly + Json::Bool(false), // Whether to include an array of txs that were removed in reorgs + Json::Bool(true) // Whether to include UTxOs treated as change. + ), ) .into() } From 7a18c583cbfcd958db9c79e8b13a0a68c1d20d41 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 21 Oct 2022 15:37:16 +0200 Subject: [PATCH 06/14] bitcoind: filter received coins based on parent descriptors Our bitcoind watchonly wallet could, maybe, have other descriptors that were imported. Sounds pretty unlikely since we use a dedicated wallet but hey. More importantly, we'll need to know the parent descriptor of the coin in order to recognize it as newly received or change. --- src/bitcoin/d/mod.rs | 21 +++++++++++++++--- src/bitcoin/mod.rs | 42 ++++++++++++++++++++++++++---------- src/bitcoin/poller/looper.rs | 19 ++++++++++------ src/bitcoin/poller/mod.rs | 4 +++- src/descriptors.rs | 6 ++++++ src/lib.rs | 1 + src/testutils.rs | 8 +++++-- 7 files changed, 78 insertions(+), 23 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 4de7a4bb..4c010c1d 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -10,7 +10,7 @@ use jsonrpc::{ client::Client, simple_http::{self, SimpleHttpTransport}, }; -use miniscript::bitcoin; +use miniscript::{bitcoin, descriptor}; use serde_json::Value as Json; @@ -532,8 +532,8 @@ impl BitcoinD { Json::String(block_hash.to_string()), Json::Number(1.into()), // Default for min_confirmations for the returned Json::Bool(true), // Whether to include watchonly - Json::Bool(false), // Whether to include an array of txs that were removed in reorgs - Json::Bool(true) // Whether to include UTxOs treated as change. + Json::Bool(false), // Whether to include an array of txs that were removed in reorgs + Json::Bool(true) // Whether to include UTxOs treated as change. ), ) .into() @@ -714,6 +714,7 @@ pub struct LSBlockEntry { pub amount: bitcoin::Amount, pub block_height: Option, pub address: bitcoin::Address, + pub parent_descs: Vec>, } impl From<&Json> for LSBlockEntry { @@ -745,12 +746,26 @@ impl From<&Json> for LSBlockEntry { .and_then(Json::as_str) .and_then(|s| bitcoin::Address::from_str(s).ok()) .expect("bitcoind can't give a bad address"); + let parent_descs = json + .get("parent_descs") + .and_then(Json::as_array) + .and_then(|descs| { + descs + .iter() + .map(|desc| { + desc.as_str() + .and_then(|s| descriptor::Descriptor::<_>::from_str(s).ok()) + }) + .collect::>>() + }) + .expect("bitcoind can't give invalid descriptors"); LSBlockEntry { outpoint, amount, block_height, address, + parent_descs, } } } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index ec7cf141..a95de2d8 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -4,7 +4,10 @@ pub mod d; pub mod poller; -use d::{BitcoindError, LSBlockEntry}; +use crate::{ + bitcoin::d::{BitcoindError, LSBlockEntry}, + descriptors, +}; use std::{collections::HashMap, error, fmt, sync}; @@ -56,7 +59,11 @@ pub trait BitcoinInterface: Send { fn is_in_chain(&self, tip: &BlockChainTip) -> bool; /// Get coins received since the specified tip. - fn received_coins(&self, tip: &BlockChainTip) -> Vec; + fn received_coins( + &self, + tip: &BlockChainTip, + desc: &descriptors::InheritanceDescriptor, + ) -> Vec; /// Get all coins that were confirmed, and at what height and time. fn confirmed_coins( @@ -106,25 +113,34 @@ impl BitcoinInterface for d::BitcoinD { .unwrap_or(false) } - fn received_coins(&self, tip: &BlockChainTip) -> Vec { + fn received_coins( + &self, + tip: &BlockChainTip, + desc: &descriptors::InheritanceDescriptor, + ) -> Vec { // TODO: don't assume only a single descriptor is loaded on the wo wallet let lsb_res = self.list_since_block(&tip.hash); lsb_res .received_coins .into_iter() - .map(|entry| { + .filter_map(|entry| { let LSBlockEntry { outpoint, amount, block_height, address, + parent_descs, } = entry; - UTxO { - outpoint, - amount, - block_height, - address, + if parent_descs.iter().any(|parent_desc| desc == parent_desc) { + Some(UTxO { + outpoint, + amount, + block_height, + address, + }) + } else { + None } }) .collect() @@ -287,8 +303,12 @@ impl BitcoinInterface for sync::Arc> self.lock().unwrap().is_in_chain(tip) } - fn received_coins(&self, tip: &BlockChainTip) -> Vec { - self.lock().unwrap().received_coins(tip) + fn received_coins( + &self, + tip: &BlockChainTip, + desc: &descriptors::InheritanceDescriptor, + ) -> Vec { + self.lock().unwrap().received_coins(tip, desc) } fn confirmed_coins( diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 25aa39ae..77751092 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -1,6 +1,7 @@ use crate::{ bitcoin::{BitcoinInterface, BlockChainTip, UTxO}, database::{Coin, DatabaseConnection, DatabaseInterface}, + descriptors, }; use std::{ @@ -26,13 +27,14 @@ fn update_coins( bit: &impl BitcoinInterface, db_conn: &mut Box, previous_tip: &BlockChainTip, + desc: &descriptors::InheritanceDescriptor, ) -> UpdatedCoins { let curr_coins = db_conn.coins(); log::debug!("Current coins: {:?}", curr_coins); // Start by fetching newly received coins. let mut received = Vec::new(); - for utxo in bit.received_coins(previous_tip) { + for utxo in bit.received_coins(previous_tip, desc) { if let Some(derivation_index) = db_conn.derivation_index_by_address(&utxo.address) { if !curr_coins.contains_key(&utxo.outpoint) { let UTxO { @@ -154,7 +156,11 @@ fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdat TipUpdate::Reorged(common_ancestor) } -fn updates(bit: &impl BitcoinInterface, db: &impl DatabaseInterface) { +fn updates( + bit: &impl BitcoinInterface, + db: &impl DatabaseInterface, + desc: &descriptors::InheritanceDescriptor, +) { let mut db_conn = db.connection(); // Check if there was a new block before updating ourselves. @@ -167,18 +173,18 @@ fn updates(bit: &impl BitcoinInterface, db: &impl DatabaseInterface) { // between our former chain and the new one, then restart fresh. db_conn.rollback_tip(&new_tip); log::info!("Tip was rolled back to '{}'.", new_tip); - return updates(bit, db); + return updates(bit, db, desc); } }; // Then check the state of our coins. Do it even if the tip did not change since last poll, as // we may have unconfirmed transactions. - let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip); + let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, desc); // If the tip changed while we were polling our Bitcoin interface, start over. if bit.chain_tip() != latest_tip { log::info!("Chain tip changed while we were updating our state. Starting over."); - return updates(bit, db); + return updates(bit, db, desc); } // The chain tip did not change since we started our updates. Record them and the latest tip. @@ -213,6 +219,7 @@ pub fn looper( db: sync::Arc>, shutdown: sync::Arc, poll_interval: time::Duration, + desc: descriptors::InheritanceDescriptor, ) { let mut last_poll = None; let mut synced = false; @@ -247,6 +254,6 @@ pub fn looper( } } - updates(&bit, &db); + updates(&bit, &db, &desc); } } diff --git a/src/bitcoin/poller/mod.rs b/src/bitcoin/poller/mod.rs index 97a1c22d..7fe3e860 100644 --- a/src/bitcoin/poller/mod.rs +++ b/src/bitcoin/poller/mod.rs @@ -3,6 +3,7 @@ mod looper; use crate::{ bitcoin::{poller::looper::looper, BitcoinInterface}, database::DatabaseInterface, + descriptors, }; use std::{ @@ -21,13 +22,14 @@ impl Poller { bit: sync::Arc>, db: sync::Arc>, poll_interval: time::Duration, + desc: descriptors::InheritanceDescriptor, ) -> Poller { let shutdown = sync::Arc::from(atomic::AtomicBool::from(false)); let handle = thread::Builder::new() .name("Bitcoin poller".to_string()) .spawn({ let shutdown = shutdown.clone(); - move || looper(bit, db, shutdown, poll_interval) + move || looper(bit, db, shutdown, poll_interval, desc) }) .expect("Must not fail"); diff --git a/src/descriptors.rs b/src/descriptors.rs index 05974816..46b44009 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -263,6 +263,12 @@ impl str::FromStr for InheritanceDescriptor { } } +impl PartialEq> for InheritanceDescriptor { + fn eq(&self, other: &descriptor::Descriptor) -> bool { + self.0.eq(other) + } +} + impl InheritanceDescriptor { pub fn new( owner_key: descriptor::DescriptorPublicKey, diff --git a/src/lib.rs b/src/lib.rs index 702a9c87..a8a6cfe2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -317,6 +317,7 @@ impl DaemonHandle { bit.clone(), db.clone(), config.bitcoin_config.poll_interval_secs, + config.main_descriptor.clone(), ); // Finally, set up the API. diff --git a/src/testutils.rs b/src/testutils.rs index f738d503..7b11ab9a 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -2,7 +2,7 @@ use crate::{ bitcoin::{BitcoinError, BitcoinInterface, BlockChainTip, UTxO}, config::{BitcoinConfig, Config}, database::{Coin, DatabaseConnection, DatabaseInterface, SpendBlock}, - DaemonHandle, + descriptors, DaemonHandle, }; use std::{collections::HashMap, env, fs, io, path, process, str::FromStr, sync, thread, time}; @@ -44,7 +44,11 @@ impl BitcoinInterface for DummyBitcoind { true } - fn received_coins(&self, _: &BlockChainTip) -> Vec { + fn received_coins( + &self, + _: &BlockChainTip, + _: &descriptors::InheritanceDescriptor, + ) -> Vec { Vec::new() } From d4db804e4bad928466fa1db4a84a474ff4c33d7b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 08:55:39 +0200 Subject: [PATCH 07/14] qa: add a missing 'wait_for' in spend creation test --- tests/test_rpc.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 530ef603..b8044aeb 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -88,6 +88,7 @@ def test_create_spend(minisafed, bitcoind): bitcoind.generate_block(1, wait_for_mempool=txid) txid = bitcoind.rpc.sendtoaddress(addr, 0.3556) bitcoind.generate_block(1, wait_for_mempool=txid) + wait_for(lambda: len(minisafed.rpc.listcoins()["coins"]) == 16) # Stop the daemon, should be a no-op minisafed.stop() From 1320ee30bacdedd449dfa89a3008e01995917c9f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 08:55:58 +0200 Subject: [PATCH 08/14] daemon: use multipath descriptors In config, expect to be given a multipath descriptor that contains a derivation path for both receive and change addresses, but only for those. Instead of 'xpub/*', start using 'xpub/0/*' and 'xpub/1/*'. When creating the watchonly wallet on bitcoind import both the receive and change descriptors. When polling, check for coins on both descriptors. --- src/bitcoin/d/mod.rs | 28 +++-- src/bitcoin/mod.rs | 13 +- src/bitcoin/poller/looper.rs | 15 +-- src/commands/mod.rs | 2 +- src/config.rs | 6 +- src/database/sqlite/mod.rs | 4 +- src/descriptors.rs | 194 ++++++++++++++++++++---------- src/lib.rs | 17 ++- src/testutils.rs | 6 +- tests/fixtures.py | 2 +- tests/test_framework/minisafed.py | 36 ++++-- 11 files changed, 211 insertions(+), 112 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 4c010c1d..5e21472e 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -351,13 +351,19 @@ impl BitcoinD { None } - // TODO: rescan feature will probably need another timestamp than 'now' - fn import_descriptor(&self, descriptor: &InheritanceDescriptor) -> Option { - let descriptors = vec![serde_json::json!({ - "desc": descriptor.to_string(), - "timestamp": "now", - "active": false, - })]; + // Import the receive and change descriptors from the multipath descriptor to bitcoind. + fn import_descriptor(&self, desc: &InheritanceDescriptor) -> Option { + let descriptors = [desc.receive_descriptor(), desc.change_descriptor()] + .iter() + .map(|desc| { + // TODO: rescan feature will probably need another timestamp than 'now' + serde_json::json!({ + "desc": desc.to_string(), + "timestamp": "now", + "active": false, + }) + }) + .collect(); let res = self.make_wallet_request("importdescriptors", ¶ms!(Json::Array(descriptors))); let all_succeeded = res @@ -471,9 +477,11 @@ impl BitcoinD { } // Check our main descriptor is imported in this wallet. - if !self - .list_descriptors() - .contains(&main_descriptor.to_string()) + let receive_desc = main_descriptor.receive_descriptor(); + let change_desc = main_descriptor.change_descriptor(); + let desc_list = self.list_descriptors(); + if !desc_list.contains(&receive_desc.to_string()) + || !desc_list.contains(&change_desc.to_string()) { return Err(BitcoindError::MissingDescriptor); } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index a95de2d8..ae7b2257 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -62,7 +62,7 @@ pub trait BitcoinInterface: Send { fn received_coins( &self, tip: &BlockChainTip, - desc: &descriptors::InheritanceDescriptor, + descs: &[descriptors::InheritanceDescriptor], ) -> Vec; /// Get all coins that were confirmed, and at what height and time. @@ -116,7 +116,7 @@ impl BitcoinInterface for d::BitcoinD { fn received_coins( &self, tip: &BlockChainTip, - desc: &descriptors::InheritanceDescriptor, + descs: &[descriptors::InheritanceDescriptor], ) -> Vec { // TODO: don't assume only a single descriptor is loaded on the wo wallet let lsb_res = self.list_since_block(&tip.hash); @@ -132,7 +132,10 @@ impl BitcoinInterface for d::BitcoinD { address, parent_descs, } = entry; - if parent_descs.iter().any(|parent_desc| desc == parent_desc) { + if parent_descs + .iter() + .any(|parent_desc| descs.iter().any(|desc| desc == parent_desc)) + { Some(UTxO { outpoint, amount, @@ -306,9 +309,9 @@ impl BitcoinInterface for sync::Arc> fn received_coins( &self, tip: &BlockChainTip, - desc: &descriptors::InheritanceDescriptor, + descs: &[descriptors::InheritanceDescriptor], ) -> Vec { - self.lock().unwrap().received_coins(tip, desc) + self.lock().unwrap().received_coins(tip, descs) } fn confirmed_coins( diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 77751092..64904d5f 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -27,14 +27,14 @@ fn update_coins( bit: &impl BitcoinInterface, db_conn: &mut Box, previous_tip: &BlockChainTip, - desc: &descriptors::InheritanceDescriptor, + descs: &[descriptors::InheritanceDescriptor], ) -> UpdatedCoins { let curr_coins = db_conn.coins(); log::debug!("Current coins: {:?}", curr_coins); // Start by fetching newly received coins. let mut received = Vec::new(); - for utxo in bit.received_coins(previous_tip, desc) { + for utxo in bit.received_coins(previous_tip, descs) { if let Some(derivation_index) = db_conn.derivation_index_by_address(&utxo.address) { if !curr_coins.contains_key(&utxo.outpoint) { let UTxO { @@ -159,7 +159,7 @@ fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdat fn updates( bit: &impl BitcoinInterface, db: &impl DatabaseInterface, - desc: &descriptors::InheritanceDescriptor, + descs: &[descriptors::InheritanceDescriptor], ) { let mut db_conn = db.connection(); @@ -173,18 +173,18 @@ fn updates( // between our former chain and the new one, then restart fresh. db_conn.rollback_tip(&new_tip); log::info!("Tip was rolled back to '{}'.", new_tip); - return updates(bit, db, desc); + return updates(bit, db, descs); } }; // Then check the state of our coins. Do it even if the tip did not change since last poll, as // we may have unconfirmed transactions. - let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, desc); + let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, descs); // If the tip changed while we were polling our Bitcoin interface, start over. if bit.chain_tip() != latest_tip { log::info!("Chain tip changed while we were updating our state. Starting over."); - return updates(bit, db, desc); + return updates(bit, db, descs); } // The chain tip did not change since we started our updates. Record them and the latest tip. @@ -223,6 +223,7 @@ pub fn looper( ) { let mut last_poll = None; let mut synced = false; + let descs = [desc.receive_descriptor(), desc.change_descriptor()]; maybe_initialize_tip(&bit, &db); @@ -254,6 +255,6 @@ pub fn looper( } } - updates(&bit, &db, &desc); + updates(&bit, &db, &descs); } } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 9805331a..e5704daa 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -572,7 +572,7 @@ mod tests { assert_eq!( addr, bitcoin::Address::from_str( - "bc1qgudekhcrejgtlx3yhlvdul7t4q76e5lhm0vtcsndxs6aslh4r9jsqkqhwu" + "bc1q9ksrc647hx8zp2cewl8p5f487dgux3777yees8rjcx46t4daqzzqt7yga8" ) .unwrap() ); diff --git a/src/config.rs b/src/config.rs index a873abf4..ac532b55 100644 --- a/src/config.rs +++ b/src/config.rs @@ -228,7 +228,7 @@ mod tests { data_dir = "/home/wizardsardine/custom/folder/" daemon = false log_level = "debug" - main_descriptor = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2d" + main_descriptor = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9" [bitcoin_config] network = "bitcoin" @@ -245,7 +245,7 @@ mod tests { data_dir = '/home/wizardsardine/custom/folder/' daemon = false log_level = 'TRACE' - main_descriptor = 'wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2d' + main_descriptor = 'wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9' [bitcoin_config] network = 'bitcoin' @@ -266,7 +266,7 @@ mod tests { log_level = "trace" data_dir = "/home/wizardsardine/custom/folder/" - main_descriptor = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2e" + main_descriptor = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#y5wcna2e" [bitcoin_config] network = "bitcoin" diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index c0178fd2..cd0ec795 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -484,7 +484,7 @@ mod tests { use bitcoin::{hashes::Hash, util::bip32}; fn dummy_options() -> FreshDbOptions { - let desc_str = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2d"; + let desc_str = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9"; let main_descriptor = InheritanceDescriptor::from_str(desc_str).unwrap(); FreshDbOptions { bitcoind_network: bitcoin::Network::Bitcoin, @@ -533,7 +533,7 @@ mod tests { .to_string() .contains("Database was created for network"); fs::remove_file(&db_path).unwrap(); - let other_desc_str = "wsh(andor(pk(tpubDExU4YLJkyQ9RRbVScQq2brFxWWha7WmAUByPWyaWYwmcTv3Shx8aHp6mVwuE5n4TeM4z5DTWGf2YhNPmXtfvyr8cUDVvA3txdrFnFgNdF7/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))"; + let other_desc_str = "wsh(andor(pk(tpubDExU4YLJkyQ9RRbVScQq2brFxWWha7WmAUByPWyaWYwmcTv3Shx8aHp6mVwuE5n4TeM4z5DTWGf2YhNPmXtfvyr8cUDVvA3txdrFnFgNdF7/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))"; let other_desc = InheritanceDescriptor::from_str(other_desc_str).unwrap(); let db = SqliteDb::new(db_path.clone(), Some(options.clone()), &secp).unwrap(); db.sanity_check(bitcoin::Network::Bitcoin, &other_desc) diff --git a/src/descriptors.rs b/src/descriptors.rs index 46b44009..87a5d6ab 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -31,7 +31,11 @@ impl std::fmt::Display for DescCreationError { match self { Self::InsaneTimelock(tl) => write!(f, "Timelock value '{}' isn't safe to use", tl), Self::InvalidKey(key) => { - write!(f, "Invalid key '{}'. Need a wildcard ('ranged') xpub", key) + write!( + f, + "Invalid key '{}'. Need a wildcard ('ranged') xpub with a multipath for (and only for) deriving change addresses. That is, an xpub of the form 'xpub.../<0;1>/*'.", + key + ) } Self::Miniscript(e) => write!(f, "Miniscript error: '{}'.", e), Self::IncompatibleDesc => write!(f, "Descriptor is not compatible."), @@ -167,12 +171,22 @@ fn csv_check(csv_value: u32) -> Result<(), DescCreationError> { .map_err(|_| DescCreationError::InsaneTimelock(csv_value)) } -fn is_unhardened_deriv(key: &descriptor::DescriptorPublicKey) -> bool { +// We require the descriptor key to: +// - Be deriveable (to contain a wildcard) +// - Be multipath (to contain a step in the derivation path with multiple indexes) +// - The multipath step to only contain two indexes, 0 and 1. +fn is_valid_desc_key(key: &descriptor::DescriptorPublicKey) -> bool { match *key { - descriptor::DescriptorPublicKey::Single(..) - | descriptor::DescriptorPublicKey::MultiXPub(..) => false, - descriptor::DescriptorPublicKey::XPub(ref xpub) => { + descriptor::DescriptorPublicKey::Single(..) | descriptor::DescriptorPublicKey::XPub(..) => { + false + } + descriptor::DescriptorPublicKey::MultiXPub(ref xpub) => { + // Rust-miniscript enforces BIP389 which states that all paths must have the same len. + let len = xpub.derivation_paths.get(0).expect("Cannot be empty").len(); xpub.wildcard == descriptor::Wildcard::Unhardened + && xpub.derivation_paths.len() == 2 + && xpub.derivation_paths[0][len - 1] == 0.into() + && xpub.derivation_paths[1][len - 1] == 1.into() } } } @@ -203,7 +217,7 @@ impl str::FromStr for InheritanceDescriptor { _ => return Err(DescCreationError::IncompatibleDesc), }; let invalid_key = ms.iter_pk().find_map(|pk| { - if is_unhardened_deriv(&pk) { + if is_valid_desc_key(&pk) { None } else { Some(pk) @@ -269,6 +283,53 @@ impl PartialEq> for Inhe } } +// Derive a **single-path** descriptor at a **unhardened** derivation index. Will panic if either +// of these isn't true. +fn derive_desc( + desc: &descriptor::Descriptor, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, +) -> descriptor::Descriptor { + assert!(index.is_normal()); + + // Unfortunately we can't just use `self.0.at_derivation_index().derived_descriptor()` + // since it would return a raw public key, but we need the origin too. + // TODO: upstream our DerivedPublicKey stuff to rust-miniscript. + // + // So we roll our own translation. + struct Derivator<'a, C: secp256k1::Verification>(u32, &'a secp256k1::Secp256k1); + impl<'a, C: secp256k1::Verification> + Translator + for Derivator<'a, C> + { + fn pk( + &mut self, + pk: &descriptor::DescriptorPublicKey, + ) -> Result { + let definite_key = pk + .clone() + .at_derivation_index(self.0) + .expect("We disallow multipath keys."); + let origin = ( + definite_key.master_fingerprint(), + definite_key + .full_derivation_path() + .expect("We disallow multipath keys."), + ); + let key = definite_key.derive_public_key(self.1)?; + Ok(DerivedPublicKey { origin, key }) + } + translate_hash_clone!( + descriptor::DescriptorPublicKey, + DerivedPublicKey, + descriptor::ConversionError + ); + } + + desc.translate_pk(&mut Derivator(index.into(), secp)) + .expect("May only fail on hardened derivation indexes, but we ruled out this case.") +} + impl InheritanceDescriptor { pub fn new( owner_key: descriptor::DescriptorPublicKey, @@ -285,7 +346,7 @@ impl InheritanceDescriptor { if let Some(key) = vec![&owner_key, &heir_key] .iter() - .find(|k| !is_unhardened_deriv(k)) + .find(|k| !is_valid_desc_key(k)) { return Err(DescCreationError::InvalidKey((**key).clone())); } @@ -325,7 +386,7 @@ impl InheritanceDescriptor { /// Whether all xpubs contained in this descriptor are for the passed expected network. pub fn all_xpubs_net_is(&self, expected_net: bitcoin::Network) -> bool { self.0.for_each_key(|xpub| { - if let descriptor::DescriptorPublicKey::XPub(xpub) = xpub { + if let descriptor::DescriptorPublicKey::MultiXPub(xpub) = xpub { xpub.xkey.network == expected_net } else { false @@ -333,6 +394,46 @@ impl InheritanceDescriptor { }) } + // TODO: Use a newtype to differentiate single and multi path descriptors. + // TODO: Cache it inside the struct, it's very inefficient to use into_single_descriptors() for + // every single derivation. + pub fn receive_descriptor(&self) -> InheritanceDescriptor { + let singlepath_descs = self + .0 + .clone() + .into_single_descriptors() + .expect("Can't error, all paths have the same length"); + assert_eq!(singlepath_descs.len(), 2); + + // We use /0/* for receiving, so it's the first descriptor between <0;1>. + // FIXME: don't rely on ordering. + InheritanceDescriptor( + singlepath_descs + .into_iter() + .next() + .expect("Just checked the length"), + ) + } + + pub fn change_descriptor(&self) -> InheritanceDescriptor { + let singlepath_descs = self + .0 + .clone() + .into_single_descriptors() + .expect("Can't error, all paths have the same length"); + assert_eq!(singlepath_descs.len(), 2); + + // We use /1/* for change, so it's the second descriptor between <0;1>. + // FIXME: don't rely on ordering. + InheritanceDescriptor( + singlepath_descs + .into_iter() + .rev() + .next() + .expect("Just checked the length"), + ) + } + /// Derive this descriptor at a given index for a receiving address. /// /// # Panics @@ -344,48 +445,7 @@ impl InheritanceDescriptor { ) -> DerivedInheritanceDescriptor { assert!(index.is_normal()); - // Unfortunately we can't just use `self.0.at_derivation_index().derived_descriptor()` - // since it would return a raw public key, but we need the origin too. - // TODO: upstream our DerivedPublicKey stuff to rust-miniscript. - // - // So we roll our own translation. - struct Derivator<'a, C: secp256k1::Verification>(u32, &'a secp256k1::Secp256k1); - impl<'a, C: secp256k1::Verification> - Translator< - descriptor::DescriptorPublicKey, - DerivedPublicKey, - descriptor::ConversionError, - > for Derivator<'a, C> - { - fn pk( - &mut self, - pk: &descriptor::DescriptorPublicKey, - ) -> Result { - let definite_key = pk - .clone() - .at_derivation_index(self.0) - .expect("We disallow multipath keys."); - let origin = ( - definite_key.master_fingerprint(), - definite_key - .full_derivation_path() - .expect("We disallow multipath keys."), - ); - let key = definite_key.derive_public_key(self.1)?; - Ok(DerivedPublicKey { origin, key }) - } - translate_hash_clone!( - descriptor::DescriptorPublicKey, - DerivedPublicKey, - descriptor::ConversionError - ); - } - - let desc = self - .0 - .translate_pk(&mut Derivator(index.into(), secp)) - .expect("May only fail on hardened derivation indexes, but we ruled out this case."); - DerivedInheritanceDescriptor(desc) + DerivedInheritanceDescriptor(derive_desc(&self.receive_descriptor().0, index, secp)) } /// Get the value (in blocks) of the relative timelock for the heir's spending path. @@ -478,10 +538,10 @@ mod tests { #[test] fn inheritance_descriptor_creation() { - let owner_key = descriptor::DescriptorPublicKey::from_str("xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/*").unwrap(); - let heir_key = descriptor::DescriptorPublicKey::from_str("xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/*").unwrap(); + let owner_key = descriptor::DescriptorPublicKey::from_str("xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*").unwrap(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*").unwrap(); let timelock = 52560; - assert_eq!(InheritanceDescriptor::new(owner_key, heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk(xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/*),and_v(v:pkh(xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/*),older(52560))))#eeyujkt7"); + assert_eq!(InheritanceDescriptor::new(owner_key, heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk(xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*),and_v(v:pkh(xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*),older(52560))))#8n2ydpkt"); // We prevent footguns with timelocks by requiring a u16. Note how the following wouldn't // compile: @@ -489,30 +549,36 @@ mod tests { //InheritanceDescriptor::new(owner_key.clone(), heir_key.clone(), (1 << 31) + 1).unwrap_err(); //InheritanceDescriptor::new(owner_key, heir_key, (1 << 22) + 1).unwrap_err(); - let owner_key = descriptor::DescriptorPublicKey::from_str("[aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/*").unwrap(); - let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/*").unwrap(); + let owner_key = descriptor::DescriptorPublicKey::from_str("[aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/<0;1>/*").unwrap(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/<0;1>/*").unwrap(); let timelock = 57600; - assert_eq!(InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk([aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/*),and_v(v:pkh(xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/*),older(57600))))#8kamh6y8"); + assert_eq!(InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk([aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/<0;1>/*),and_v(v:pkh(xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/<0;1>/*),older(57600))))#l6dlpc2l"); - // We can't pass a raw key, an xpub that is not deriveable, or only hardened derivable - let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/354").unwrap(); + // We can't pass a raw key, an xpub that is not deriveable, only hardened derivable, + // without both the change and receive derivation paths, or with more than 2 different + // derivation paths. + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/<0;1>/354").unwrap(); InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); - let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/*'").unwrap(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/<0;1>/*'").unwrap(); InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); let heir_key = descriptor::DescriptorPublicKey::from_str( "02e24913be26dbcfdf8e8e94870b28725cdae09b448b6c127767bf0154e3a3c8e5", ) .unwrap(); + InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/*'").unwrap(); + InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/<0;1;2>/*'").unwrap(); InheritanceDescriptor::new(owner_key, heir_key, timelock).unwrap_err(); } #[test] fn inheritance_descriptor_derivation() { let secp = secp256k1::Secp256k1::verification_only(); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))#y5wcna2d").unwrap(); + let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9").unwrap(); let der_desc = desc.derive_receive(11.into(), &secp); assert_eq!( - "bc1qvjzcg25nsxmfccct0txjvljxjwn68htkrw57jqmjhfzvhyd2z4msc74w65", + "bc1q26gtczlz03u6juf5cxppapk4sr4fyz53s3g4zs2cgactcahqv6yqc2t8e6", der_desc.address(bitcoin::Network::Bitcoin).to_string() ); @@ -525,13 +591,13 @@ mod tests { #[test] fn inheritance_descriptor_tl_value() { - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(1),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))").unwrap(); + let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(1),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 1); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(42000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))").unwrap(); + let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(42000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 42000); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/*),older(65535),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/*)))").unwrap(); + let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(65535),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 0xffff); } diff --git a/src/lib.rs b/src/lib.rs index a8a6cfe2..168fc199 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -511,11 +511,14 @@ mod tests { stream.flush().unwrap(); } - // Send them a response to 'listdescriptors' with the main descriptor - fn complete_desc_check(server: &net::TcpListener, desc: &str) { + // Send them a response to 'listdescriptors' with the receive and change descriptors + fn complete_desc_check(server: &net::TcpListener, receive_desc: &str, change_desc: &str) { let net_resp = [ "HTTP/1.1 200\n\r\n{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{\"descriptors\":[{\"desc\":\"".as_bytes(), - desc.as_bytes(), + receive_desc.as_bytes(), + "\"},".as_bytes(), + "{\"desc\":\"".as_bytes(), + change_desc.as_bytes(), "\"}]}}\n".as_bytes(), ] .concat(); @@ -595,8 +598,10 @@ mod tests { }; // Create a dummy config with this bitcoind - let desc_str = "wsh(andor(pk(xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/*),older(10000),pk(xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/*)))#tk6wzexy"; + let desc_str = "wsh(andor(pk(xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/<0;1>/*),older(10000),pk(xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*)))#yudtr0k5"; let desc = InheritanceDescriptor::from_str(desc_str).unwrap(); + let receive_desc = desc.receive_descriptor(); + let change_desc = desc.change_descriptor(); let config = Config { bitcoin_config, bitcoind_config: Some(bitcoind_config), @@ -621,7 +626,7 @@ mod tests { complete_version_check(&server); complete_network_check(&server); complete_wallet_check(&server, &wo_path); - complete_desc_check(&server, desc_str); + complete_desc_check(&server, &receive_desc.to_string(), &change_desc.to_string()); complete_tip_init(&server); complete_sync_check(&server); daemon_thread.join().unwrap(); @@ -636,7 +641,7 @@ mod tests { complete_version_check(&server); complete_network_check(&server); complete_wallet_check(&server, &wo_path); - complete_desc_check(&server, desc_str); + complete_desc_check(&server, &receive_desc.to_string(), &change_desc.to_string()); complete_sync_check(&server); daemon_thread.join().unwrap(); diff --git a/src/testutils.rs b/src/testutils.rs index 7b11ab9a..bf3282f7 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -47,7 +47,7 @@ impl BitcoinInterface for DummyBitcoind { fn received_coins( &self, _: &BlockChainTip, - _: &descriptors::InheritanceDescriptor, + _: &[descriptors::InheritanceDescriptor], ) -> Vec { Vec::new() } @@ -274,8 +274,8 @@ impl DummyMinisafe { poll_interval_secs: time::Duration::from_secs(2), }; - let owner_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/*").unwrap(); - let heir_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/*").unwrap(); + let owner_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/<0;1>/*").unwrap(); + let heir_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*").unwrap(); let desc = crate::descriptors::InheritanceDescriptor::new(owner_key, heir_key, 10_000).unwrap(); let config = Config { diff --git a/tests/fixtures.py b/tests/fixtures.py index c94707ea..2a7bbe5b 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -121,7 +121,7 @@ def minisafed(bitcoind, directory): owner_hd = BIP32.from_seed(os.urandom(32), network="test") owner_xpub = owner_hd.get_xpub() - main_desc = Descriptor.from_str(f"wsh(or_d(pk({owner_xpub}/*),and_v(v:pkh(tpubD9vQiBdDxYzU4cVFtApWj4devZrvcfWaPXX1zHdDc7GPfUsDKqGnbhraccfm7BAXgRgUbVQUV2v2o4NitjGEk7hpbuP85kvBrD4ahFDtNBJ/*),older(65000))))") + main_desc = Descriptor.from_str(f"wsh(or_d(pk({owner_xpub}/<0;1>/*),and_v(v:pkh(tpubD9vQiBdDxYzU4cVFtApWj4devZrvcfWaPXX1zHdDc7GPfUsDKqGnbhraccfm7BAXgRgUbVQUV2v2o4NitjGEk7hpbuP85kvBrD4ahFDtNBJ/<0;1>/*),older(65000))))") minisafed = Minisafed( datadir, diff --git a/tests/test_framework/minisafed.py b/tests/test_framework/minisafed.py index 8600a7de..085e07e6 100644 --- a/tests/test_framework/minisafed.py +++ b/tests/test_framework/minisafed.py @@ -28,7 +28,7 @@ class Minisafed(TailableProc): self, datadir, owner_hd, - main_desc, + multi_desc, bitcoind_rpc_port, bitcoind_cookie_path, ): @@ -37,7 +37,8 @@ class Minisafed(TailableProc): self.prefix = os.path.split(datadir)[-1] self.owner_hd = owner_hd - self.main_desc = main_desc + self.multi_desc = multi_desc + self.receive_desc, self.change_desc = multi_desc.singlepath_descriptors() self.conf_file = os.path.join(datadir, "config.toml") self.cmd_line = [MINISAFED_PATH, "--conf", f"{self.conf_file}"] @@ -49,7 +50,7 @@ class Minisafed(TailableProc): f.write("daemon = false\n") f.write(f"log_level = '{LOG_LEVEL}'\n") - f.write(f'main_descriptor = "{main_desc}"\n') + f.write(f'main_descriptor = "{multi_desc}"\n') f.write("[bitcoin_config]\n") f.write('network = "regtest"\n') @@ -71,24 +72,32 @@ class Minisafed(TailableProc): # Sign each input. for i, psbt_in in enumerate(psbt.i): # First, gather the needed information from the PSBT input. - # 'hd_keypaths' is of the form {pubkey: (fingerprint (4 bytes), derivation index (4 bytes))} + # 'hd_keypaths' is of the form {pubkey: (fingerprint (4 bytes), derivation path (n * 4 bytes))} fing_der = next(iter(psbt_in.map[PSBT_IN_BIP32_DERIVATION].values())) - der_index = int.from_bytes(fing_der[4:], byteorder="little", signed=True) + raw_der_path = fing_der[4:] + der_path = [ + int.from_bytes(raw_der_path[i : i + 4], byteorder="little", signed=True) + for i in range(0, len(raw_der_path), 4) + ] script_code = psbt_in.map[PSBT_IN_WITNESS_SCRIPT] # Now sign the transaction with the key of the "owner" (the participant that # can sign immediately without a timelock) sighash = sighash_all_witness(script_code, psbt, i) privkey = coincurve.PrivateKey( - self.owner_hd.get_privkey_from_path([der_index]) + self.owner_hd.get_privkey_from_path(der_path) ) pubkey = privkey.public_key.format() assert pubkey in psbt_in.map[PSBT_IN_BIP32_DERIVATION].keys(), ( + der_path, + fing_der, pubkey, psbt_in.map[PSBT_IN_BIP32_DERIVATION].keys(), ) sig = privkey.sign(sighash, hasher=None) + b"\x01" - logging.debug(f"Adding signature {sig.hex()} for pubkey {pubkey.hex()}") + logging.debug( + f"Adding signature {sig.hex()} for pubkey {pubkey.hex()} (path {der_path})" + ) assert PSBT_IN_PARTIAL_SIG not in psbt_in.map psbt_in.map[PSBT_IN_PARTIAL_SIG] = {pubkey: sig} @@ -108,12 +117,19 @@ class Minisafed(TailableProc): # First, gather the needed information from the PSBT input. # 'hd_keypaths' is of the form {pubkey: (fingerprint, derivation index)} fing_der = next(iter(psbt_in.map[PSBT_IN_BIP32_DERIVATION].values())) - der_index = int.from_bytes(fing_der[4:], byteorder="little", signed=True) + raw_der_path = fing_der[4:] + der_path = [ + 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. - desc = Descriptor.from_str(str(self.main_desc)) - desc.derive(der_index) + desc = Descriptor.from_str( + str(self.receive_desc if der_path[0] == 0 else self.change_desc) + ) + desc.derive(der_path[1]) sat_material = SatisfactionMaterial( signatures=psbt_in.map[PSBT_IN_PARTIAL_SIG], ) From ca3d7c1f3360daa53fbec723fb960051a926d687 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 09:25:17 +0200 Subject: [PATCH 09/14] descriptors: introduce a newtype for the multipath descriptor The multipath descriptor has very different properties than the receive and change ones. Use a newtype to assist us in differentiating those. --- src/bitcoin/d/mod.rs | 8 +- src/bitcoin/poller/looper.rs | 2 +- src/bitcoin/poller/mod.rs | 2 +- src/commands/mod.rs | 5 +- src/config.rs | 6 +- src/database/sqlite/mod.rs | 24 +++-- src/database/sqlite/schema.rs | 6 +- src/database/sqlite/utils.rs | 3 +- src/descriptors.rs | 182 ++++++++++++++++++---------------- src/lib.rs | 4 +- src/testutils.rs | 2 +- 11 files changed, 131 insertions(+), 113 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 5e21472e..d1427049 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -1,7 +1,7 @@ ///! Implementation of the Bitcoin interface using bitcoind. ///! ///! We use the RPC interface and a watchonly descriptor wallet. -use crate::{bitcoin::BlockChainTip, config, descriptors::InheritanceDescriptor}; +use crate::{bitcoin::BlockChainTip, config, descriptors::MultipathDescriptor}; use std::{collections::HashSet, convert::TryInto, fs, io, str::FromStr, time::Duration}; @@ -352,7 +352,7 @@ impl BitcoinD { } // Import the receive and change descriptors from the multipath descriptor to bitcoind. - fn import_descriptor(&self, desc: &InheritanceDescriptor) -> Option { + fn import_descriptor(&self, desc: &MultipathDescriptor) -> Option { let descriptors = [desc.receive_descriptor(), desc.change_descriptor()] .iter() .map(|desc| { @@ -401,7 +401,7 @@ impl BitcoinD { /// Create the watchonly wallet on bitcoind, and import it the main descriptor. pub fn create_watchonly_wallet( &self, - main_descriptor: &InheritanceDescriptor, + main_descriptor: &MultipathDescriptor, ) -> Result<(), BitcoindError> { // Remove any leftover. This can happen if we delete the watchonly wallet but don't restart // bitcoind. @@ -441,7 +441,7 @@ impl BitcoinD { /// Perform various sanity checks on the bitcoind instance. pub fn sanity_check( &self, - main_descriptor: &InheritanceDescriptor, + main_descriptor: &MultipathDescriptor, config_network: bitcoin::Network, ) -> Result<(), BitcoindError> { // Check the minimum supported bitcoind version diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 64904d5f..25084e50 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -219,7 +219,7 @@ pub fn looper( db: sync::Arc>, shutdown: sync::Arc, poll_interval: time::Duration, - desc: descriptors::InheritanceDescriptor, + desc: descriptors::MultipathDescriptor, ) { let mut last_poll = None; let mut synced = false; diff --git a/src/bitcoin/poller/mod.rs b/src/bitcoin/poller/mod.rs index 7fe3e860..46cbb60d 100644 --- a/src/bitcoin/poller/mod.rs +++ b/src/bitcoin/poller/mod.rs @@ -22,7 +22,7 @@ impl Poller { bit: sync::Arc>, db: sync::Arc>, poll_interval: time::Duration, - desc: descriptors::InheritanceDescriptor, + desc: descriptors::MultipathDescriptor, ) -> Poller { let shutdown = sync::Arc::from(atomic::AtomicBool::from(false)); let handle = thread::Builder::new() diff --git a/src/commands/mod.rs b/src/commands/mod.rs index e5704daa..6c04752d 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -174,7 +174,8 @@ impl DaemonControl { fn derived_desc(&self, index: bip32::ChildNumber) -> descriptors::DerivedInheritanceDescriptor { self.config .main_descriptor - .derive_receive(index, &self.secp) + .receive_descriptor() + .derive(index, &self.secp) } } @@ -487,7 +488,7 @@ impl DaemonControl { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct GetInfoDescriptors { - pub main: descriptors::InheritanceDescriptor, + pub main: descriptors::MultipathDescriptor, } /// Information about the daemon diff --git a/src/config.rs b/src/config.rs index ac532b55..347ee76c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,4 +1,4 @@ -use crate::descriptors::InheritanceDescriptor; +use crate::descriptors::MultipathDescriptor; use std::{net::SocketAddr, path::PathBuf, str::FromStr, time::Duration}; @@ -92,7 +92,7 @@ pub struct Config { deserialize_with = "deserialize_fromstr", serialize_with = "serialize_to_string" )] - pub main_descriptor: InheritanceDescriptor, + pub main_descriptor: MultipathDescriptor, /// Settings for the Bitcoin interface pub bitcoin_config: BitcoinConfig, /// Settings specific to bitcoind as the Bitcoin interface @@ -113,7 +113,7 @@ pub enum ConfigError { DatadirNotFound, FileNotFound, ReadingFile(String), - UnexpectedDescriptor(Box), + UnexpectedDescriptor(Box), Unexpected(String), } diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index cd0ec795..3b672df4 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -18,7 +18,7 @@ use crate::{ }, Coin, }, - descriptors::InheritanceDescriptor, + descriptors::MultipathDescriptor, }; use std::{convert::TryInto, fmt, io, path}; @@ -36,7 +36,7 @@ pub enum SqliteDbError { FileNotFound(path::PathBuf), UnsupportedVersion(i64), InvalidNetwork(bitcoin::Network), - DescriptorMismatch(Box), + DescriptorMismatch(Box), Rusqlite(rusqlite::Error), } @@ -80,7 +80,7 @@ impl From for SqliteDbError { #[derive(Debug, Clone)] pub struct FreshDbOptions { pub bitcoind_network: bitcoin::Network, - pub main_descriptor: InheritanceDescriptor, + pub main_descriptor: MultipathDescriptor, } #[derive(Debug, Clone)] @@ -119,7 +119,7 @@ impl SqliteDb { pub fn sanity_check( &self, bitcoind_network: bitcoin::Network, - main_descriptor: &InheritanceDescriptor, + main_descriptor: &MultipathDescriptor, ) -> Result<(), SqliteDbError> { let mut conn = self.connection()?; @@ -240,7 +240,8 @@ impl SqliteConn { let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; let next_la_address = db_wallet .main_descriptor - .derive_receive(next_la_index.into(), secp) + .receive_descriptor() + .derive(next_la_index.into(), secp) .address(network); db_tx .execute( @@ -485,7 +486,7 @@ mod tests { fn dummy_options() -> FreshDbOptions { let desc_str = "wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9"; - let main_descriptor = InheritanceDescriptor::from_str(desc_str).unwrap(); + let main_descriptor = MultipathDescriptor::from_str(desc_str).unwrap(); FreshDbOptions { bitcoind_network: bitcoin::Network::Bitcoin, main_descriptor, @@ -534,7 +535,7 @@ mod tests { .contains("Database was created for network"); fs::remove_file(&db_path).unwrap(); let other_desc_str = "wsh(andor(pk(tpubDExU4YLJkyQ9RRbVScQq2brFxWWha7WmAUByPWyaWYwmcTv3Shx8aHp6mVwuE5n4TeM4z5DTWGf2YhNPmXtfvyr8cUDVvA3txdrFnFgNdF7/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))"; - let other_desc = InheritanceDescriptor::from_str(other_desc_str).unwrap(); + let other_desc = MultipathDescriptor::from_str(other_desc_str).unwrap(); let db = SqliteDb::new(db_path.clone(), Some(options.clone()), &secp).unwrap(); db.sanity_check(bitcoin::Network::Bitcoin, &other_desc) .unwrap_err() @@ -714,7 +715,8 @@ mod tests { // There is the index for the first index let addr = options .main_descriptor - .derive_receive(0.into(), &secp) + .receive_descriptor() + .derive(0.into(), &secp) .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 0.into()); @@ -722,7 +724,8 @@ mod tests { // There is the index for the 199th index (look-ahead limit) let addr = options .main_descriptor - .derive_receive(199.into(), &secp) + .receive_descriptor() + .derive(199.into(), &secp) .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 199.into()); @@ -730,7 +733,8 @@ mod tests { // And not for the 200th one. let addr = options .main_descriptor - .derive_receive(200.into(), &secp) + .receive_descriptor() + .derive(200.into(), &secp) .address(options.bitcoind_network); assert!(conn.db_address(&addr).is_none()); diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index b3b0f617..99975e48 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -1,4 +1,4 @@ -use crate::descriptors::InheritanceDescriptor; +use crate::descriptors::MultipathDescriptor; use std::{convert::TryFrom, str::FromStr}; @@ -103,7 +103,7 @@ impl TryFrom<&rusqlite::Row<'_>> for DbTip { pub struct DbWallet { pub id: i64, pub timestamp: u32, - pub main_descriptor: InheritanceDescriptor, + pub main_descriptor: MultipathDescriptor, pub deposit_derivation_index: bip32::ChildNumber, } @@ -115,7 +115,7 @@ impl TryFrom<&rusqlite::Row<'_>> for DbWallet { let timestamp = row.get(1)?; let desc_str: String = row.get(2)?; - let main_descriptor = InheritanceDescriptor::from_str(&desc_str) + let main_descriptor = MultipathDescriptor::from_str(&desc_str) .expect("Insane database: can't parse deposit descriptor"); let der_idx: u32 = row.get(3)?; diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index c9c0cd0a..43a3102c 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -98,7 +98,8 @@ pub fn create_fresh_db( // TODO: have this as a helper in descriptors.rs let address = options .main_descriptor - .derive_receive(index.into(), secp) + .receive_descriptor() + .derive(index.into(), secp) .address(options.bitcoind_network); query += &format!( "INSERT INTO addresses (address, derivation_index) VALUES (\"{}\", {});\n", diff --git a/src/descriptors.rs b/src/descriptors.rs index 87a5d6ab..f5fcb02c 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -191,8 +191,13 @@ fn is_valid_desc_key(key: &descriptor::DescriptorPublicKey) -> bool { } } +/// An [InheritanceDescriptor] that contains multipath keys for (and only for) the receive keychain +/// and the change keychain. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct MultipathDescriptor(descriptor::Descriptor); + /// A Miniscript descriptor with a main, unencombered, branch (the main owner of the coins) -/// and a timelocked branch (the heir). +/// and a timelocked branch (the heir). All keys in this descriptor are singlepath. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct InheritanceDescriptor(descriptor::Descriptor); @@ -200,16 +205,16 @@ pub struct InheritanceDescriptor(descriptor::Descriptor); -impl fmt::Display for InheritanceDescriptor { +impl fmt::Display for MultipathDescriptor { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}", self.0) } } -impl str::FromStr for InheritanceDescriptor { +impl str::FromStr for MultipathDescriptor { type Err = DescCreationError; - fn from_str(s: &str) -> Result { + fn from_str(s: &str) -> Result { let wsh_desc = descriptor::Wsh::::from_str(s) .map_err(DescCreationError::Miniscript)?; let ms = match wsh_desc.as_inner() { @@ -273,7 +278,13 @@ impl str::FromStr for InheritanceDescriptor { .find(|s| matches!(s, SemanticPolicy::Key(_))) .ok_or(DescCreationError::IncompatibleDesc)?; - Ok(InheritanceDescriptor(descriptor::Descriptor::Wsh(wsh_desc))) + Ok(MultipathDescriptor(descriptor::Descriptor::Wsh(wsh_desc))) + } +} + +impl fmt::Display for InheritanceDescriptor { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.0) } } @@ -283,59 +294,12 @@ impl PartialEq> for Inhe } } -// Derive a **single-path** descriptor at a **unhardened** derivation index. Will panic if either -// of these isn't true. -fn derive_desc( - desc: &descriptor::Descriptor, - index: bip32::ChildNumber, - secp: &secp256k1::Secp256k1, -) -> descriptor::Descriptor { - assert!(index.is_normal()); - - // Unfortunately we can't just use `self.0.at_derivation_index().derived_descriptor()` - // since it would return a raw public key, but we need the origin too. - // TODO: upstream our DerivedPublicKey stuff to rust-miniscript. - // - // So we roll our own translation. - struct Derivator<'a, C: secp256k1::Verification>(u32, &'a secp256k1::Secp256k1); - impl<'a, C: secp256k1::Verification> - Translator - for Derivator<'a, C> - { - fn pk( - &mut self, - pk: &descriptor::DescriptorPublicKey, - ) -> Result { - let definite_key = pk - .clone() - .at_derivation_index(self.0) - .expect("We disallow multipath keys."); - let origin = ( - definite_key.master_fingerprint(), - definite_key - .full_derivation_path() - .expect("We disallow multipath keys."), - ); - let key = definite_key.derive_public_key(self.1)?; - Ok(DerivedPublicKey { origin, key }) - } - translate_hash_clone!( - descriptor::DescriptorPublicKey, - DerivedPublicKey, - descriptor::ConversionError - ); - } - - desc.translate_pk(&mut Derivator(index.into(), secp)) - .expect("May only fail on hardened derivation indexes, but we ruled out this case.") -} - -impl InheritanceDescriptor { +impl MultipathDescriptor { pub fn new( owner_key: descriptor::DescriptorPublicKey, heir_key: descriptor::DescriptorPublicKey, timelock: u16, - ) -> Result { + ) -> Result { // We require the locktime to: // - not be disabled // - be in number of blocks @@ -378,7 +342,7 @@ impl InheritanceDescriptor { miniscript::Segwitv0::check_local_validity(&tl_miniscript) .expect("Miniscript must be sane"); - Ok(InheritanceDescriptor(descriptor::Descriptor::Wsh( + Ok(MultipathDescriptor(descriptor::Descriptor::Wsh( descriptor::Wsh::new(tl_miniscript).expect("Must pass sanity checks"), ))) } @@ -394,9 +358,9 @@ impl InheritanceDescriptor { }) } - // TODO: Use a newtype to differentiate single and multi path descriptors. // TODO: Cache it inside the struct, it's very inefficient to use into_single_descriptors() for // every single derivation. + /// Get the descriptor for receiving addresses. pub fn receive_descriptor(&self) -> InheritanceDescriptor { let singlepath_descs = self .0 @@ -415,6 +379,9 @@ impl InheritanceDescriptor { ) } + // TODO: Cache it inside the struct, it's very inefficient to use into_single_descriptors() for + // every single derivation. + /// Get the descriptor for change addresses. pub fn change_descriptor(&self) -> InheritanceDescriptor { let singlepath_descs = self .0 @@ -434,20 +401,6 @@ impl InheritanceDescriptor { ) } - /// Derive this descriptor at a given index for a receiving address. - /// - /// # Panics - /// - If the given index is hardened. - pub fn derive_receive( - &self, - index: bip32::ChildNumber, - secp: &secp256k1::Secp256k1, - ) -> DerivedInheritanceDescriptor { - assert!(index.is_normal()); - - DerivedInheritanceDescriptor(derive_desc(&self.receive_descriptor().0, index, secp)) - } - /// Get the value (in blocks) of the relative timelock for the heir's spending path. pub fn timelock_value(&self) -> u32 { let wsh_desc = match &self.0 { @@ -487,6 +440,65 @@ impl InheritanceDescriptor { } } +impl InheritanceDescriptor { + /// Derive this descriptor at a given index for a receiving address. + /// + /// # Panics + /// - If the given index is hardened. + pub fn derive( + &self, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, + ) -> DerivedInheritanceDescriptor { + assert!(index.is_normal()); + + // Unfortunately we can't just use `self.0.at_derivation_index().derived_descriptor()` + // since it would return a raw public key, but we need the origin too. + // TODO: upstream our DerivedPublicKey stuff to rust-miniscript. + // + // So we roll our own translation. + struct Derivator<'a, C: secp256k1::Verification>(u32, &'a secp256k1::Secp256k1); + impl<'a, C: secp256k1::Verification> + Translator< + descriptor::DescriptorPublicKey, + DerivedPublicKey, + descriptor::ConversionError, + > for Derivator<'a, C> + { + fn pk( + &mut self, + pk: &descriptor::DescriptorPublicKey, + ) -> Result { + let definite_key = pk + .clone() + .at_derivation_index(self.0) + .expect("We disallow multipath keys."); + let origin = ( + definite_key.master_fingerprint(), + definite_key + .full_derivation_path() + .expect("We disallow multipath keys."), + ); + let key = definite_key.derive_public_key(self.1)?; + Ok(DerivedPublicKey { origin, key }) + } + translate_hash_clone!( + descriptor::DescriptorPublicKey, + DerivedPublicKey, + descriptor::ConversionError + ); + } + + DerivedInheritanceDescriptor( + self.0 + .translate_pk(&mut Derivator(index.into(), secp)) + .expect( + "May only fail on hardened derivation indexes, but we ruled out this case.", + ), + ) + } +} + /// Map of a raw public key to the xpub used to derive it and its derivation path pub type Bip32Deriv = BTreeMap; @@ -541,42 +553,42 @@ mod tests { let owner_key = descriptor::DescriptorPublicKey::from_str("xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*").unwrap(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*").unwrap(); let timelock = 52560; - assert_eq!(InheritanceDescriptor::new(owner_key, heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk(xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*),and_v(v:pkh(xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*),older(52560))))#8n2ydpkt"); + assert_eq!(MultipathDescriptor::new(owner_key, heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk(xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*),and_v(v:pkh(xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*),older(52560))))#8n2ydpkt"); // We prevent footguns with timelocks by requiring a u16. Note how the following wouldn't // compile: - //InheritanceDescriptor::new(owner_key.clone(), heir_key.clone(), 0x00_01_0f_00).unwrap_err(); - //InheritanceDescriptor::new(owner_key.clone(), heir_key.clone(), (1 << 31) + 1).unwrap_err(); - //InheritanceDescriptor::new(owner_key, heir_key, (1 << 22) + 1).unwrap_err(); + //MultipathDescriptor::new(owner_key.clone(), heir_key.clone(), 0x00_01_0f_00).unwrap_err(); + //MultipathDescriptor::new(owner_key.clone(), heir_key.clone(), (1 << 31) + 1).unwrap_err(); + //MultipathDescriptor::new(owner_key, heir_key, (1 << 22) + 1).unwrap_err(); let owner_key = descriptor::DescriptorPublicKey::from_str("[aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/<0;1>/*").unwrap(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/<0;1>/*").unwrap(); let timelock = 57600; - assert_eq!(InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk([aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/<0;1>/*),and_v(v:pkh(xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/<0;1>/*),older(57600))))#l6dlpc2l"); + assert_eq!(MultipathDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap().to_string(), "wsh(or_d(pk([aabb0011/10/4893]xpub661MyMwAqRbcFG59fiikD8UV762quhruT8K8bdjqy6N2o3LG7yohoCdLg1m2HAY1W6rfBrtauHkBhbfA4AQ3iazaJj5wVPhwgaRCHBW2DBg/<0;1>/*),and_v(v:pkh(xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/24/32/<0;1>/*),older(57600))))#l6dlpc2l"); // We can't pass a raw key, an xpub that is not deriveable, only hardened derivable, // without both the change and receive derivation paths, or with more than 2 different // derivation paths. let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/<0;1>/354").unwrap(); - InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + MultipathDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/<0;1>/*'").unwrap(); - InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + MultipathDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); let heir_key = descriptor::DescriptorPublicKey::from_str( "02e24913be26dbcfdf8e8e94870b28725cdae09b448b6c127767bf0154e3a3c8e5", ) .unwrap(); - InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + MultipathDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/0/*'").unwrap(); - InheritanceDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); + MultipathDescriptor::new(owner_key.clone(), heir_key, timelock).unwrap_err(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub661MyMwAqRbcFfxf71L4Dx4w5TmyNXrBicTEAM7vLzumxangwATWWgdJPb6xH1JHcJH9S3jNZx3fCnkkB1WyqrqGgavj1rehHcbythmruvZ/<0;1;2>/*'").unwrap(); - InheritanceDescriptor::new(owner_key, heir_key, timelock).unwrap_err(); + MultipathDescriptor::new(owner_key, heir_key, timelock).unwrap_err(); } #[test] fn inheritance_descriptor_derivation() { let secp = secp256k1::Secp256k1::verification_only(); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9").unwrap(); - let der_desc = desc.derive_receive(11.into(), &secp); + let desc = MultipathDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))#5f6qd0d9").unwrap(); + let der_desc = desc.receive_descriptor().derive(11.into(), &secp); assert_eq!( "bc1q26gtczlz03u6juf5cxppapk4sr4fyz53s3g4zs2cgactcahqv6yqc2t8e6", der_desc.address(bitcoin::Network::Bitcoin).to_string() @@ -591,13 +603,13 @@ mod tests { #[test] fn inheritance_descriptor_tl_value() { - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(1),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); + let desc = MultipathDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(1),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 1); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(42000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); + let desc = MultipathDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(42000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 42000); - let desc = InheritanceDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(65535),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); + let desc = MultipathDescriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/<0;1>/*),older(65535),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/<0;1>/*)))").unwrap(); assert_eq!(desc.timelock_value(), 0xffff); } diff --git a/src/lib.rs b/src/lib.rs index 168fc199..122620da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -384,7 +384,7 @@ mod tests { use super::*; use crate::{ config::{BitcoinConfig, BitcoindConfig}, - descriptors::InheritanceDescriptor, + descriptors::MultipathDescriptor, testutils::*, }; @@ -599,7 +599,7 @@ mod tests { // Create a dummy config with this bitcoind let desc_str = "wsh(andor(pk(xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/<0;1>/*),older(10000),pk(xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*)))#yudtr0k5"; - let desc = InheritanceDescriptor::from_str(desc_str).unwrap(); + let desc = MultipathDescriptor::from_str(desc_str).unwrap(); let receive_desc = desc.receive_descriptor(); let change_desc = desc.change_descriptor(); let config = Config { diff --git a/src/testutils.rs b/src/testutils.rs index bf3282f7..78fe9fd2 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -277,7 +277,7 @@ impl DummyMinisafe { let owner_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/<0;1>/*").unwrap(); let heir_key = descriptor::DescriptorPublicKey::from_str("xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*").unwrap(); let desc = - crate::descriptors::InheritanceDescriptor::new(owner_key, heir_key, 10_000).unwrap(); + crate::descriptors::MultipathDescriptor::new(owner_key, heir_key, 10_000).unwrap(); let config = Config { bitcoin_config, bitcoind_config: None, From 4f3daa7741b6996c17133dacedb4e70c66f5bac8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 09:39:32 +0200 Subject: [PATCH 10/14] descriptors: cache the receive and change descriptors --- src/bitcoin/poller/looper.rs | 5 +- src/descriptors.rs | 97 +++++++++++++++++++----------------- src/lib.rs | 4 +- 3 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 25084e50..93ea97df 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -223,7 +223,10 @@ pub fn looper( ) { let mut last_poll = None; let mut synced = false; - let descs = [desc.receive_descriptor(), desc.change_descriptor()]; + let descs = [ + desc.receive_descriptor().clone(), + desc.change_descriptor().clone(), + ]; maybe_initialize_tip(&bit, &db); diff --git a/src/descriptors.rs b/src/descriptors.rs index f5fcb02c..40fabdc2 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -194,7 +194,11 @@ fn is_valid_desc_key(key: &descriptor::DescriptorPublicKey) -> bool { /// An [InheritanceDescriptor] that contains multipath keys for (and only for) the receive keychain /// and the change keychain. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct MultipathDescriptor(descriptor::Descriptor); +pub struct MultipathDescriptor { + multi_desc: descriptor::Descriptor, + receive_desc: InheritanceDescriptor, + change_desc: InheritanceDescriptor, +} /// A Miniscript descriptor with a main, unencombered, branch (the main owner of the coins) /// and a timelocked branch (the heir). All keys in this descriptor are singlepath. @@ -207,7 +211,7 @@ pub struct DerivedInheritanceDescriptor(descriptor::Descriptor impl fmt::Display for MultipathDescriptor { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.0) + write!(f, "{}", self.multi_desc) } } @@ -277,8 +281,26 @@ impl str::FromStr for MultipathDescriptor { .iter() .find(|s| matches!(s, SemanticPolicy::Key(_))) .ok_or(DescCreationError::IncompatibleDesc)?; + let multi_desc = descriptor::Descriptor::Wsh(wsh_desc); - Ok(MultipathDescriptor(descriptor::Descriptor::Wsh(wsh_desc))) + // Compute the receive and change "sub" descriptors right away. According to our pubkey + // check above, there must be only two of those, 0 and 1. + // We use /0/* for receiving and /1/* for change. + // FIXME: don't rely on into_single_descs()'s ordering. + let mut singlepath_descs = multi_desc + .clone() + .into_single_descriptors() + .expect("Can't error, all paths have the same length") + .into_iter(); + assert_eq!(singlepath_descs.len(), 2); + let receive_desc = InheritanceDescriptor(singlepath_descs.next().expect("First of 2")); + let change_desc = InheritanceDescriptor(singlepath_descs.next().expect("Second of 2")); + + Ok(MultipathDescriptor { + multi_desc, + receive_desc, + change_desc, + }) } } @@ -341,15 +363,33 @@ impl MultipathDescriptor { .expect("Well typed"); miniscript::Segwitv0::check_local_validity(&tl_miniscript) .expect("Miniscript must be sane"); - - Ok(MultipathDescriptor(descriptor::Descriptor::Wsh( + let multi_desc = descriptor::Descriptor::Wsh( descriptor::Wsh::new(tl_miniscript).expect("Must pass sanity checks"), - ))) + ); + + // Compute the receive and change "sub" descriptors right away. According to our pubkey + // check above, there must be only two of those, 0 and 1. + // We use /0/* for receiving and /1/* for change. + // FIXME: don't rely on into_single_descs()'s ordering. + let mut singlepath_descs = multi_desc + .clone() + .into_single_descriptors() + .expect("Can't error, all paths have the same length") + .into_iter(); + assert_eq!(singlepath_descs.len(), 2); + let receive_desc = InheritanceDescriptor(singlepath_descs.next().expect("First of 2")); + let change_desc = InheritanceDescriptor(singlepath_descs.next().expect("Second of 2")); + + Ok(MultipathDescriptor { + multi_desc, + receive_desc, + change_desc, + }) } /// Whether all xpubs contained in this descriptor are for the passed expected network. pub fn all_xpubs_net_is(&self, expected_net: bitcoin::Network) -> bool { - self.0.for_each_key(|xpub| { + self.multi_desc.for_each_key(|xpub| { if let descriptor::DescriptorPublicKey::MultiXPub(xpub) = xpub { xpub.xkey.network == expected_net } else { @@ -358,52 +398,19 @@ impl MultipathDescriptor { }) } - // TODO: Cache it inside the struct, it's very inefficient to use into_single_descriptors() for - // every single derivation. /// Get the descriptor for receiving addresses. - pub fn receive_descriptor(&self) -> InheritanceDescriptor { - let singlepath_descs = self - .0 - .clone() - .into_single_descriptors() - .expect("Can't error, all paths have the same length"); - assert_eq!(singlepath_descs.len(), 2); - - // We use /0/* for receiving, so it's the first descriptor between <0;1>. - // FIXME: don't rely on ordering. - InheritanceDescriptor( - singlepath_descs - .into_iter() - .next() - .expect("Just checked the length"), - ) + pub fn receive_descriptor(&self) -> &InheritanceDescriptor { + &self.receive_desc } - // TODO: Cache it inside the struct, it's very inefficient to use into_single_descriptors() for - // every single derivation. /// Get the descriptor for change addresses. - pub fn change_descriptor(&self) -> InheritanceDescriptor { - let singlepath_descs = self - .0 - .clone() - .into_single_descriptors() - .expect("Can't error, all paths have the same length"); - assert_eq!(singlepath_descs.len(), 2); - - // We use /1/* for change, so it's the second descriptor between <0;1>. - // FIXME: don't rely on ordering. - InheritanceDescriptor( - singlepath_descs - .into_iter() - .rev() - .next() - .expect("Just checked the length"), - ) + pub fn change_descriptor(&self) -> &InheritanceDescriptor { + &self.change_desc } /// Get the value (in blocks) of the relative timelock for the heir's spending path. pub fn timelock_value(&self) -> u32 { - let wsh_desc = match &self.0 { + let wsh_desc = match &self.multi_desc { descriptor::Descriptor::Wsh(desc) => desc, _ => unreachable!(), }; diff --git a/src/lib.rs b/src/lib.rs index 122620da..19271982 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -600,8 +600,8 @@ mod tests { // Create a dummy config with this bitcoind let desc_str = "wsh(andor(pk(xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/<0;1>/*),older(10000),pk(xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*)))#yudtr0k5"; let desc = MultipathDescriptor::from_str(desc_str).unwrap(); - let receive_desc = desc.receive_descriptor(); - let change_desc = desc.change_descriptor(); + let receive_desc = desc.receive_descriptor().clone(); + let change_desc = desc.change_descriptor().clone(); let config = Config { bitcoin_config, bitcoind_config: Some(bitcoind_config), From 9b04a551474b2cd5ed793e42832454635d964495 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 10:01:33 +0200 Subject: [PATCH 11/14] db: store derivation index also for addresses from the change desc This doubles the storage required but there is no way around it if we want the poller to detect those coins without grinding. --- src/database/sqlite/mod.rs | 39 ++++++++++++++++++++++++++++------- src/database/sqlite/schema.rs | 20 ++++++++++++------ src/database/sqlite/utils.rs | 12 +++++++---- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 3b672df4..50c92f4d 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -238,17 +238,22 @@ impl SqliteConn { // Update the address to derivation index mapping. // TODO: have this as a helper in descriptors.rs let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; - let next_la_address = db_wallet + let next_receive_address = db_wallet .main_descriptor .receive_descriptor() .derive(next_la_index.into(), secp) .address(network); - db_tx - .execute( - "INSERT INTO addresses (address, derivation_index) VALUES (?1, ?2)", - rusqlite::params![next_la_address.to_string(), next_la_index], - ) - .map(|_| ()) + let next_change_address = db_wallet + .main_descriptor + .change_descriptor() + .derive(next_la_index.into(), secp) + .address(network); + db_tx.execute( + "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (?1, ?2, ?3)", + rusqlite::params![next_receive_address.to_string(), next_change_address.to_string(), next_la_index], + )?; + + Ok(()) }) .expect("Database must be available") } @@ -363,7 +368,7 @@ impl SqliteConn { pub fn db_address(&mut self, address: &bitcoin::Address) -> Option { db_query( &mut self.conn, - "SELECT * FROM addresses WHERE address = ?1", + "SELECT * FROM addresses WHERE receive_address = ?1 OR change_address = ?1", rusqlite::params![address.to_string()], |row| row.try_into(), ) @@ -721,6 +726,15 @@ mod tests { let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 0.into()); + // And also for the change address + let addr = options + .main_descriptor + .change_descriptor() + .derive(0.into(), &secp) + .address(options.bitcoind_network); + let db_addr = conn.db_address(&addr).unwrap(); + assert_eq!(db_addr.derivation_index, 0.into()); + // There is the index for the 199th index (look-ahead limit) let addr = options .main_descriptor @@ -742,6 +756,15 @@ mod tests { conn.increment_derivation_index(&secp); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 200.into()); + + // Same for the change descriptor. + let addr = options + .main_descriptor + .change_descriptor() + .derive(200.into(), &secp) + .address(options.bitcoind_network); + let db_addr = conn.db_address(&addr).unwrap(); + assert_eq!(db_addr.derivation_index, 200.into()); } fs::remove_dir_all(&tmp_dir).unwrap(); diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 99975e48..87c55bb5 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -57,7 +57,8 @@ CREATE TABLE coins ( * we can get the derivation index from the parent descriptor from bitcoind. */ CREATE TABLE addresses ( - address TEXT NOT NULL UNIQUE, + receive_address TEXT NOT NULL UNIQUE, + change_address TEXT NOT NULL UNIQUE, derivation_index INTEGER NOT NULL UNIQUE ); @@ -195,7 +196,8 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { #[derive(Debug, Clone, PartialEq, Eq)] pub struct DbAddress { - pub address: bitcoin::Address, + pub receive_address: bitcoin::Address, + pub change_address: bitcoin::Address, pub derivation_index: bip32::ChildNumber, } @@ -203,15 +205,21 @@ impl TryFrom<&rusqlite::Row<'_>> for DbAddress { type Error = rusqlite::Error; fn try_from(row: &rusqlite::Row) -> Result { - let address: String = row.get(0)?; - let address = bitcoin::Address::from_str(&address).expect("We only store valid addresses"); + let receive_address: String = row.get(0)?; + let receive_address = + bitcoin::Address::from_str(&receive_address).expect("We only store valid addresses"); - let derivation_index: u32 = row.get(1)?; + let change_address: String = row.get(1)?; + let change_address = + bitcoin::Address::from_str(&change_address).expect("We only store valid addresses"); + + let derivation_index: u32 = row.get(2)?; let derivation_index = bip32::ChildNumber::from(derivation_index); assert!(derivation_index.is_normal()); Ok(DbAddress { - address, + receive_address, + change_address, derivation_index, }) } diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index 43a3102c..82d09d84 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -95,15 +95,19 @@ pub fn create_fresh_db( // necessarily 0. let mut query = String::with_capacity(100 * LOOK_AHEAD_LIMIT as usize); for index in 0..LOOK_AHEAD_LIMIT { - // TODO: have this as a helper in descriptors.rs - let address = options + let receive_address = options .main_descriptor .receive_descriptor() .derive(index.into(), secp) .address(options.bitcoind_network); + let change_address = options + .main_descriptor + .change_descriptor() + .derive(index.into(), secp) + .address(options.bitcoind_network); query += &format!( - "INSERT INTO addresses (address, derivation_index) VALUES (\"{}\", {});\n", - address, index + "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (\"{}\", \"{}\", {});\n", + receive_address, change_address, index ); } From 58a0e57c59bf7b4f451580917f701e95e45d1af6 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 10:41:12 +0200 Subject: [PATCH 12/14] db: record whether a coin was received on a change address So we know what descriptor to use when spending it. --- src/bitcoin/poller/looper.rs | 5 ++++- src/commands/mod.rs | 3 +++ src/database/mod.rs | 10 +++++++--- src/database/sqlite/mod.rs | 14 +++++++++++--- src/database/sqlite/schema.rs | 10 +++++++--- src/testutils.rs | 5 ++++- 6 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 93ea97df..7d39124c 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -35,7 +35,9 @@ fn update_coins( // Start by fetching newly received coins. let mut received = Vec::new(); for utxo in bit.received_coins(previous_tip, descs) { - if let Some(derivation_index) = db_conn.derivation_index_by_address(&utxo.address) { + if let Some((derivation_index, is_change)) = + db_conn.derivation_index_by_address(&utxo.address) + { if !curr_coins.contains_key(&utxo.outpoint) { let UTxO { outpoint, amount, .. @@ -44,6 +46,7 @@ fn update_coins( outpoint, amount, derivation_index, + is_change, block_height: None, block_time: None, spend_txid: None, diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 6c04752d..92d5e19d 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -627,6 +627,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(100_000), derivation_index: bip32::ChildNumber::from(13), + is_change: false, spend_txid: None, spend_block: None, }]); @@ -721,6 +722,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(100_000), derivation_index: bip32::ChildNumber::from(13), + is_change: false, spend_txid: None, spend_block: None, }, @@ -730,6 +732,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(115_680), derivation_index: bip32::ChildNumber::from(34), + is_change: false, spend_txid: None, spend_block: None, }, diff --git a/src/database/mod.rs b/src/database/mod.rs index c9e2d88b..6097248e 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -49,10 +49,11 @@ pub trait DatabaseConnection { fn increment_derivation_index(&mut self, secp: &secp256k1::Secp256k1); + /// Get the derivation index for this address, as well as whether this address is change. fn derivation_index_by_address( &mut self, address: &bitcoin::Address, - ) -> Option; + ) -> Option<(bip32::ChildNumber, bool)>; /// Get all our coins, past or present, spent or not. fn coins(&mut self) -> HashMap; @@ -154,9 +155,9 @@ impl DatabaseConnection for SqliteConn { fn derivation_index_by_address( &mut self, address: &bitcoin::Address, - ) -> Option { + ) -> Option<(bip32::ChildNumber, bool)> { self.db_address(address) - .map(|db_addr| db_addr.derivation_index) + .map(|db_addr| (db_addr.derivation_index, address == &db_addr.change_address)) } fn coins_by_outpoints( @@ -215,6 +216,7 @@ pub struct Coin { pub block_time: Option, pub amount: bitcoin::Amount, pub derivation_index: bip32::ChildNumber, + pub is_change: bool, pub spend_txid: Option, pub spend_block: Option, } @@ -227,6 +229,7 @@ impl std::convert::From for Coin { block_time, amount, derivation_index, + is_change, spend_txid, spend_block, .. @@ -237,6 +240,7 @@ impl std::convert::From for Coin { block_time, amount, derivation_index, + is_change, spend_txid, spend_block: spend_block.map(SpendBlock::from), } diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 50c92f4d..d6b14d30 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -288,14 +288,15 @@ impl SqliteConn { for coin in coins { let deriv_index: u32 = coin.derivation_index.into(); db_tx.execute( - "INSERT INTO coins (wallet_id, txid, vout, amount_sat, derivation_index) \ - VALUES (?1, ?2, ?3, ?4, ?5)", + "INSERT INTO coins (wallet_id, txid, vout, amount_sat, derivation_index, is_change) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", rusqlite::params![ WALLET_ID, coin.outpoint.txid.to_vec(), coin.outpoint.vout, coin.amount.to_sat(), deriv_index, + coin.is_change, ], )?; } @@ -607,6 +608,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), + is_change: false, spend_txid: None, spend_block: None, }; @@ -618,7 +620,7 @@ mod tests { assert_eq!(coins.len(), 1); assert_eq!(coins[0].outpoint, coin_a.outpoint); - // Add a second one, we'll get both. + // Add a second one (this one is change), we'll get both. let coin_b = Coin { outpoint: bitcoin::OutPoint::from_str( "61db3e276b095e5b05f1849dd6bfffb4e7e5ec1c4a4210099b98fce01571936f:12", @@ -628,6 +630,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(1111), derivation_index: bip32::ChildNumber::from_normal_idx(103).unwrap(), + is_change: true, spend_txid: None, spend_block: None, }; @@ -802,6 +805,7 @@ mod tests { block_time: None, amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), + is_change: false, spend_txid: None, spend_block: None, }, @@ -814,6 +818,7 @@ mod tests { block_time: Some(1_111_899), amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(100).unwrap(), + is_change: false, spend_txid: None, spend_block: None, }, @@ -826,6 +831,7 @@ mod tests { block_time: Some(1_121_899), amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(1000).unwrap(), + is_change: false, spend_txid: Some( bitcoin::Txid::from_str( "0c62a990d20d54429e70859292e82374ba6b1b951a3ab60f26bb65fee5724ff7", @@ -846,6 +852,7 @@ mod tests { block_time: Some(1_131_899), amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10000).unwrap(), + is_change: false, spend_txid: None, spend_block: None, }, @@ -858,6 +865,7 @@ mod tests { block_time: Some(1_134_899), amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(100000).unwrap(), + is_change: false, spend_txid: Some( bitcoin::Txid::from_str( "7477017f992cdc7ba08acafb77cb3b5bc0f42ac340d3e1e1da0785bdda20d5f6", diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 87c55bb5..1f729d41 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -44,6 +44,7 @@ CREATE TABLE coins ( vout INTEGER NOT NULL, amount_sat INTEGER NOT NULL, derivation_index INTEGER NOT NULL, + is_change BOOLEAN NOT NULL CHECK (is_change IN (0,1)), spend_txid BLOB, spend_block_height INTEGER, spend_block_time INTEGER, @@ -146,6 +147,7 @@ pub struct DbCoin { pub block_time: Option, pub amount: bitcoin::Amount, pub derivation_index: bip32::ChildNumber, + pub is_change: bool, pub spend_txid: Option, pub spend_block: Option, } @@ -168,12 +170,13 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { let amount = bitcoin::Amount::from_sat(amount); let der_idx: u32 = row.get(7)?; let derivation_index = bip32::ChildNumber::from(der_idx); + let is_change: bool = row.get(8)?; - let spend_txid: Option> = row.get(8)?; + let spend_txid: Option> = row.get(9)?; let spend_txid = spend_txid.map(|txid| encode::deserialize(&txid).expect("We only store valid txids")); - let spend_height: Option = row.get(9)?; - let spend_time: Option = row.get(10)?; + let spend_height: Option = row.get(10)?; + let spend_time: Option = row.get(11)?; assert_eq!(spend_height.is_none(), spend_time.is_none()); let spend_block = spend_height.map(|height| DbSpendBlock { height, @@ -188,6 +191,7 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { block_time, amount, derivation_index, + is_change, spend_txid, spend_block, }) diff --git a/src/testutils.rs b/src/testutils.rs index 78fe9fd2..ea13d1d2 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -187,7 +187,10 @@ impl DatabaseConnection for DummyDbConn { } } - fn derivation_index_by_address(&mut self, _: &bitcoin::Address) -> Option { + fn derivation_index_by_address( + &mut self, + _: &bitcoin::Address, + ) -> Option<(bip32::ChildNumber, bool)> { None } From d9f905a19a5c6076683bbe7714ecb86fbafe0555 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 12:01:06 +0200 Subject: [PATCH 13/14] db: track the next unused derivation index for change, too --- src/commands/mod.rs | 4 +- src/database/mod.rs | 22 +++++++--- src/database/sqlite/mod.rs | 82 +++++++++++++++++++++++++---------- src/database/sqlite/schema.rs | 7 ++- src/database/sqlite/utils.rs | 41 +++++++++++++++--- src/testutils.rs | 25 ++++++++--- 6 files changed, 139 insertions(+), 42 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 92d5e19d..6f0e9a9a 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -200,9 +200,9 @@ impl DaemonControl { /// whether it was actually used. pub fn get_new_address(&self) -> GetAddressResult { let mut db_conn = self.db.connection(); - let index = db_conn.derivation_index(); + let index = db_conn.receive_index(); // TODO: should we wrap around instead of failing? - db_conn.increment_derivation_index(&self.secp); + db_conn.increment_receive_index(&self.secp); let address = self .derived_desc(index) .address(self.config.bitcoin_config.network); diff --git a/src/database/mod.rs b/src/database/mod.rs index 6097248e..20795bb1 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -45,9 +45,13 @@ pub trait DatabaseConnection { /// Update our best chain seen. fn update_tip(&mut self, tip: &BlockChainTip); - fn derivation_index(&mut self) -> bip32::ChildNumber; + fn receive_index(&mut self) -> bip32::ChildNumber; - fn increment_derivation_index(&mut self, secp: &secp256k1::Secp256k1); + fn change_index(&mut self) -> bip32::ChildNumber; + + fn increment_receive_index(&mut self, secp: &secp256k1::Secp256k1); + + fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1); /// Get the derivation index for this address, as well as whether this address is change. fn derivation_index_by_address( @@ -114,12 +118,20 @@ impl DatabaseConnection for SqliteConn { self.update_tip(tip) } - fn derivation_index(&mut self) -> bip32::ChildNumber { + fn receive_index(&mut self) -> bip32::ChildNumber { self.db_wallet().deposit_derivation_index } - fn increment_derivation_index(&mut self, secp: &secp256k1::Secp256k1) { - self.increment_derivation_index(secp) + fn change_index(&mut self) -> bip32::ChildNumber { + self.db_wallet().change_derivation_index + } + + fn increment_receive_index(&mut self, secp: &secp256k1::Secp256k1) { + self.increment_deposit_index(secp) + } + + fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1) { + self.increment_change_index(secp) } fn coins(&mut self) -> HashMap { diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index d6b14d30..dc15827b 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -14,7 +14,7 @@ use crate::{ database::{ sqlite::{ schema::{DbAddress, DbCoin, DbSpendTransaction, DbTip, DbWallet}, - utils::{create_fresh_db, db_exec, db_query, db_tx_query, LOOK_AHEAD_LIMIT}, + utils::{create_fresh_db, db_exec, db_query, db_tx_query, populate_address_mapping}, }, Coin, }, @@ -210,10 +210,7 @@ impl SqliteConn { .expect("Database must be available") } - pub fn increment_derivation_index( - &mut self, - secp: &secp256k1::Secp256k1, - ) { + pub fn increment_deposit_index(&mut self, secp: &secp256k1::Secp256k1) { let network = self.db_tip().network; db_exec(&mut self.conn, |db_tx| { @@ -235,24 +232,41 @@ impl SqliteConn { rusqlite::params![next_index], )?; - // Update the address to derivation index mapping. - // TODO: have this as a helper in descriptors.rs - let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; - let next_receive_address = db_wallet - .main_descriptor - .receive_descriptor() - .derive(next_la_index.into(), secp) - .address(network); - let next_change_address = db_wallet - .main_descriptor - .change_descriptor() - .derive(next_la_index.into(), secp) - .address(network); + if next_index > db_wallet.change_derivation_index.into() { + populate_address_mapping(db_tx, &db_wallet, next_index, network, secp)?; + } + + Ok(()) + }) + .expect("Database must be available") + } + + pub fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1) { + let network = self.db_tip().network; + + db_exec(&mut self.conn, |db_tx| { + let db_wallet: DbWallet = + db_tx_query(db_tx, "SELECT * FROM wallets", rusqlite::params![], |row| { + row.try_into() + }) + .expect("Db must not fail") + .pop() + .expect("There is always a row in the wallet table"); + let next_index: u32 = db_wallet + .change_derivation_index + .increment() + .expect("Must not get in hardened territory") + .into(); + // NOTE: should be updated if we ever have multi-wallet support db_tx.execute( - "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (?1, ?2, ?3)", - rusqlite::params![next_receive_address.to_string(), next_change_address.to_string(), next_la_index], + "UPDATE wallets SET change_derivation_index = (?1)", + rusqlite::params![next_index], )?; + if next_index > db_wallet.deposit_derivation_index.into() { + populate_address_mapping(db_tx, &db_wallet, next_index, network, secp)?; + } + Ok(()) }) .expect("Database must be available") @@ -756,11 +770,11 @@ mod tests { assert!(conn.db_address(&addr).is_none()); // But if we increment the deposit derivation index, the 200th one will be there. - conn.increment_derivation_index(&secp); + conn.increment_deposit_index(&secp); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 200.into()); - // Same for the change descriptor. + // It will also be there for the change descriptor. let addr = options .main_descriptor .change_descriptor() @@ -768,6 +782,30 @@ mod tests { .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 200.into()); + + // But not for the 201th. + let addr = options + .main_descriptor + .change_descriptor() + .derive(201.into(), &secp) + .address(options.bitcoind_network); + assert!(conn.db_address(&addr).is_none()); + + // If we increment the *change* derivation index to 1, it will still not be there. + conn.increment_change_index(&secp); + assert!(conn.db_address(&addr).is_none()); + + // But doing it once again it will be there for both change and receive. + conn.increment_change_index(&secp); + let db_addr = conn.db_address(&addr).unwrap(); + assert_eq!(db_addr.derivation_index, 201.into()); + let addr = options + .main_descriptor + .receive_descriptor() + .derive(201.into(), &secp) + .address(options.bitcoind_network); + let db_addr = conn.db_address(&addr).unwrap(); + assert_eq!(db_addr.derivation_index, 201.into()); } fs::remove_dir_all(&tmp_dir).unwrap(); diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 1f729d41..40398df7 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -27,7 +27,8 @@ CREATE TABLE wallets ( id INTEGER PRIMARY KEY NOT NULL, timestamp INTEGER NOT NULL, main_descriptor TEXT NOT NULL, - deposit_derivation_index INTEGER NOT NULL + deposit_derivation_index INTEGER NOT NULL, + change_derivation_index INTEGER NOT NULL ); /* Our (U)TxOs. @@ -107,6 +108,7 @@ pub struct DbWallet { pub timestamp: u32, pub main_descriptor: MultipathDescriptor, pub deposit_derivation_index: bip32::ChildNumber, + pub change_derivation_index: bip32::ChildNumber, } impl TryFrom<&rusqlite::Row<'_>> for DbWallet { @@ -122,12 +124,15 @@ impl TryFrom<&rusqlite::Row<'_>> for DbWallet { let der_idx: u32 = row.get(3)?; let deposit_derivation_index = bip32::ChildNumber::from(der_idx); + let der_idx: u32 = row.get(4)?; + let change_derivation_index = bip32::ChildNumber::from(der_idx); Ok(DbWallet { id, timestamp, main_descriptor, deposit_derivation_index, + change_derivation_index, }) } } diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index 82d09d84..c091d188 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -1,8 +1,11 @@ -use crate::database::sqlite::{schema::SCHEMA, FreshDbOptions, SqliteDbError, DB_VERSION}; +use crate::database::sqlite::{ + schema::{DbWallet, SCHEMA}, + FreshDbOptions, SqliteDbError, DB_VERSION, +}; use std::{convert::TryInto, fs, path, time}; -use miniscript::bitcoin::secp256k1; +use miniscript::bitcoin::{self, secp256k1}; pub const LOOK_AHEAD_LIMIT: u32 = 200; @@ -123,9 +126,9 @@ pub fn create_fresh_db( rusqlite::params![options.bitcoind_network.to_string()], )?; tx.execute( - "INSERT INTO wallets (timestamp, main_descriptor, deposit_derivation_index) \ - VALUES (?1, ?2, ?3)", - rusqlite::params![timestamp, options.main_descriptor.to_string(), 0,], + "INSERT INTO wallets (timestamp, main_descriptor, deposit_derivation_index, change_derivation_index) \ + VALUES (?1, ?2, ?3, ?4)", + rusqlite::params![timestamp, options.main_descriptor.to_string(), 0, 0], )?; tx.execute_batch(&query)?; @@ -134,3 +137,31 @@ pub fn create_fresh_db( Ok(()) } + +/// Insert the deposit and change addresses for this index in the address->index mapping table +pub fn populate_address_mapping( + db_tx: &rusqlite::Transaction, + db_wallet: &DbWallet, + next_index: u32, + network: bitcoin::Network, + secp: &secp256k1::Secp256k1, +) -> rusqlite::Result<()> { + // Update the address to derivation index mapping. + let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; + let next_receive_address = db_wallet + .main_descriptor + .receive_descriptor() + .derive(next_la_index.into(), secp) + .address(network); + let next_change_address = db_wallet + .main_descriptor + .change_descriptor() + .derive(next_la_index.into(), secp) + .address(network); + db_tx.execute( + "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (?1, ?2, ?3)", + rusqlite::params![next_receive_address.to_string(), next_change_address.to_string(), next_la_index], + )?; + + Ok(()) +} diff --git a/src/testutils.rs b/src/testutils.rs index ea13d1d2..eff9da06 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -77,7 +77,8 @@ impl BitcoinInterface for DummyBitcoind { } pub struct DummyDb { - curr_index: bip32::ChildNumber, + deposit_index: bip32::ChildNumber, + change_index: bip32::ChildNumber, curr_tip: Option, coins: HashMap, spend_txs: HashMap, @@ -86,7 +87,8 @@ pub struct DummyDb { impl DummyDb { pub fn new() -> DummyDb { DummyDb { - curr_index: 0.into(), + deposit_index: 0.into(), + change_index: 0.into(), curr_tip: None, coins: HashMap::new(), spend_txs: HashMap::new(), @@ -123,13 +125,22 @@ impl DatabaseConnection for DummyDbConn { self.db.write().unwrap().curr_tip = Some(*tip); } - fn derivation_index(&mut self) -> bip32::ChildNumber { - self.db.read().unwrap().curr_index + fn receive_index(&mut self) -> bip32::ChildNumber { + self.db.read().unwrap().deposit_index } - fn increment_derivation_index(&mut self, _: &secp256k1::Secp256k1) { - let next_index = self.db.write().unwrap().curr_index.increment().unwrap(); - self.db.write().unwrap().curr_index = next_index; + fn change_index(&mut self) -> bip32::ChildNumber { + self.db.read().unwrap().deposit_index + } + + fn increment_receive_index(&mut self, _: &secp256k1::Secp256k1) { + let next_index = self.db.write().unwrap().deposit_index.increment().unwrap(); + self.db.write().unwrap().deposit_index = next_index; + } + + fn increment_change_index(&mut self, _: &secp256k1::Secp256k1) { + let next_index = self.db.write().unwrap().change_index.increment().unwrap(); + self.db.write().unwrap().change_index = next_index; } fn coins(&mut self) -> HashMap { From 117171f24ff5bd6731d9e5e50e4515a03013a9eb Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 24 Oct 2022 13:13:22 +0200 Subject: [PATCH 14/14] commands: use a separate key chain for change addresses --- src/commands/mod.rs | 37 +++++++++++++++++------------- tests/test_spend.py | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 tests/test_spend.py diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 6f0e9a9a..da16d6c5 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -20,7 +20,6 @@ use std::{ use miniscript::{ bitcoin::{ self, - util::bip32, util::psbt::{Input as PsbtIn, Output as PsbtOut, PartiallySignedTransaction as Psbt}, }, psbt::PsbtExt, @@ -170,12 +169,14 @@ fn serializable_size(t: &T) -> u64 { } impl DaemonControl { - // Get the descriptor at this derivation index - fn derived_desc(&self, index: bip32::ChildNumber) -> descriptors::DerivedInheritanceDescriptor { - self.config - .main_descriptor - .receive_descriptor() - .derive(index, &self.secp) + // Get the derived descriptor for this coin + fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedInheritanceDescriptor { + let desc = if coin.is_change { + self.config.main_descriptor.change_descriptor() + } else { + self.config.main_descriptor.receive_descriptor() + }; + desc.derive(coin.derivation_index, &self.secp) } } @@ -204,7 +205,10 @@ impl DaemonControl { // TODO: should we wrap around instead of failing? db_conn.increment_receive_index(&self.secp); let address = self - .derived_desc(index) + .config + .main_descriptor + .receive_descriptor() + .derive(index, &self.secp) .address(self.config.bitcoin_config.network); GetAddressResult { address } } @@ -279,7 +283,7 @@ impl DaemonControl { ..bitcoin::TxIn::default() }); - let coin_desc = self.derived_desc(coin.derivation_index); + let coin_desc = self.derived_desc(coin); sat_vb += desc_sat_vb(&coin_desc); let witness_script = Some(coin_desc.witness_script()); let witness_utxo = Some(bitcoin::TxOut { @@ -341,14 +345,15 @@ impl DaemonControl { // an added output* (for the change). if nochange_feerate_vb > feerate_vb { // Get the change address to create a dummy change txo. - // TODO: decent change management - let first_coin = coins - .get(coins_outpoints.get(0).expect("We checked it wasn't empty")) - .expect("We checked they were all present"); - let coin_desc = self.derived_desc(first_coin.derivation_index); + let change_desc = self + .config + .main_descriptor + .receive_descriptor() + .derive(db_conn.change_index(), &self.secp); + db_conn.increment_change_index(&self.secp); let mut change_txo = bitcoin::TxOut { value: std::u64::MAX, - script_pubkey: coin_desc.script_pubkey(), + script_pubkey: change_desc.script_pubkey(), }; // Serialized size is equal to the virtual size for an output. let change_vb: u64 = serializable_size(&change_txo); @@ -555,6 +560,8 @@ mod tests { use crate::testutils::*; use std::str::FromStr; + use bitcoin::util::bip32; + #[test] fn getinfo() { let ms = DummyMinisafe::new(); diff --git a/tests/test_spend.py b/tests/test_spend.py new file mode 100644 index 00000000..508d9c06 --- /dev/null +++ b/tests/test_spend.py @@ -0,0 +1,55 @@ +from fixtures import * +from test_framework.serializations import PSBT +from test_framework.utils import wait_for + + +def test_spend_change(minisafed, bitcoind): + """We can spend a coin that was received on a change address.""" + # Receive a coin on a receive address + addr = minisafed.rpc.getnewaddress()["address"] + txid = bitcoind.rpc.sendtoaddress(addr, 0.01) + bitcoind.generate_block(1, wait_for_mempool=txid) + wait_for(lambda: len(minisafed.rpc.listcoins()["coins"]) == 1) + + # Create a transaction that will spend this coin to 1) one of our receive + # addresses 2) an external address 3) one of our change addresses. + outpoints = [c["outpoint"] for c in minisafed.rpc.listcoins()["coins"]] + destinations = { + bitcoind.rpc.getnewaddress(): 100_000, + minisafed.rpc.getnewaddress()["address"]: 100_000, + } + res = minisafed.rpc.createspend(outpoints, destinations, 2) + assert "psbt" in res + + # The transaction must contain a change output. + spend_psbt = PSBT.from_base64(res["psbt"]) + assert len(spend_psbt.o) == 3 + assert len(spend_psbt.tx.vout) == 3 + + # Sign and broadcast this first Spend transaction. + signed_psbt = minisafed.sign_psbt(spend_psbt) + minisafed.rpc.updatespend(signed_psbt.to_base64()) + spend_txid = signed_psbt.tx.txid().hex() + minisafed.rpc.broadcastspend(spend_txid) + bitcoind.generate_block(1, wait_for_mempool=spend_txid) + wait_for(lambda: len(minisafed.rpc.listcoins()["coins"]) == 3) + + # Now create a new transaction that spends the change output as well as + # the output sent to the receive address. + outpoints = [ + c["outpoint"] + for c in minisafed.rpc.listcoins()["coins"] + if c["spend_info"] is None + ] + destinations = { + bitcoind.rpc.getnewaddress(): 100_000, + } + res = minisafed.rpc.createspend(outpoints, destinations, 2) + spend_psbt = PSBT.from_base64(res["psbt"]) + + # We can sign and broadcast it. + signed_psbt = minisafed.sign_psbt(spend_psbt) + minisafed.rpc.updatespend(signed_psbt.to_base64()) + spend_txid = signed_psbt.tx.txid().hex() + minisafed.rpc.broadcastspend(spend_txid) + bitcoind.generate_block(1, wait_for_mempool=spend_txid)