From 2b7ba37a2b757487654c29e4b6db338e31e0a333 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 6 Jul 2022 19:12:16 +0200 Subject: [PATCH 1/4] bitcoind: check all results in 'importdescriptors' result --- src/bitcoind/interface.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/bitcoind/interface.rs b/src/bitcoind/interface.rs index 9d0290e5..a0d70946 100644 --- a/src/bitcoind/interface.rs +++ b/src/bitcoind/interface.rs @@ -401,13 +401,21 @@ impl BitcoinD { "importdescriptors", ¶ms!(Json::Array(all_descriptors)), )?; - if res.get(0).map(|x| x.get("success")) == Some(Some(&Json::Bool(true))) { + let all_succeeded = res + .as_array() + .map(|results| { + results + .iter() + .all(|res| res.get("success") == Some(&Json::Bool(true))) + }) + .unwrap_or(false); + if all_succeeded { return Ok(()); } Err(BitcoindError::Custom(format!( "Error returned from 'importdescriptor': {:?}", - res.get(0).map(|r| r.get("error")) + res ))) } From 6abe1971caea1690c0fa8b2067f1d3cd6c7aa357 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 6 Jul 2022 19:13:41 +0200 Subject: [PATCH 2/4] bitcoind: reduce load in get_spending_txid By ignoring more entries that are obviously not the one we are looking for --- src/bitcoind/interface.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/bitcoind/interface.rs b/src/bitcoind/interface.rs index a0d70946..97ff6dcc 100644 --- a/src/bitcoind/interface.rs +++ b/src/bitcoind/interface.rs @@ -5,7 +5,12 @@ use revault_tx::bitcoin::{ Address, Amount, BlockHash, OutPoint, Script, Transaction, TxOut, Txid, }; -use std::{collections::HashMap, fs, str::FromStr, time::Duration}; +use std::{ + collections::{HashMap, HashSet}, + fs, + str::FromStr, + time::Duration, +}; use jsonrpc::{ arg, @@ -835,6 +840,11 @@ impl BitcoinD { block_hash )); + // Get the spent txid to ignore the entries about this transaction + let spent_txid = spent_outpoint.txid.to_string(); + // We use a cache to avoid needless iterations, since listsinceblock returns an entry + // per transaction output, not per transaction. + let mut visited_txs = HashSet::new(); for transaction in transactions { if transaction.get("category").map(|c| c.as_str()).flatten() != Some("send") { continue; @@ -852,6 +862,12 @@ impl BitcoinD { block_hash )); + if visited_txs.contains(&spending_txid) || &spent_txid == spending_txid { + continue; + } else { + visited_txs.insert(spending_txid); + } + let gettx_res = self.make_watchonly_request( "gettransaction", ¶ms!( From c8ed4f4176d671d6eba07fac87faa70f0d609525 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 6 Jul 2022 19:15:12 +0200 Subject: [PATCH 3/4] bitcoind: import Miniscript descriptors instead of addresses - Don't generate a set of addresses to be imported, import the ranged Miniscript descriptors directly. - Tell listsinceblock to list change, too (custom bitcoind patch). This is necessary because deposit addresses aren't part of bitcoind's address book anymore and we would otherwise fail to detect some deposits. - Use the 'wallet_desc' field in listsinceblock entries to track coins, instead of the former labels imported along with the addresses. This also cleans up the descriptor import routines. --- src/bitcoind/interface.rs | 175 +++++++++++++------------------------- src/bitcoind/poller.rs | 61 ++++--------- 2 files changed, 75 insertions(+), 161 deletions(-) diff --git a/src/bitcoind/interface.rs b/src/bitcoind/interface.rs index 97ff6dcc..b0c5782a 100644 --- a/src/bitcoind/interface.rs +++ b/src/bitcoind/interface.rs @@ -24,11 +24,6 @@ use serde_json::Value as Json; // If bitcoind takes more than 3 minutes to answer one of our queries, fail. const RPC_SOCKET_TIMEOUT: u64 = 180; -// Labels used to tag utxos in the watchonly wallet -const DEPOSIT_UTXOS_LABEL: &str = "revault-deposit"; -const UNVAULT_UTXOS_LABEL: &str = "revault-unvault"; -const CPFP_UTXOS_LABEL: &str = "revault-cpfp"; - pub struct BitcoinD { node_client: Client, watchonly_client: Client, @@ -350,32 +345,14 @@ impl BitcoinD { } } - /// Constructs an `addr()` descriptor out of an address - pub fn addr_descriptor(&self, address: &str) -> Result { - let desc_wo_checksum = format!("addr({})", address); - - Ok(self - .make_watchonly_request( - "getdescriptorinfo", - ¶ms!(Json::String(desc_wo_checksum)), - )? - .get("descriptor") - .expect("No 'descriptor' in 'getdescriptorinfo'") - .as_str() - .expect("'descriptor' in 'getdescriptorinfo' isn't a string anymore") - .to_string()) - } - - fn bulk_import_descriptors( + fn import_descriptors( &self, client: &Client, descriptors: Vec, - timestamp: u32, - label: String, - fresh_wallet: bool, + timestamp: Option, active: bool, ) -> Result<(), BitcoindError> { - if !fresh_wallet { + if timestamp.is_some() { log::debug!("Not a fresh wallet, rescan *may* take some time."); } @@ -388,13 +365,11 @@ impl BitcoinD { // will rescan the last few blocks for each of them. desc_map.insert( "timestamp".to_string(), - if fresh_wallet { - Json::String("now".to_string()) - } else { - Json::Number(serde_json::Number::from(timestamp)) - }, + timestamp + .map(serde_json::Number::from) + .map(Json::Number) + .unwrap_or_else(|| Json::String("now".to_string())), ); - desc_map.insert("label".to_string(), Json::String(label.clone())); desc_map.insert("active".to_string(), Json::Bool(active)); Json::Object(desc_map) @@ -424,84 +399,26 @@ impl BitcoinD { ))) } - pub fn startup_import_deposit_descriptors( - &self, - descriptors: Vec, - timestamp: u32, - fresh_wallet: bool, - ) -> Result<(), BitcoindError> { - self.bulk_import_descriptors( - &self.watchonly_client, - descriptors, - timestamp, - DEPOSIT_UTXOS_LABEL.to_string(), - fresh_wallet, - false, - ) - } - - pub fn startup_import_unvault_descriptors( + /// Import the deposit and Unvault descriptors, when at startup. + pub fn startup_import_descriptors( &self, - descriptors: Vec, - timestamp: u32, - fresh_wallet: bool, + descriptors: [String; 2], + timestamp: Option, ) -> Result<(), BitcoindError> { - self.bulk_import_descriptors( + self.import_descriptors( &self.watchonly_client, - descriptors, + descriptors.to_vec(), timestamp, - UNVAULT_UTXOS_LABEL.to_string(), - fresh_wallet, false, ) } - pub fn startup_import_cpfp_descriptor( - &self, - descriptor: String, - timestamp: u32, - fresh_wallet: bool, - ) -> Result<(), BitcoindError> { - self.bulk_import_descriptors( - &self.cpfp_client, - vec![descriptor], - timestamp, - CPFP_UTXOS_LABEL.to_string(), - fresh_wallet, - true, - ) - } - - fn import_fresh_descriptor( + pub fn import_cpfp_descriptor( &self, descriptor: String, - label: String, + timestamp: Option, ) -> Result<(), BitcoindError> { - let mut desc_map = serde_json::Map::with_capacity(3); - desc_map.insert("desc".to_string(), Json::String(descriptor)); - desc_map.insert("timestamp".to_string(), Json::String("now".to_string())); - desc_map.insert("label".to_string(), Json::String(label)); - - let res = self.make_watchonly_request( - "importdescriptors", - ¶ms!(Json::Array(vec![Json::Object(desc_map,)])), - )?; - if res.get(0).map(|x| x.get("success")).flatten() == Some(&Json::Bool(true)) { - return Ok(()); - } - - Err(BitcoindError::Custom(format!( - "In import_fresh descriptor, no success returned from 'importdescriptor': {:?}", - res - ))) - } - - pub fn import_fresh_deposit_descriptor(&self, descriptor: String) -> Result<(), BitcoindError> { - self.import_fresh_descriptor(descriptor, DEPOSIT_UTXOS_LABEL.to_string()) - } - - pub fn import_fresh_unvault_descriptor(&self, descriptor: String) -> Result<(), BitcoindError> { - self.import_fresh_descriptor(descriptor, UNVAULT_UTXOS_LABEL.to_string()) + self.import_descriptors(&self.cpfp_client, vec![descriptor], timestamp, true) } pub fn list_unspent_cpfp(&self) -> Result, BitcoindError> { @@ -559,7 +476,7 @@ impl BitcoinD { fn list_since_block( &self, tip: &BlockchainTip, - label: Option<&'static str>, + descriptor: Option, ) -> Result, BitcoindError> { let req = if tip.height == 0 { self.make_request(&self.watchonly_client, "listsinceblock", ¶ms!())? @@ -567,7 +484,13 @@ impl BitcoinD { self.make_request( &self.watchonly_client, "listsinceblock", - ¶ms!(Json::String(tip.hash.to_string())), + ¶ms!( + Json::String(tip.hash.to_string()), + Json::Number(1.into()), + Json::Bool(true), + Json::Bool(true), + Json::Bool(true) + ), )? }; Ok(req @@ -578,20 +501,26 @@ impl BitcoinD { .iter() .filter_map(|t| { let t = ListSinceBlockTransaction::from(t); - if label.or_else(|| t.label.as_deref()) == t.label.as_deref() { - Some(t) - } else { - None + match descriptor { + None => Some(t), + Some(ref desc) => { + if t.wallet_descs.contains(desc) { + Some(t) + } else { + None + } + } } }) .collect()) } - pub fn list_deposits_since_block( + fn list_deposits_since_block( &self, tip: &BlockchainTip, + deposit_desc: String, ) -> Result, BitcoindError> { - self.list_since_block(tip, Some(DEPOSIT_UTXOS_LABEL)) + self.list_since_block(tip, Some(deposit_desc)) } /// Repeatedly called by our main loop to stay in sync with bitcoind. @@ -601,6 +530,7 @@ impl BitcoinD { deposits_utxos: &HashMap, db_tip: &BlockchainTip, min_conf: u32, + deposit_desc: String, ) -> Result { let (mut new_unconf, mut new_conf, mut new_spent) = (HashMap::new(), HashMap::new(), HashMap::new()); @@ -645,7 +575,7 @@ impl BitcoinD { } // Second, we scan for new ones. - let utxos = self.list_deposits_since_block(db_tip)?; + let utxos = self.list_deposits_since_block(db_tip, deposit_desc)?; for utxo in utxos { if utxo.is_receive && deposits_utxos.get(&utxo.outpoint).is_none() { new_unconf.insert( @@ -850,9 +780,6 @@ impl BitcoinD { continue; } - // TODO: i think we can also filter out the entries *with* a "revault-somthing" label, - // but we need to be sure. - let spending_txid = transaction .get("txid") .map(|t| t.as_str()) @@ -1169,7 +1096,7 @@ pub struct ListSinceBlockTransaction { pub outpoint: OutPoint, pub txo: TxOut, pub is_receive: bool, - pub label: Option, + pub wallet_descs: Vec, pub confirmations: i32, pub blockheight: Option, } @@ -1225,11 +1152,23 @@ impl From<&Json> for ListSinceBlockTransaction { .map(|a| a.as_u64()) .flatten() .map(|b| b as u32); - let label = j - .get("label") - .map(|l| l.as_str()) - .flatten() - .map(|l| l.to_string()); + // FIXME: allocs + let wallet_descs = j + .get("wallet_descs") + .map(|l| { + l.as_array() + .expect( + "API break, 'listsinceblock' entry didn't contain a valid 'wallet_descs'.", + ) + .iter() + .map(|desc| { + desc.as_str() + .expect("Invalid desc string in 'listsinceblock'.") + .to_string() + }) + .collect() + }) + .unwrap_or_else(|| Vec::new()); ListSinceBlockTransaction { outpoint: OutPoint { @@ -1237,7 +1176,7 @@ impl From<&Json> for ListSinceBlockTransaction { vout: vout as u32, // Bitcoin makes this safe }, is_receive, - label, + wallet_descs, txo: TxOut { value, script_pubkey, diff --git a/src/bitcoind/poller.rs b/src/bitcoind/poller.rs index 977f4378..aa9348ce 100644 --- a/src/bitcoind/poller.rs +++ b/src/bitcoind/poller.rs @@ -1286,7 +1286,6 @@ fn handle_spent_unvault( fn handle_new_deposit( revaultd: &mut Arc>, db_path: &Path, - bitcoind: &BitcoinD, deposits_cache: &mut HashMap, outpoint: OutPoint, utxo: UtxoInfo, @@ -1302,6 +1301,7 @@ fn handle_new_deposit( return Ok(()); } + // FIXME: get rid of this mapping, use bitcoind instead. let derivation_index = *revaultd .read() .unwrap() @@ -1333,8 +1333,6 @@ fn handle_new_deposit( ); deposits_cache.insert(outpoint, utxo); - // Mind the gap! https://www.youtube.com/watch?v=UOPyGKDQuRk - // FIXME: of course, that's rudimentary let current_first_index = revaultd.read().unwrap().current_unused_index; if derivation_index >= current_first_index { let new_index = revaultd @@ -1342,18 +1340,10 @@ fn handle_new_deposit( .unwrap() .current_unused_index .increment() - .map_err(|e| { - // FIXME: we should probably go back to 0 at this point. - BitcoindError::Custom(format!("Deriving next index: {}", e)) - })?; + // FIXME: we should probably go back to 0 at this point. + .expect("Incrementing derivation index"); db_update_deposit_index(&revaultd.read().unwrap().db_file(), new_index)?; revaultd.write().unwrap().current_unused_index = new_index; - let next_addr = bitcoind - .addr_descriptor(&revaultd.read().unwrap().last_deposit_address().to_string())?; - bitcoind.import_fresh_deposit_descriptor(next_addr)?; - let next_addr = bitcoind - .addr_descriptor(&revaultd.read().unwrap().last_unvault_address().to_string())?; - bitcoind.import_fresh_unvault_descriptor(next_addr)?; log::debug!( "Incremented deposit derivation index from {}", @@ -1555,10 +1545,11 @@ fn update_utxos( deposits_cache, previous_tip, revaultd.read().unwrap().min_conf, + revaultd.read().unwrap().deposit_descriptor.to_string(), )?; for (outpoint, utxo) in new_deposits { - handle_new_deposit(revaultd, &db_path, bitcoind, deposits_cache, outpoint, utxo)?; + handle_new_deposit(revaultd, &db_path, deposits_cache, outpoint, utxo)?; } for (outpoint, utxo) in conf_deposits { @@ -1711,6 +1702,11 @@ fn maybe_create_wallet(revaultd: &mut RevaultD, bitcoind: &BitcoinD) -> Result<( BitcoindError::Custom(format!("Computing time since epoch: {}", e.to_string())) })?; let fresh_wallet = (curr_timestamp - wallet.timestamp as u64) < 30; + let rescan_timestamp = if fresh_wallet { + None + } else { + Some(wallet.timestamp) + }; // TODO: sanity check descriptors are imported when migrating to 0.22 @@ -1727,34 +1723,13 @@ fn maybe_create_wallet(revaultd: &mut RevaultD, bitcoind: &BitcoinD) -> Result<( bitcoind.createwallet_startup(bitcoind_wallet_path, true)?; log::info!("Importing descriptors to bitcoind watchonly wallet."); - // Now, import descriptors. - // In theory, we could just import the vault (deposit) descriptor expressed using xpubs, give a - // range to bitcoind as the gap limit, and be fine. - // Unfortunately we cannot just import descriptors as is, since bitcoind does not support - // Miniscript ones yet. Worse, we actually need to derive them to pass them to bitcoind since - // the vault one (which we are interested about) won't be expressed with a `multi()` statement ( - // currently supported by bitcoind) if there are more than 15 stakeholders. - // Therefore, we derive [max index] `addr()` descriptors to import into bitcoind, and handle - // the derivation index mess ourselves :'( - let addresses: Vec<_> = revaultd - .all_deposit_addresses() - .into_iter() - .map(|a| bitcoind.addr_descriptor(&a)) - .collect::, _>>()?; - log::trace!("Importing deposit descriptors '{:?}'", &addresses); - bitcoind.startup_import_deposit_descriptors(addresses, wallet.timestamp, fresh_wallet)?; - - // As a consequence, we don't have enough information to opportunistically import a - // descriptor at the reception of a deposit anymore. Thus we need to blindly import *both* - // deposit and unvault descriptors.. - // FIXME: maybe we actually have, with the derivation_index_map ? - let addresses: Vec<_> = revaultd - .all_unvault_addresses() - .into_iter() - .map(|a| bitcoind.addr_descriptor(&a)) - .collect::, _>>()?; - log::trace!("Importing unvault descriptors '{:?}'", &addresses); - bitcoind.startup_import_unvault_descriptors(addresses, wallet.timestamp, fresh_wallet)?; + bitcoind.startup_import_descriptors( + [ + revaultd.deposit_descriptor.to_string(), + revaultd.unvault_descriptor.to_string(), + ], + rescan_timestamp, + )?; } if let Some(cpfp_key) = revaultd.cpfp_key { @@ -1794,7 +1769,7 @@ fn maybe_create_wallet(revaultd: &mut RevaultD, bitcoind: &BitcoinD) -> Result<( .inner() .to_string_with_secret(&keymap); - bitcoind.startup_import_cpfp_descriptor(cpfp_desc, wallet.timestamp, fresh_wallet)?; + bitcoind.import_cpfp_descriptor(cpfp_desc, rescan_timestamp)?; } } else { log::info!("Not creating the CPFP wallet, as we don't have a CPFP key"); From 44ef0a860ee8d8294e6c2a849cc8b098c7cf3060 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 15 Jul 2022 14:33:16 +0200 Subject: [PATCH 4/4] ci: use my custom bitcoind --- .cirrus.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 6a6f8f6d..27d0f817 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -68,9 +68,9 @@ task: # Download the bitcoind binary ARCHIVE_NAME="$BITCOIND_DIR_NAME.tar.gz" - curl https://bitcoincore.org/bin/bitcoin-core-$BITCOIND_VERSION/bitcoin-$BITCOIND_VERSION-x86_64-linux-gnu.tar.gz -o $ARCHIVE_NAME + curl http://download.darosior.ninja/dump/bitcoind-patched-for-revaultd.tar.gz -o $ARCHIVE_NAME tar -xzf $ARCHIVE_NAME - export BITCOIND_PATH=$BITCOIND_DIR_NAME/bin/bitcoind + export BITCOIND_PATH=bitcoind-patched-for-revaultd/bin/bitcoind # Setup the postgres instance for the servers pg_ctlcluster 13 main start