From c711c5b696b7c8353da29b3c451bf2ca00e8daa8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 16 Feb 2023 16:09:54 +0100 Subject: [PATCH 1/3] commands: don't overestimate the feerate in tx creation sanity checks It was accounting for the transaction size without witnesses! --- src/commands/mod.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 496fd872..4fabc183 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -129,7 +129,10 @@ fn check_output_value(value: bitcoin::Amount) -> Result<(), CommandError> { // Apply some sanity checks on a created transaction's PSBT. // TODO: add more sanity checks from revault_tx -fn sanity_check_psbt(psbt: &Psbt) -> Result<(), CommandError> { +fn sanity_check_psbt( + spent_desc: &descriptors::MultipathDescriptor, + psbt: &Psbt, +) -> Result<(), CommandError> { let tx = &psbt.unsigned_tx; // Must have as many in/out in the PSBT and Bitcoin tx. @@ -161,7 +164,7 @@ fn sanity_check_psbt(psbt: &Psbt) -> Result<(), CommandError> { } // Check the feerate isn't insane. - let tx_vb = tx.vsize() as u64; + let tx_vb = (tx.vsize() + spent_desc.max_sat_vbytes() * tx.input.len()) as u64; let feerate_sats_vb = abs_fee .checked_div(tx_vb) .ok_or_else(|| CommandError::SanityCheckFailure(psbt.clone()))?; @@ -466,7 +469,7 @@ impl DaemonControl { inputs: psbt_ins, outputs: psbt_outs, }; - sanity_check_psbt(&psbt)?; + sanity_check_psbt(&self.config.main_descriptor, &psbt)?; // TODO: maybe check for common standardness rules (max size, ..)? Ok(CreateSpendResult { psbt }) @@ -742,7 +745,7 @@ impl DaemonControl { })?; psbt.unsigned_tx.output[0].value = output_value.to_sat(); - sanity_check_psbt(&psbt)?; + sanity_check_psbt(&self.config.main_descriptor, &psbt)?; Ok(CreateRecoveryResult { psbt }) } @@ -950,6 +953,10 @@ mod tests { let tx = res.psbt.unsigned_tx; assert_eq!(tx.output[1].value, 89_658); + // A feerate of 555 won't trigger the sanity checks (they were previously not taking the + // satisfaction size into account and overestimating the feerate). + control.create_spend(&destinations, &[dummy_op], 555).unwrap(); + // If we ask for a too high feerate, or a too large/too small output, it'll fail. assert_eq!( control.create_spend(&destinations, &[dummy_op], 10_000), From 6e1c41238faaca0b6cf7e4131885ec2588d2137c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 16 Feb 2023 16:11:09 +0100 Subject: [PATCH 2/3] commands: have a detailed error for the insane fee sanity check --- src/commands/mod.rs | 61 +++++++++++++++++++++++++++++++++++++++++---- src/jsonrpc/mod.rs | 1 + 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 4fabc183..080aaccc 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -54,6 +54,7 @@ pub enum CommandError { /* out value */ bitcoin::Amount, /* target feerate */ u64, ), + InsaneFees(InsaneFeeInfo), FetchingTransaction(bitcoin::OutPoint), SanityCheckFailure(Psbt), UnknownSpend(bitcoin::Txid), @@ -86,6 +87,19 @@ impl fmt::Display for CommandError { "Cannot create a {} sat/vb transaction with input value {} and output value {}", feerate, in_val, out_val ), + Self::InsaneFees(info) => write!( + f, + "We assume transactions with a fee larger than {} sats or a feerate larger than {} sats/vb are a mistake. \ + The created transaction {}.", + MAX_FEE, + MAX_FEERATE, + match info { + InsaneFeeInfo::NegativeFee => "would have a negative fee".to_string(), + InsaneFeeInfo::TooHighFee(f) => format!("{} sats in fees", f), + InsaneFeeInfo::InvalidFeerate => "would have an invalid feerate".to_string(), + InsaneFeeInfo::TooHighFeerate(r) => format!("has a feerate of {} sats/vb", r), + }, + ), Self::FetchingTransaction(op) => { write!(f, "Could not fetch transaction for coin {}", op) } @@ -127,6 +141,14 @@ fn check_output_value(value: bitcoin::Amount) -> Result<(), CommandError> { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InsaneFeeInfo { + NegativeFee, + InvalidFeerate, + TooHighFee(u64), + TooHighFeerate(u64), +} + // Apply some sanity checks on a created transaction's PSBT. // TODO: add more sanity checks from revault_tx fn sanity_check_psbt( @@ -158,18 +180,20 @@ fn sanity_check_psbt( let value_out: u64 = tx.output.iter().map(|o| o.value).sum(); let abs_fee = value_in .checked_sub(value_out) - .ok_or_else(|| CommandError::SanityCheckFailure(psbt.clone()))?; + .ok_or_else(|| CommandError::InsaneFees(InsaneFeeInfo::NegativeFee))?; if abs_fee > MAX_FEE { - return Err(CommandError::SanityCheckFailure(psbt.clone())); + return Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFee(abs_fee))); } // Check the feerate isn't insane. let tx_vb = (tx.vsize() + spent_desc.max_sat_vbytes() * tx.input.len()) as u64; let feerate_sats_vb = abs_fee .checked_div(tx_vb) - .ok_or_else(|| CommandError::SanityCheckFailure(psbt.clone()))?; + .ok_or_else(|| CommandError::InsaneFees(InsaneFeeInfo::InvalidFeerate))?; if !(1..=MAX_FEERATE).contains(&feerate_sats_vb) { - return Err(CommandError::SanityCheckFailure(psbt.clone())); + return Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate( + feerate_sats_vb, + ))); } // Check for dust outputs @@ -442,6 +466,9 @@ impl DaemonControl { let with_change_feerate_vb = absolute_fee.to_sat().checked_div(with_change_vb).unwrap(); if with_change_feerate_vb > feerate_vb { + // TODO: try first with the exact feerate, then try again with 90% of the feerate + // if it fails. Otherwise with small transactions and large feerates it's possible + // the feerate increase from the target be dramatically higher. let target_fee = with_change_vb.checked_mul(feerate_vb).unwrap(); let change_amount = absolute_fee .checked_sub(bitcoin::Amount::from_sat(target_fee)) @@ -955,7 +982,9 @@ mod tests { // A feerate of 555 won't trigger the sanity checks (they were previously not taking the // satisfaction size into account and overestimating the feerate). - control.create_spend(&destinations, &[dummy_op], 555).unwrap(); + control + .create_spend(&destinations, &[dummy_op], 555) + .unwrap(); // If we ask for a too high feerate, or a too large/too small output, it'll fail. assert_eq!( @@ -1026,6 +1055,28 @@ mod tests { Err(CommandError::AlreadySpent(dummy_op)) ); + // We'd bail out if they tried to create a transaction with a too high feerate. + let dummy_op_dup = bitcoin::OutPoint { + txid: dummy_op.txid, + vout: dummy_op.vout + 10, + }; + db_conn.new_unspent_coins(&[Coin { + outpoint: dummy_op_dup, + block_height: None, + block_time: None, + amount: bitcoin::Amount::from_sat(400_000), + derivation_index: bip32::ChildNumber::from(42), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + assert_eq!( + control.create_spend(&destinations, &[dummy_op_dup], 1_001), + Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate( + 1001 + ))) + ); + ms.shutdown(); } diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 85dfd0b2..8c0f0df3 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -159,6 +159,7 @@ impl From for Error { | commands::CommandError::AddressNetwork(..) | commands::CommandError::InvalidOutputValue(..) | commands::CommandError::InsufficientFunds(..) + | commands::CommandError::InsaneFees(..) | commands::CommandError::UnknownSpend(..) | commands::CommandError::SpendFinalization(..) | commands::CommandError::InsaneRescanTimestamp(..) From 518eb91064ac4120792b71cec8f52eb26e359b89 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 16 Feb 2023 16:12:28 +0100 Subject: [PATCH 3/3] commands: print the PSBT in base64 on sanity checks failure To make it readable.. --- src/commands/mod.rs | 9 ++++++--- src/commands/utils.rs | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 080aaccc..07d5ed04 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -10,7 +10,10 @@ use crate::{ descriptors, DaemonControl, VERSION, }; -use utils::{deser_amount_from_sats, deser_base64, deser_hex, ser_amount, ser_base64, ser_hex}; +use utils::{ + deser_amount_from_sats, deser_base64, deser_hex, ser_amount, ser_base64, ser_hex, + to_base64_string, +}; use std::{ collections::{hash_map, BTreeMap, HashMap}, @@ -105,8 +108,8 @@ impl fmt::Display for CommandError { } Self::SanityCheckFailure(psbt) => write!( f, - "BUG! Please report this. Failed sanity checks for PSBT '{:?}'.", - psbt + "BUG! Please report this. Failed sanity checks for PSBT '{}'.", + to_base64_string(psbt) ), Self::UnknownSpend(txid) => write!(f, "Unknown spend transaction '{}'.", txid), Self::SpendFinalization(e) => { diff --git a/src/commands/utils.rs b/src/commands/utils.rs index 6c1ae85a..85ac95ea 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -15,12 +15,16 @@ where Ok(bitcoin::Amount::from_sat(a)) } +pub fn to_base64_string(t: T) -> String { + base64::encode(consensus::serialize(&t)) +} + pub fn ser_base64(t: T, s: S) -> Result where S: Serializer, T: consensus::Encodable, { - s.serialize_str(&base64::encode(consensus::serialize(&t))) + s.serialize_str(&to_base64_string(t)) } pub fn deser_base64<'de, D, T>(d: D) -> Result