Merge rust-bitcoin/rust-bitcoin#2064: Feature: Psbt fee checks

dac627cc09 Feature: Psbt fee checks (junderw)

Pull request description:

  Closes #2061

  These new methods on Psbt will add checks for high fees by default. The threshold for "high fees" is currently set to 25000 sat/vbyte, which is about 20x higher than the highest next block fees seen on the "Mempool" website.

  The primary goal of this change is to prevent users of the library from accidentally sending absurd amounts of fees.

  (ie. Recently in September 2023 there was a transaction that sent an absurd amount of fees and made news in the Bitcoin world. Luckily the mining pool gave it back, but some might not be so lucky.)

  There are variants of the method that allow for users to set their own "absurd" threshold using a `FeeRate` value. And there is a method that performs no checks, and the method name is alarming enough to draw attention in a review, so at least developers will be aware of the concept.

ACKs for top commit:
  apoelstra:
    ACK dac627cc09
  tcharding:
    ACK dac627cc09

Tree-SHA512: ae0beafdb50339ba3efc44a48ba19c0aeeb0a2671eb43867c1e02b807677ce99fb6b4c47b74a9ed2999f827b3edc00a8871fa4730dd12a4cb265be99437c13db

bitcoin/examples/ecdsa-psbt.rs

@@ -82,7 +82,7 @@ fn main() -> Result<()> {
     let finalized = online.finalize_psbt(signed)?;
 
     // You can use `bt sendrawtransaction` to broadcast the extracted transaction.
-    let tx = finalized.extract_tx();
+    let tx = finalized.extract_tx_unchecked_fee_rate();
     tx.verify(|_| Some(previous_output())).expect("failed to verify transaction");
 
     let hex = encode::serialize_hex(&tx);

bitcoin/examples/taproot-psbt.rs

@@ -319,7 +319,7 @@ fn generate_bip86_key_spend_tx(
     });
 
     // EXTRACTOR
-    let tx = psbt.extract_tx();
+    let tx = psbt.extract_tx_unchecked_fee_rate();
     tx.verify(|_| {
         Some(TxOut {
             value: from_amount,
@@ -553,7 +553,7 @@ impl BenefactorWallet {
             });
 
             // EXTRACTOR
-            let tx = psbt.extract_tx();
+            let tx = psbt.extract_tx_unchecked_fee_rate();
             tx.verify(|_| {
                 Some(TxOut { value: input_value, script_pubkey: output_script_pubkey.clone() })
             })
@@ -695,7 +695,7 @@ impl BeneficiaryWallet {
         });
 
         // EXTRACTOR
-        let tx = psbt.extract_tx();
+        let tx = psbt.extract_tx_unchecked_fee_rate();
         tx.verify(|_| {
             Some(TxOut { value: input_value, script_pubkey: input_script_pubkey.clone() })
         })

bitcoin/src/blockdata/fee_rate.rs

@@ -119,7 +119,7 @@ impl FeeRate {
 impl fmt::Display for FeeRate {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         if f.alternate() {
-            write!(f, "{} sat/kwu", self.0)
+            write!(f, "{}.00 sat/vbyte", self.to_sat_per_vb_ceil())
         } else {
             fmt::Display::fmt(&self.0, f)
         }

bitcoin/src/psbt/macros.rs

@@ -9,6 +9,52 @@ macro_rules! hex_psbt {
     };
 }
 
+#[cfg(test)]
+macro_rules! psbt_with_values {
+    ($input:expr, $output:expr) => {
+        Psbt {
+            unsigned_tx: Transaction {
+                version: transaction::Version::TWO,
+                lock_time: absolute::LockTime::ZERO,
+                input: vec![TxIn {
+                    previous_output: OutPoint {
+                        txid: "f61b1742ca13176464adb3cb66050c00787bb3a4eead37e985f2df1e37718126"
+                            .parse()
+                            .unwrap(),
+                        vout: 0,
+                    },
+                    script_sig: ScriptBuf::new(),
+                    sequence: Sequence::ENABLE_LOCKTIME_NO_RBF,
+                    witness: Witness::default(),
+                }],
+                output: vec![TxOut {
+                    value: Amount::from_sat($output),
+                    script_pubkey: ScriptBuf::from_hex(
+                        "a9143545e6e33b832c47050f24d3eeb93c9c03948bc787",
+                    )
+                    .unwrap(),
+                }],
+            },
+            xpub: Default::default(),
+            version: 0,
+            proprietary: BTreeMap::new(),
+            unknown: BTreeMap::new(),
+
+            inputs: vec![Input {
+                witness_utxo: Some(TxOut {
+                    value: Amount::from_sat($input),
+                    script_pubkey: ScriptBuf::from_hex(
+                        "a914339725ba21efd62ac753a9bcd067d6c7a6a39d0587",
+                    )
+                    .unwrap(),
+                }),
+                ..Default::default()
+            }],
+            outputs: vec![],
+        }
+    };
+}
+
 macro_rules! combine {
     ($thing:ident, $slf:ident, $other:ident) => {
         if let (&None, Some($thing)) = (&$slf.$thing, $other.$thing) {

bitcoin/src/psbt/mod.rs

@@ -21,7 +21,7 @@ use crate::crypto::ecdsa;
 use crate::crypto::key::{PrivateKey, PublicKey};
 use crate::prelude::*;
 use crate::sighash::{self, EcdsaSighashType, SighashCache};
-use crate::Amount;
+use crate::{Amount, FeeRate};
 
 #[macro_use]
 mod macros;
@@ -121,8 +121,55 @@ impl Psbt {
         Ok(psbt)
     }
 
-    /// Extracts the `Transaction` from a PSBT by filling in the available signature information.
-    pub fn extract_tx(self) -> Transaction {
+    /// The default `max_fee_rate` value used for extracting transactions with [`extract_tx`]
+    ///
+    /// As of 2023, even the biggest overpayers during the highest fee markets only paid around
+    /// 1000 sats/vByte. 25k sats/vByte is obviously a mistake at this point.
+    ///
+    /// [`extract_tx`]: Psbt::extract_tx
+    pub const DEFAULT_MAX_FEE_RATE: FeeRate = FeeRate::from_sat_per_vb_unchecked(25_000);
+
+    /// An alias for [`extract_tx_fee_rate_limit`].
+    ///
+    /// [`extract_tx_fee_rate_limit`]: Psbt::extract_tx_fee_rate_limit
+    pub fn extract_tx(self) -> Result<Transaction, ExtractTxError> {
+        self.internal_extract_tx_with_fee_rate_limit(Self::DEFAULT_MAX_FEE_RATE)
+    }
+
+    /// Extracts the [`Transaction`] from a [`Psbt`] by filling in the available signature information.
+    ///
+    /// ## Errors
+    ///
+    /// [`ExtractTxError`] variants will contain either the [`Psbt`] itself or the [`Transaction`]
+    /// that was extracted. These can be extracted from the Errors in order to recover.
+    /// See the error documentation for info on the variants. In general, it covers large fees.
+    pub fn extract_tx_fee_rate_limit(self) -> Result<Transaction, ExtractTxError> {
+        self.internal_extract_tx_with_fee_rate_limit(Self::DEFAULT_MAX_FEE_RATE)
+    }
+
+    /// Extracts the [`Transaction`] from a [`Psbt`] by filling in the available signature information.
+    ///
+    /// ## Errors
+    ///
+    /// See [`extract_tx`].
+    ///
+    /// [`extract_tx`]: Psbt::extract_tx
+    pub fn extract_tx_with_fee_rate_limit(
+        self,
+        max_fee_rate: FeeRate,
+    ) -> Result<Transaction, ExtractTxError> {
+        self.internal_extract_tx_with_fee_rate_limit(max_fee_rate)
+    }
+
+    /// Perform [`extract_tx_fee_rate_limit`] without the fee rate check.
+    ///
+    /// This can result in a transaction with absurdly high fees. Use with caution.
+    ///
+    /// [`extract_tx_fee_rate_limit`]: Psbt::extract_tx_fee_rate_limit
+    pub fn extract_tx_unchecked_fee_rate(self) -> Transaction { self.internal_extract_tx() }
+
+    #[inline]
+    fn internal_extract_tx(self) -> Transaction {
         let mut tx: Transaction = self.unsigned_tx;
 
         for (vin, psbtin) in tx.input.iter_mut().zip(self.inputs.into_iter()) {
@@ -133,6 +180,38 @@ impl Psbt {
         tx
     }
 
+    #[inline]
+    fn internal_extract_tx_with_fee_rate_limit(
+        self,
+        max_fee_rate: FeeRate,
+    ) -> Result<Transaction, ExtractTxError> {
+        let fee = match self.fee() {
+            Ok(fee) => fee,
+            Err(Error::MissingUtxo) =>
+                return Err(ExtractTxError::MissingInputValue { tx: self.internal_extract_tx() }),
+            Err(Error::NegativeFee) => return Err(ExtractTxError::SendingTooMuch { psbt: self }),
+            Err(Error::FeeOverflow) =>
+                return Err(ExtractTxError::AbsurdFeeRate {
+                    fee_rate: FeeRate::MAX,
+                    tx: self.internal_extract_tx(),
+                }),
+            _ => unreachable!(),
+        };
+
+        // Note: Move prevents usage of &self from now on.
+        let tx = self.internal_extract_tx();
+
+        // Now that the extracted Transaction is made, decide how to return it.
+        let fee_rate =
+            FeeRate::from_sat_per_kwu(fee.to_sat().saturating_mul(1000) / tx.weight().to_wu());
+        // Prefer to return an AbsurdFeeRate error when both trigger.
+        if fee_rate > max_fee_rate {
+            return Err(ExtractTxError::AbsurdFeeRate { fee_rate, tx });
+        }
+
+        Ok(tx)
+    }
+
     /// Combines this [`Psbt`] with `other` PSBT as described by BIP 174.
     ///
     /// In accordance with BIP 174 this function is commutative i.e., `A.combine(B) == B.combine(A)`
@@ -771,6 +850,48 @@ impl From<IndexOutOfBoundsError> for SignError {
     fn from(e: IndexOutOfBoundsError) -> Self { SignError::IndexOutOfBounds(e) }
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+/// This error is returned when extracting a [`Transaction`] from a [`Psbt`].
+pub enum ExtractTxError {
+    /// The [`FeeRate`] is too high
+    AbsurdFeeRate {
+        /// The [`FeeRate`]
+        fee_rate: FeeRate,
+        /// The extracted [`Transaction`] (use this to ignore the error)
+        tx: Transaction,
+    },
+    /// One or more of the inputs lacks value information (witness_utxo or non_witness_utxo)
+    MissingInputValue {
+        /// The extracted [`Transaction`] (use this to ignore the error)
+        tx: Transaction,
+    },
+    /// Input value is less than Output Value, and the [`Transaction`] would be invalid.
+    SendingTooMuch {
+        /// The original [`Psbt`] is returned untouched.
+        psbt: Psbt,
+    },
+}
+
+impl fmt::Display for ExtractTxError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ExtractTxError::AbsurdFeeRate { fee_rate, .. } =>
+                write!(f, "An absurdly high fee rate of {}", fee_rate),
+            ExtractTxError::MissingInputValue { .. } => write!(
+                f,
+                "One of the inputs lacked value information (witness_utxo or non_witness_utxo)"
+            ),
+            ExtractTxError::SendingTooMuch { .. } => write!(
+                f,
+                "Transaction would be invalid due to output value being greater than input value."
+            ),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ExtractTxError {}
+
 /// Input index out of bounds (actual index, maximum index allowed).
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum IndexOutOfBoundsError {
@@ -924,6 +1045,51 @@ mod tests {
         assert!(!pk.compressed);
     }
 
+    #[test]
+    fn psbt_high_fee_checks() {
+        let psbt = psbt_with_values!(5_000_000_000_000, 1000);
+        assert_eq!(
+            psbt.clone().extract_tx().map_err(|e| match e {
+                ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
+                _ => panic!(""),
+            }),
+            Err(FeeRate::from_sat_per_kwu(15060240960843))
+        );
+        assert_eq!(
+            psbt.clone().extract_tx_fee_rate_limit().map_err(|e| match e {
+                ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
+                _ => panic!(""),
+            }),
+            Err(FeeRate::from_sat_per_kwu(15060240960843))
+        );
+        assert_eq!(
+            psbt.clone()
+                .extract_tx_with_fee_rate_limit(FeeRate::from_sat_per_kwu(15060240960842))
+                .map_err(|e| match e {
+                    ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
+                    _ => panic!(""),
+                }),
+            Err(FeeRate::from_sat_per_kwu(15060240960843))
+        );
+        assert!(psbt
+            .clone()
+            .extract_tx_with_fee_rate_limit(FeeRate::from_sat_per_kwu(15060240960843))
+            .is_ok());
+
+        // Testing that extract_tx will error at 25k sat/vbyte (6250000 sat/kwu)
+        assert_eq!(
+            psbt_with_values!(2076001, 1000).extract_tx().map_err(|e| match e {
+                ExtractTxError::AbsurdFeeRate { fee_rate, .. } => fee_rate,
+                _ => panic!(""),
+            }),
+            Err(FeeRate::from_sat_per_kwu(6250003)) // 6250000 is 25k sat/vbyte
+        );
+
+        // Lowering the input satoshis by 1 lowers the sat/kwu by 3
+        // Putting it exactly at 25k sat/vbyte
+        assert!(psbt_with_values!(2076000, 1000).extract_tx().is_ok());
+    }
+
     #[test]
     fn serialize_then_deserialize_output() {
         let secp = &Secp256k1::new();

bitcoin/tests/psbt.rs

@@ -380,7 +380,7 @@ fn finalize(psbt: Psbt) -> Psbt {
 fn extract_transaction(psbt: Psbt) -> Transaction {
     let expected_tx_hex = include_str!("data/extract_tx_hex");
 
-    let tx = psbt.extract_tx();
+    let tx = psbt.extract_tx_unchecked_fee_rate();
 
     let got = serialize_hex(&tx);
     assert_eq!(got, expected_tx_hex);