From 54410cd9c435e1e6228b985d78e55b357c2368b8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 16:05:47 +0100 Subject: [PATCH 01/11] database: allow to query coins by their spending status --- src/bitcoin/poller/looper.rs | 4 ++-- src/commands/mod.rs | 4 ++-- src/database/mod.rs | 13 ++++++++--- src/database/sqlite/mod.rs | 43 +++++++++++++++++++++++++++--------- src/testutils.rs | 17 +++++++++++--- 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index aa9c8648..807965b3 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -1,6 +1,6 @@ use crate::{ bitcoin::{BitcoinInterface, BlockChainTip, UTxO}, - database::{Coin, DatabaseConnection, DatabaseInterface}, + database::{Coin, CoinType, DatabaseConnection, DatabaseInterface}, descriptors, }; @@ -30,7 +30,7 @@ fn update_coins( descs: &[descriptors::InheritanceDescriptor], secp: &secp256k1::Secp256k1, ) -> UpdatedCoins { - let curr_coins = db_conn.coins(); + let curr_coins = db_conn.coins(CoinType::All); log::debug!("Current coins: {:?}", curr_coins); // Start by fetching newly received coins. diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 89247970..2c5f1dd2 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -6,7 +6,7 @@ mod utils; use crate::{ bitcoin::BitcoinInterface, - database::{Coin, DatabaseInterface}, + database::{Coin, CoinType, DatabaseInterface}, descriptors, DaemonControl, VERSION, }; @@ -243,7 +243,7 @@ impl DaemonControl { pub fn list_coins(&self) -> ListCoinsResult { let mut db_conn = self.db.connection(); let coins: Vec = db_conn - .coins() + .coins(CoinType::All) // Can't use into_values as of Rust 1.48 .into_iter() .map(|(_, coin)| { diff --git a/src/database/mod.rs b/src/database/mod.rs index 494c6cee..6662a365 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -81,7 +81,7 @@ pub trait DatabaseConnection { ) -> Option<(bip32::ChildNumber, bool)>; /// Get all our coins, past or present, spent or not. - fn coins(&mut self) -> HashMap; + fn coins(&mut self, coin_type: CoinType) -> HashMap; /// List coins that are being spent and whose spending transaction is still unconfirmed. fn list_spending_coins(&mut self) -> HashMap; @@ -178,8 +178,8 @@ impl DatabaseConnection for SqliteConn { self.complete_wallet_rescan() } - fn coins(&mut self) -> HashMap { - self.coins() + fn coins(&mut self, coin_type: CoinType) -> HashMap { + self.coins(coin_type) .into_iter() .map(|db_coin| (db_coin.outpoint, db_coin.into())) .collect() @@ -316,3 +316,10 @@ impl Coin { self.spend_txid.is_some() } } + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum CoinType { + All, + Unspent, + Spent, +} diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index d5233b69..9f8ca382 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -16,7 +16,7 @@ use crate::{ schema::{DbAddress, DbCoin, DbSpendTransaction, DbTip, DbWallet}, utils::{create_fresh_db, db_exec, db_query, db_tx_query, LOOK_AHEAD_LIMIT}, }, - Coin, + Coin, CoinType, }, descriptors::MultipathDescriptor, }; @@ -313,10 +313,14 @@ impl SqliteConn { } /// Get all the coins from DB. - pub fn coins(&mut self) -> Vec { + pub fn coins(&mut self, coin_type: CoinType) -> Vec { db_query( &mut self.conn, - "SELECT * FROM coins", + match coin_type { + CoinType::All => "SELECT * FROM coins", + CoinType::Unspent => "SELECT * FROM coins WHERE spend_txid IS NULL", + CoinType::Spent => "SELECT * FROM coins WHERE spend_txid IS NOT NULL", + }, rusqlite::params![], |row| row.try_into(), ) @@ -682,7 +686,7 @@ mod tests { let mut conn = db.connection().unwrap(); // Necessarily empty at first. - assert!(conn.coins().is_empty()); + assert!(conn.coins(CoinType::All).is_empty()); // Add one, we'll get it. let coin_a = Coin { @@ -699,13 +703,17 @@ mod tests { spend_block: None, }; conn.new_unspent_coins(&[coin_a]); - assert_eq!(conn.coins()[0].outpoint, coin_a.outpoint); + assert_eq!(conn.coins(CoinType::All)[0].outpoint, coin_a.outpoint); // We can query it by its outpoint let coins = conn.db_coins(&[coin_a.outpoint]); assert_eq!(coins.len(), 1); assert_eq!(coins[0].outpoint, coin_a.outpoint); + // It is unspent. + assert_eq!(conn.coins(CoinType::Unspent)[0].outpoint, coin_a.outpoint); + assert!(conn.coins(CoinType::Spent).is_empty()); + // Add a second one (this one is change), we'll get both. let coin_b = Coin { outpoint: bitcoin::OutPoint::from_str( @@ -721,8 +729,11 @@ mod tests { spend_block: None, }; conn.new_unspent_coins(&[coin_b]); - let outpoints: HashSet = - conn.coins().into_iter().map(|c| c.outpoint).collect(); + let outpoints: HashSet = conn + .coins(CoinType::All) + .into_iter() + .map(|c| c.outpoint) + .collect(); assert!(outpoints.contains(&coin_a.outpoint)); assert!(outpoints.contains(&coin_b.outpoint)); @@ -738,11 +749,15 @@ mod tests { assert!(coins.iter().any(|c| c.outpoint == coin_a.outpoint)); assert!(coins.iter().any(|c| c.outpoint == coin_b.outpoint)); + // They are both unspent + assert_eq!(conn.coins(CoinType::Unspent).len(), 2); + assert!(conn.coins(CoinType::Spent).is_empty()); + // Now if we confirm one, it'll be marked as such. let height = 174500; let time = 174500; conn.confirm_coins(&[(coin_a.outpoint, height, time)]); - let coins = conn.coins(); + let coins = conn.coins(CoinType::All); assert_eq!(coins[0].block_height, Some(height)); assert_eq!(coins[0].block_time, Some(time)); assert!(coins[1].block_height.is_none()); @@ -753,14 +768,18 @@ mod tests { coin_a.outpoint, bitcoin::Txid::from_slice(&[0; 32][..]).unwrap(), )]); - let coins_map: HashMap = - conn.coins().into_iter().map(|c| (c.outpoint, c)).collect(); + let coins_map: HashMap = conn + .coins(CoinType::All) + .into_iter() + .map(|c| (c.outpoint, c)) + .collect(); assert!(coins_map .get(&coin_a.outpoint) .unwrap() .spend_txid .is_some()); + // We will see it as 'spending' let outpoints: HashSet = conn .list_spending_coins() .into_iter() @@ -768,6 +787,10 @@ mod tests { .collect(); assert!(outpoints.contains(&coin_a.outpoint)); + // The first one is spent, not the second one. + assert_eq!(conn.coins(CoinType::Spent)[0].outpoint, coin_a.outpoint); + assert_eq!(conn.coins(CoinType::Unspent)[0].outpoint, coin_b.outpoint); + // Now if we confirm the spend. let height = 128_097; let time = 3_000_000; diff --git a/src/testutils.rs b/src/testutils.rs index c42a743a..e50eafef 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -1,7 +1,7 @@ use crate::{ bitcoin::{BitcoinInterface, Block, BlockChainTip, UTxO}, config::{BitcoinConfig, Config}, - database::{Coin, DatabaseConnection, DatabaseInterface, SpendBlock}, + database::{Coin, CoinType, DatabaseConnection, DatabaseInterface, SpendBlock}, descriptors, DaemonHandle, }; @@ -189,8 +189,19 @@ impl DatabaseConnection for DummyDatabase { self.db.write().unwrap().change_index = index; } - fn coins(&mut self) -> HashMap { - self.db.read().unwrap().coins.clone() + fn coins(&mut self, coin_type: CoinType) -> HashMap { + let coins = self.db.read().unwrap().coins.clone(); + match coin_type { + CoinType::All => coins, + CoinType::Unspent => coins + .into_iter() + .filter(|(_, c)| c.spend_txid.is_none()) + .collect(), + CoinType::Spent => coins + .into_iter() + .filter(|(_, c)| c.spend_txid.is_some()) + .collect(), + } } fn list_spending_coins(&mut self) -> HashMap { From 9f23161a53ad32d03d4ec464c48a819862dbf3e5 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 16:57:39 +0100 Subject: [PATCH 02/11] commands: correct the max feerate value We should have a functional test for this... --- src/commands/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 2c5f1dd2..890da887 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -39,7 +39,7 @@ const DUST_OUTPUT_SATS: u64 = 5_000; const MAX_FEE: u64 = bitcoin::blockdata::constants::COIN_VALUE; // Assume that paying more than 1000sat/vb in feerate is a bug. -const MAX_FEERATE: u64 = bitcoin::blockdata::constants::COIN_VALUE; +const MAX_FEERATE: u64 = 1_000; // Timestamp in the header of the genesis block. Used for sanity checks. const MAINNET_GENESIS_TIME: u32 = 1231006505; From f2312593da8b27c6c6a69c7b0463959369c5acb6 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 16:58:14 +0100 Subject: [PATCH 03/11] commands: check for dust outputs in the PSBT sanity checks --- src/commands/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 890da887..05183ea0 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -162,6 +162,13 @@ fn sanity_check_psbt(psbt: &Psbt) -> Result<(), CommandError> { return Err(CommandError::SanityCheckFailure(psbt.clone())); } + // Check for dust outputs + for txo in psbt.unsigned_tx.output.iter() { + if txo.value < txo.script_pubkey.dust_value().to_sat() { + return Err(CommandError::SanityCheckFailure(psbt.clone())); + } + } + Ok(()) } From 3d5d0134b4b5bc06910a89d23def36de5b548c30 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 17:07:17 +0100 Subject: [PATCH 04/11] commands: fix the capacity of input vectors in create_spend They were based on the expected size of the outputs vector... --- src/commands/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 05183ea0..52f45c02 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -301,8 +301,8 @@ impl DaemonControl { // fees, and add necessary information to the PSBT inputs. let mut in_value = bitcoin::Amount::from_sat(0); let mut sat_vb = 0; - let mut txins = Vec::with_capacity(destinations.len()); - let mut psbt_ins = Vec::with_capacity(destinations.len()); + let mut txins = Vec::with_capacity(coins_outpoints.len()); + let mut psbt_ins = Vec::with_capacity(coins_outpoints.len()); let mut spent_txs = HashMap::with_capacity(coins_outpoints.len()); let coins = db_conn.coins_by_outpoints(coins_outpoints); for op in coins_outpoints { From c09ae3f87bc8bb2391d88d1a718b35d4a6c568fa Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 17:09:27 +0100 Subject: [PATCH 05/11] commands, jsonrpc: add a new 'createrecovery' command This is a new command dedicated to the recovery usecase. For now it's dead simple: sweep all coins that are available through the recovery path to a given address. --- doc/API.md | 25 +++++++++ src/commands/mod.rs | 124 +++++++++++++++++++++++++++++++++++++++++++- src/jsonrpc/api.rs | 26 +++++++++- src/jsonrpc/mod.rs | 3 +- 4 files changed, 174 insertions(+), 4 deletions(-) diff --git a/doc/API.md b/doc/API.md index 71793a82..d976e673 100644 --- a/doc/API.md +++ b/doc/API.md @@ -11,12 +11,15 @@ Commands must be sent as valid JSONRPC 2.0 requests, ending with a `\n`. | [`getinfo`](#getinfo) | Get general information about the daemon | | [`getnewaddress`](#getnewaddress) | Get a new receiving address | | [`listcoins`](#listcoins) | List all wallet transaction outputs. | +| [`createspend`](#createspend) | Create a new Spend transaction | +| [`updatespend`](#updatespend) | Store a created Spend transaction | | [`listspendtxs`](#listspendtxs) | List all stored Spend transactions | | [`delspendtx`](#delspendtx) | Delete a stored Spend transaction | | [`broadcastspend`](#broadcastspend) | Finalize a stored Spend PSBT, and broadcast it | | [`startrescan`](#startrescan) | Start rescanning the block chain from a given date | | [`listconfirmed`](#listconfirmed) | List of confirmed transactions of incoming and outgoing funds | | [`listtransactions`](#listtransactions) | List of transactions with the given txids | +| [`createrecovery`](#createrecovery) | Create a recovery transaction to sweep expired coins | # Reference @@ -261,3 +264,25 @@ Confirmation time is based on the timestamp of blocks. | Field | Type | Description | | -------------- | ------ | ------------------------------------------------------ | | `transactions` | array | Array of [Transaction resource](#transaction-resource) | + + +### `createrecovery` + +Create a transaction that sweeps all coins whose timelocked recovery path is available to a provided +address at a provided feerate. + +Will error if no such coins are available or the sum of their value is not enough to cover the +requested feerate. + +#### Request + +| Field | Type | Description | +| ---------- | ----------------- | ----------------------------------------------------------------- | +| `address` | str | The Bitcoin address to sweep the coins to. | +| `feerate` | integer | Target feerate for the transaction, in satoshis per virtual byte. | + +#### Response + +| Field | Type | Description | +| -------------- | --------- | ---------------------------------------------------- | +| `psbt` | string | PSBT of the recovery transaction, encoded as base64. | diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 52f45c02..f28f64a2 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -15,7 +15,7 @@ use utils::{ }; use std::{ - collections::{BTreeMap, HashMap}, + collections::{hash_map, BTreeMap, HashMap}, convert::TryInto, fmt, }; @@ -67,6 +67,7 @@ pub enum CommandError { InsaneRescanTimestamp(u32), /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), + RecoveryNotAvailable, } impl fmt::Display for CommandError { @@ -102,6 +103,10 @@ impl fmt::Display for CommandError { ), Self::InsaneRescanTimestamp(t) => write!(f, "Insane timestamp '{}'.", t), Self::RescanTrigger(s) => write!(f, "Error while starting rescan: '{}'", s), + Self::RecoveryNotAvailable => write!( + f, + "No coin currently available through the timelocked recovery path." + ), } } } @@ -628,6 +633,117 @@ impl DaemonControl { .collect(); ListTransactionsResult { transactions } } + + /// Create a transaction that sweeps all coins whose timelocked recovery path is currently + /// available to a provided address with the provided feerate. + /// + /// Note that not all coins may be spendable through the recovery path at the same time. + pub fn create_recovery( + &self, + address: bitcoin::Address, + feerate_vb: u64, + ) -> Result { + if feerate_vb < 1 { + return Err(CommandError::InvalidFeerate(feerate_vb)); + } + let mut db_conn = self.db.connection(); + + // The transaction template. We'll fill-in the inputs afterward. + let mut psbt = Psbt { + unsigned_tx: bitcoin::Transaction { + version: 2, + lock_time: bitcoin::PackedLockTime(0), // TODO: anti-fee sniping + input: Vec::new(), + output: vec![bitcoin::TxOut { + script_pubkey: address.script_pubkey(), + value: 0xFF_FF_FF_FF, + }], + }, + version: 0, + xpub: BTreeMap::new(), + proprietary: BTreeMap::new(), + unknown: BTreeMap::new(), + inputs: Vec::new(), + outputs: vec![PsbtOut::default()], + }; + + // Query the coins that we can spend through the recovery path from the database. + let current_height = self.bitcoin.chain_tip().height; + let desc_timelock = self.config.main_descriptor.timelock_value(); + let timelock: i32 = desc_timelock + .try_into() + .expect("Must fit, it's effectively a u16"); + let sweepable_coins = db_conn + .coins(CoinType::Unspent) + .into_iter() + .filter(|(_, c)| { + // We are interested in coins available at the *next* block + c.block_height + .map(|h| current_height + 1 >= h + timelock) + .unwrap_or(false) + }); + + // Fill-in the transaction inputs and PSBT inputs information. Record the value + // that is fed to the transaction while doing so, to compute the fees afterward. + let csv_value: u16 = desc_timelock + .try_into() + .expect("Must fit, it's effectively a u16"); + let mut in_value = bitcoin::Amount::from_sat(0); + let mut sat_vb = 0; + let mut spent_txs = HashMap::new(); + for (_, coin) in sweepable_coins { + in_value += coin.amount; + psbt.unsigned_tx.input.push(bitcoin::TxIn { + previous_output: coin.outpoint, + sequence: bitcoin::Sequence::from_height(csv_value), + // TODO: once we move to Taproot, anti-fee-sniping using nSequence + ..bitcoin::TxIn::default() + }); + + // Fetch the transaction that created this coin if necessary + if let hash_map::Entry::Vacant(e) = spent_txs.entry(coin.outpoint) { + let tx = self + .bitcoin + .wallet_transaction(&coin.outpoint.txid) + .ok_or(CommandError::FetchingTransaction(coin.outpoint))?; + e.insert(tx.0); + } + + 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 { + value: coin.amount.to_sat(), + script_pubkey: coin_desc.script_pubkey(), + }); + let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); + let bip32_derivation = coin_desc.bip32_derivations(); + psbt.inputs.push(PsbtIn { + witness_script, + witness_utxo, + non_witness_utxo, + bip32_derivation, + ..PsbtIn::default() + }); + } + + // The sweepable_coins iterator may have been empty. + if psbt.unsigned_tx.input.is_empty() { + return Err(CommandError::RecoveryNotAvailable); + } + + // Compute the value of the single output based on the requested feerate. + let tx_vbytes = psbt.unsigned_tx.vsize() as u64 + sat_vb; + let absolute_fee = bitcoin::Amount::from_sat(tx_vbytes.checked_mul(feerate_vb).unwrap()); + let output_value = in_value.checked_sub(absolute_fee).ok_or({ + CommandError::InsufficientFunds(in_value, bitcoin::Amount::from_sat(0), feerate_vb) + })?; + psbt.unsigned_tx.output[0].value = output_value.to_sat(); + + sanity_check_psbt(&psbt)?; + + Ok(CreateRecoveryResult { psbt }) + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -708,6 +824,12 @@ pub struct TransactionInfo { pub time: Option, } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct CreateRecoveryResult { + #[serde(serialize_with = "ser_base64", deserialize_with = "deser_base64")] + pub psbt: Psbt, +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index 82886372..63ca6493 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -39,8 +39,7 @@ fn create_spend(control: &DaemonControl, params: Params) -> Result Result Result { + let address = params + .get(0, "address") + .ok_or_else(|| Error::invalid_params("Missing 'address' parameter."))? + .as_str() + .and_then(|s| bitcoin::Address::from_str(s).ok()) + .ok_or_else(|| Error::invalid_params("Invalid 'address' parameter."))?; + let feerate: u64 = params + .get(1, "feerate") + .ok_or_else(|| Error::invalid_params("Missing 'feerate' parameter."))? + .as_u64() + .ok_or_else(|| Error::invalid_params("Invalid 'feerate' parameter."))?; + + let res = control.create_recovery(address, feerate)?; + Ok(serde_json::json!(&res)) +} + /// Handle an incoming JSONRPC2 request. pub fn handle_request(control: &DaemonControl, req: Request) -> Result { let result = match req.method.as_str() { @@ -148,6 +164,12 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result { + let params = req.params.ok_or_else(|| { + Error::invalid_params("Missing 'address' and 'feerate' parameters.") + })?; + create_recovery(control, params)? + } "createspend" => { let params = req.params.ok_or_else(|| { Error::invalid_params( diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 24ca83be..dddb35c7 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -161,7 +161,8 @@ impl From for Error { | commands::CommandError::UnknownSpend(..) | commands::CommandError::SpendFinalization(..) | commands::CommandError::InsaneRescanTimestamp(..) - | commands::CommandError::AlreadyRescanning => { + | commands::CommandError::AlreadyRescanning + | commands::CommandError::RecoveryNotAvailable => { Error::new(ErrorCode::InvalidParams, e.to_string()) } commands::CommandError::FetchingTransaction(..) From a78f46fa2fc5337c51d1eaa6ac6bbee04cfdf437 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 17:17:02 +0100 Subject: [PATCH 06/11] [refactoring] jsonrpc: sort command names alphabetically --- src/jsonrpc/api.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index 63ca6493..4b142bbf 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -187,7 +187,23 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result serde_json::json!(&control.get_info()), "getnewaddress" => serde_json::json!(&control.get_new_address()), "listcoins" => serde_json::json!(&control.list_coins()), + "listconfirmed" => { + let params = req.params.ok_or_else(|| { + Error::invalid_params( + "The 'listconfirmed' command requires 3 parameters: 'start', 'end' and 'limit'", + ) + })?; + list_confirmed(control, params)? + } "listspendtxs" => serde_json::json!(&control.list_spend()), + "listtransactions" => { + let params = req.params.ok_or_else(|| { + Error::invalid_params( + "The 'listtransactions' command requires 1 parameter: 'txids'", + ) + })?; + list_transactions(control, params)? + } "startrescan" => { let params = req .params @@ -201,22 +217,6 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result { - let params = req.params.ok_or_else(|| { - Error::invalid_params( - "The 'listconfirmed' command requires 3 parameters: 'start', 'end' and 'limit'", - ) - })?; - list_confirmed(control, params)? - } - "listtransactions" => { - let params = req.params.ok_or_else(|| { - Error::invalid_params( - "The 'listtransactions' command requires 1 parameter: 'txids'", - ) - })?; - list_transactions(control, params)? - } _ => { return Err(Error::method_not_found()); } From a69fb625ea3b8dfbfa1bf384e001f167fd04ff86 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 9 Dec 2022 11:45:29 +0100 Subject: [PATCH 07/11] qa: use a CSV of 10 for recovery --- tests/fixtures.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index 2d46cb72..0fce44b2 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -120,7 +120,10 @@ def lianad(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}/<0;1>/*),and_v(v:pkh(tpubD9vQiBdDxYzU4cVFtApWj4devZrvcfWaPXX1zHdDc7GPfUsDKqGnbhraccfm7BAXgRgUbVQUV2v2o4NitjGEk7hpbuP85kvBrD4ahFDtNBJ/<0;1>/*),older(65000))))") + csv_value = 10 + main_desc = Descriptor.from_str( + f"wsh(or_d(pk({owner_xpub}/<0;1>/*),and_v(v:pkh(tpubD9vQiBdDxYzU4cVFtApWj4devZrvcfWaPXX1zHdDc7GPfUsDKqGnbhraccfm7BAXgRgUbVQUV2v2o4NitjGEk7hpbuP85kvBrD4ahFDtNBJ/<0;1>/*),older({csv_value}))))" + ) lianad = Lianad( datadir, From d362885b85107b616447899d5eaaa5ba1b55c170 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 9 Dec 2022 12:16:46 +0100 Subject: [PATCH 08/11] qa: record the recovery xpub too --- tests/fixtures.py | 6 ++++-- tests/test_framework/lianad.py | 20 ++++++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index 0fce44b2..4a6b6ebb 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -119,15 +119,17 @@ def lianad(bitcoind, directory): bitcoind_cookie = os.path.join(bitcoind.bitcoin_dir, "regtest", ".cookie") owner_hd = BIP32.from_seed(os.urandom(32), network="test") - owner_xpub = owner_hd.get_xpub() + recovery_hd = BIP32.from_seed(os.urandom(32), network="test") + owner_xpub, recovery_xpub = owner_hd.get_xpub(), recovery_hd.get_xpub() csv_value = 10 main_desc = Descriptor.from_str( - f"wsh(or_d(pk({owner_xpub}/<0;1>/*),and_v(v:pkh(tpubD9vQiBdDxYzU4cVFtApWj4devZrvcfWaPXX1zHdDc7GPfUsDKqGnbhraccfm7BAXgRgUbVQUV2v2o4NitjGEk7hpbuP85kvBrD4ahFDtNBJ/<0;1>/*),older({csv_value}))))" + f"wsh(or_d(pk({owner_xpub}/<0;1>/*),and_v(v:pkh({recovery_xpub}/<0;1>/*),older({csv_value}))))" ) lianad = Lianad( datadir, owner_hd, + recovery_hd, main_desc, bitcoind.rpcport, bitcoind_cookie, diff --git a/tests/test_framework/lianad.py b/tests/test_framework/lianad.py index c321a8f0..51a0007b 100644 --- a/tests/test_framework/lianad.py +++ b/tests/test_framework/lianad.py @@ -30,6 +30,7 @@ class Lianad(TailableProc): self, datadir, owner_hd, + recovery_hd, multi_desc, bitcoind_rpc_port, bitcoind_cookie_path, @@ -40,6 +41,7 @@ class Lianad(TailableProc): self.prefix = os.path.split(datadir)[-1] self.owner_hd = owner_hd + self.recovery_hd = recovery_hd self.multi_desc = multi_desc self.receive_desc, self.change_desc = multi_desc.singlepath_descriptors() @@ -63,15 +65,20 @@ class Lianad(TailableProc): f.write(f"cookie_path = '{bitcoind_cookie_path}'\n") f.write(f"addr = '127.0.0.1:{bitcoind_rpc_port}'\n") - def sign_psbt(self, psbt): - """Sign a transaction using the owner's key. - This will fill the 'partial_sigs' field of all inputs. + def sign_psbt(self, psbt, recovery=False): + """Sign a transaction. + + This will fill the 'partial_sigs' field of all inputs. Uses either the 'primary' + 'recovery' key as specified. :param psbt: PSBT of the transaction to be signed. :returns: PSBT with a signature in each input for the owner's key. """ assert isinstance(psbt, PSBT) + # Which key to sign the transaction with. + hd = self.recovery_hd if recovery else self.owner_hd + # Sign each input. for i, psbt_in in enumerate(psbt.i): # First, gather the needed information from the PSBT input. @@ -84,12 +91,9 @@ class Lianad(TailableProc): ] 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) + # Now sign the transaction. sighash = sighash_all_witness(script_code, psbt, i) - privkey = coincurve.PrivateKey( - self.owner_hd.get_privkey_from_path(der_path) - ) + privkey = coincurve.PrivateKey(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, From 3b5cbd5122fa53d0247e82dd7fc703b5d0a4bae3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 9 Dec 2022 12:17:16 +0100 Subject: [PATCH 09/11] qa: introduce a sign_and_broadcast utility --- tests/test_framework/utils.py | 8 ++++++++ tests/test_rpc.py | 14 +++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/test_framework/utils.py b/tests/test_framework/utils.py index c0b959c7..e506e676 100644 --- a/tests/test_framework/utils.py +++ b/tests/test_framework/utils.py @@ -75,6 +75,14 @@ def spend_coins(lianad, bitcoind, coins): return tx +def sign_and_broadcast(lianad, bitcoind, psbt, recovery=False): + """Sign a PSBT, finalize it, extract the transaction and broadcast it.""" + signed_psbt = lianad.sign_psbt(psbt, recovery) + finalized_psbt = lianad.finalize_psbt(signed_psbt) + tx = finalized_psbt.tx.serialize_with_witness().hex() + return bitcoind.rpc.sendrawtransaction(tx) + + class RpcError(ValueError): def __init__(self, method: str, params: dict, error: str): super(ValueError, self).__init__( diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 3ba4e6f5..fb16defe 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -8,7 +8,14 @@ from test_framework.serializations import ( PSBT_IN_PARTIAL_SIG, PSBT_IN_NON_WITNESS_UTXO, ) -from test_framework.utils import wait_for, COIN, RpcError, get_txid, spend_coins +from test_framework.utils import ( + wait_for, + COIN, + RpcError, + get_txid, + spend_coins, + sign_and_broadcast, +) def test_getinfo(lianad): @@ -123,10 +130,7 @@ def test_create_spend(lianad, bitcoind): ) # We can sign it and broadcast it. - signed_psbt = lianad.sign_psbt(PSBT.from_base64(res["psbt"])) - finalized_psbt = lianad.finalize_psbt(signed_psbt) - tx = finalized_psbt.tx.serialize_with_witness().hex() - bitcoind.rpc.sendrawtransaction(tx) + sign_and_broadcast(PSBT.from_base64(res["psbt"])) def test_list_spend(lianad, bitcoind): From 46a94d6c8eb144484a4b0df2ddbd48b32d4d096f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 9 Dec 2022 12:17:38 +0100 Subject: [PATCH 10/11] qa: test recovery 'sweep' transaction creation --- tests/test_rpc.py | 50 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index fb16defe..add3f1cd 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -130,7 +130,7 @@ def test_create_spend(lianad, bitcoind): ) # We can sign it and broadcast it. - sign_and_broadcast(PSBT.from_base64(res["psbt"])) + sign_and_broadcast(lianad, bitcoind, PSBT.from_base64(res["psbt"])) def test_list_spend(lianad, bitcoind): @@ -537,3 +537,51 @@ def test_listtransactions(lianad, bitcoind): assert len(txs) == 3 bit_txids = set(bitcoind.rpc.decoderawtransaction(tx["tx"])["txid"] for tx in txs) assert bit_txids == txids + + +def test_create_recovery(lianad, bitcoind): + """Test the sweep of coins that are available through the timelocked path.""" + # Start by getting a few coins + destinations = { + lianad.rpc.getnewaddress()["address"]: 0.1, + lianad.rpc.getnewaddress()["address"]: 0.2, + lianad.rpc.getnewaddress()["address"]: 0.3, + } + txid = bitcoind.rpc.sendmany("", destinations) + bitcoind.generate_block(1, wait_for_mempool=txid) + wait_for( + lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() + ) + + # There's nothing to sweep + with pytest.raises( + RpcError, + match="No coin currently available through the timelocked recovery path", + ): + lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2) + + # Receive another coin, it will be one block after the others + txid = bitcoind.rpc.sendtoaddress(lianad.rpc.getnewaddress()["address"], 0.4) + + # Make the timelock of the 3 first coins mature (we use a csv of 10 in the fixture) + bitcoind.generate_block(9, wait_for_mempool=txid) + + # Now we can create a recovery tx that sweeps the first 3 coins. + res = lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 18) + reco_psbt = PSBT.from_base64(res["psbt"]) + assert len(reco_psbt.tx.vin) == 3, "The last coin's timelock hasn't matured yet" + assert len(reco_psbt.tx.vout) == 1 + assert int(0.5999 * COIN) < int(reco_psbt.tx.vout[0].nValue) < int(0.6 * COIN) + txid = sign_and_broadcast(lianad, bitcoind, reco_psbt, recovery=True) + + # And by mining one more block we'll be able to sweep the last coin. + bitcoind.generate_block(1, wait_for_mempool=txid) + wait_for( + lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() + ) + res = lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 1) + reco_psbt = PSBT.from_base64(res["psbt"]) + assert len(reco_psbt.tx.vin) == 1 + assert len(reco_psbt.tx.vout) == 1 + assert int(0.39999 * COIN) < int(reco_psbt.tx.vout[0].nValue) < int(0.4 * COIN) + sign_and_broadcast(lianad, bitcoind, reco_psbt, recovery=True) From ba994ff8ff01d04fd31ed77cf7f66e0862e68e17 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 13 Dec 2022 15:20:17 +0100 Subject: [PATCH 11/11] commands: do not underestimate the size of created transactions We were truncating when computing the virtual size of satisfactions. --- src/commands/mod.rs | 43 ++++++++++++++----------------------------- src/descriptors.rs | 30 ++++++++++++------------------ 2 files changed, 26 insertions(+), 47 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f28f64a2..06903489 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -29,8 +29,6 @@ use miniscript::{ }; use serde::{Deserialize, Serialize}; -const WITNESS_FACTOR: usize = 4; - // We would never create a transaction with an output worth less than this. // That's 1$ at 20_000$ per BTC. const DUST_OUTPUT_SATS: u64 = 5_000; @@ -159,7 +157,7 @@ fn sanity_check_psbt(psbt: &Psbt) -> Result<(), CommandError> { } // Check the feerate isn't insane. - let tx_vb: u64 = tx_vbytes(tx); + let tx_vb = tx.vsize() as u64; let feerate_sats_vb = abs_fee .checked_div(tx_vb) .ok_or_else(|| CommandError::SanityCheckFailure(psbt.clone()))?; @@ -177,24 +175,6 @@ fn sanity_check_psbt(psbt: &Psbt) -> Result<(), CommandError> { Ok(()) } -// Get the maximum satisfaction size in vbytes for this descriptor -fn desc_sat_vb(desc: &descriptors::DerivedInheritanceDescriptor) -> u64 { - desc.max_sat_weight() - .checked_div(WITNESS_FACTOR) - .unwrap() - .try_into() - .unwrap() -} - -// Get the virtual size of this transaction -fn tx_vbytes(tx: &bitcoin::Transaction) -> u64 { - tx.weight() - .checked_div(WITNESS_FACTOR) - .unwrap() - .try_into() - .unwrap() -} - // Get the size of a type that can be serialized (txos, transactions, ..) fn serializable_size(t: &T) -> u64 { bitcoin::consensus::serialize(t).len().try_into().unwrap() @@ -305,6 +285,7 @@ impl DaemonControl { // While doing so, we record the total input value of the transaction to later compute // fees, and add necessary information to the PSBT inputs. let mut in_value = bitcoin::Amount::from_sat(0); + let txin_sat_vb = self.config.main_descriptor.max_sat_vbytes(); let mut sat_vb = 0; let mut txins = Vec::with_capacity(coins_outpoints.len()); let mut psbt_ins = Vec::with_capacity(coins_outpoints.len()); @@ -335,7 +316,7 @@ impl DaemonControl { // Populate the PSBT input with the information needed by signers. let coin_desc = self.derived_desc(coin); - sat_vb += desc_sat_vb(&coin_desc); + sat_vb += txin_sat_vb; let witness_script = Some(coin_desc.witness_script()); let witness_utxo = Some(bitcoin::TxOut { value: coin.amount.to_sat(), @@ -393,14 +374,17 @@ impl DaemonControl { input: txins, output: txouts, }; - let nochange_vb = tx_vbytes(&tx) + sat_vb; + let nochange_vb = (tx.vsize() + sat_vb) as u64; let absolute_fee = in_value .checked_sub(out_value) .ok_or(CommandError::InsufficientFunds( in_value, out_value, feerate_vb, ))?; - let nochange_feerate_vb = absolute_fee.to_sat().checked_div(nochange_vb).unwrap(); + let nochange_feerate_vb = absolute_fee + .to_sat() + .checked_div(nochange_vb as u64) + .unwrap(); if nochange_feerate_vb.checked_mul(10).unwrap() < feerate_vb.checked_mul(9).unwrap() { return Err(CommandError::InsufficientFunds( in_value, out_value, feerate_vb, @@ -689,6 +673,7 @@ impl DaemonControl { .try_into() .expect("Must fit, it's effectively a u16"); let mut in_value = bitcoin::Amount::from_sat(0); + let txin_sat_vb = self.config.main_descriptor.max_sat_vbytes(); let mut sat_vb = 0; let mut spent_txs = HashMap::new(); for (_, coin) in sweepable_coins { @@ -710,7 +695,7 @@ impl DaemonControl { } let coin_desc = self.derived_desc(&coin); - sat_vb += desc_sat_vb(&coin_desc); + sat_vb += txin_sat_vb; let witness_script = Some(coin_desc.witness_script()); let witness_utxo = Some(bitcoin::TxOut { value: coin.amount.to_sat(), @@ -733,7 +718,7 @@ impl DaemonControl { } // Compute the value of the single output based on the requested feerate. - let tx_vbytes = psbt.unsigned_tx.vsize() as u64 + sat_vb; + let tx_vbytes = (psbt.unsigned_tx.vsize() + sat_vb) as u64; let absolute_fee = bitcoin::Amount::from_sat(tx_vbytes.checked_mul(feerate_vb).unwrap()); let output_value = in_value.checked_sub(absolute_fee).ok_or({ CommandError::InsufficientFunds(in_value, bitcoin::Amount::from_sat(0), feerate_vb) @@ -942,12 +927,12 @@ mod tests { assert_eq!(tx.output[0].script_pubkey, dummy_addr.script_pubkey()); assert_eq!(tx.output[0].value, dummy_value); - // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 170 sats fees. + // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 171 sats fees. // At 2sats/vb, it's twice that. - assert_eq!(tx.output[1].value, 89_830); + assert_eq!(tx.output[1].value, 89_829); let res = control.create_spend(&destinations, &[dummy_op], 2).unwrap(); let tx = res.psbt.unsigned_tx; - assert_eq!(tx.output[1].value, 89_660); + assert_eq!(tx.output[1].value, 89_658); // If we ask for a too high feerate, or a too large/too small output, it'll fail. assert_eq!( diff --git a/src/descriptors.rs b/src/descriptors.rs index 132e2f64..47357cc6 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -480,6 +480,17 @@ impl MultipathDescriptor { .expect("Cannot fail for P2WSH") } + /// Get the maximum size in vbytes (rounded up) of a satisfaction for this descriptor. + pub fn max_sat_vbytes(&self) -> usize { + self.multi_desc + .max_satisfaction_weight() + .expect("Cannot fail for P2WSH") + .checked_add(WITNESS_FACTOR - 1) + .unwrap() + .checked_div(WITNESS_FACTOR) + .unwrap() + } + /// Get the maximum size in virtual bytes of the whole input in a transaction spending /// a coin with this Script. pub fn spender_input_size(&self) -> usize { @@ -581,13 +592,6 @@ impl DerivedInheritanceDescriptor { .map(|k| (k.key.inner, (k.origin.0, k.origin.1))) .collect() } - - /// Get the maximum size in WU of a satisfaction for this descriptor. - pub fn max_sat_weight(&self) -> usize { - self.0 - .max_satisfaction_weight() - .expect("Cannot fail for P2WSH") - } } #[cfg(test)] @@ -657,7 +661,6 @@ mod tests { der_desc.script_pubkey(); der_desc.witness_script(); assert!(!der_desc.bip32_derivations().is_empty()); - assert!(!der_desc.max_sat_weight() > 0); } #[test] @@ -674,17 +677,8 @@ mod tests { #[test] fn inheritance_descriptor_sat_size() { - let secp = secp256k1::Secp256k1::verification_only(); let desc = MultipathDescriptor::from_str("wsh(or_d(pk([92162c45]tpubD6NzVbkrYhZ4WzTf9SsD6h7AH7oQEippXK2KP8qvhMMqFoNeN5YFVi7vRyeRSDGtgd2bPyMxUNmHui8t5yCgszxPPxMafu1VVzDpg9aruYW/<0;1>/*),and_v(v:pkh(tpubD6NzVbkrYhZ4Wdgu2yfdmrce5g4fiH1ZLmKhewsnNKupbi4sxjH1ZVAorkBLWSkhsjhg8kiq8C4BrBjMy3SjAKDyDdbuvUa1ToAHbiR98js/<0;1>/*),older(2))))#uact7s3g").unwrap(); - let receive_desc = desc.receive_descriptor(); - let change_desc = desc.change_descriptor(); - - // Derived or not the expected maximum satisfaction size should be the same for - // the change and receive descriptor. - assert_eq!( - receive_desc.derive(999.into(), &secp).max_sat_weight(), - change_desc.derive(999.into(), &secp).max_sat_weight() - ); + assert_eq!(desc.max_sat_vbytes(), (1 + 69 + 1 + 34 + 73 + 3) / 4); // See the stack details below. // Maximum input size is (txid + vout + scriptsig + nSequence + max_sat). // Where max_sat is: