commands: have a detailed error for the insane fee sanity check

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();
     }
 

src/jsonrpc/mod.rs

@@ -159,6 +159,7 @@ impl From<commands::CommandError> for Error {
             | commands::CommandError::AddressNetwork(..)
             | commands::CommandError::InvalidOutputValue(..)
             | commands::CommandError::InsufficientFunds(..)
+            | commands::CommandError::InsaneFees(..)
             | commands::CommandError::UnknownSpend(..)
             | commands::CommandError::SpendFinalization(..)
             | commands::CommandError::InsaneRescanTimestamp(..)