From 6e1c41238faaca0b6cf7e4131885ec2588d2137c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 16 Feb 2023 16:11:09 +0100 Subject: [PATCH] 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(..)