Fix: better fee calculation and check (#317)

* fix: better fee calculation and check

* chore: update changelog

* chore: fix typo in estimated fee

* refactor: improve simulated witness generation

CHANGELOG.md

@@ -13,12 +13,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- Fee strategy `range` support is now under the new crate feature `fee_range` and disable by default ([#314](https://github.com/farcaster-project/farcaster-core/pull/314))
-- Change Bitcoin fee unit from `sat/vB` to `sat/kvB` ([#315](https://github.com/farcaster-project/farcaster-core/pull/315))
+- Fee strategy `range` support is now under the new crate feature `fee_range` and disable by default by @h4sh3d ([#314](https://github.com/farcaster-project/farcaster-core/pull/314))
+- Change Bitcoin fee unit from `sat/vB` to `sat/kvB` by @h4sh3d ([#315](https://github.com/farcaster-project/farcaster-core/pull/315))
 
 ### Fixed
 
 - Check input lenght when parsing deals from strings by @h4sh3d ([#313](https://github.com/farcaster-project/farcaster-core/pull/313)])
+- Add estimated witness when computing transaction fee by @h4sh3d ([#317](https://github.com/farcaster-project/farcaster-core/pull/317))
 
 ## [0.6.1] - 2022-12-14
 

src/bitcoin/fee.rs

@@ -34,6 +34,7 @@
 //! ```
 
 use bitcoin::blockdata::transaction::TxOut;
+use bitcoin::blockdata::witness::Witness;
 use bitcoin::util::amount::Denomination;
 use bitcoin::util::psbt::PartiallySignedTransaction;
 use bitcoin::Amount;
@@ -158,6 +159,15 @@ fn get_available_input_sat(tx: &PartiallySignedTransaction) -> Result<Amount, Fe
     ))
 }
 
+fn upper_bound_simulated_witness() -> Witness {
+    // Simulate 2 signatures for Alice and Bob, and a script
+    //   buy/cancel script is 70 bytes
+    //   refund/punish script is 111 bytes plus a true/false opcode
+    //
+    // We simulate the biggest of the two scripts
+    Witness::from_vec(vec![vec![0; 71], vec![0; 71], vec![0], vec![0; 111]])
+}
+
 impl Fee for PartiallySignedTransaction {
     type FeeUnit = SatPerKvB;
 
@@ -178,17 +188,12 @@ impl Fee for PartiallySignedTransaction {
 
         let input_sum = get_available_input_sat(self)?;
 
-        // FIXME This does not account for witnesses
-        // currently the fees are wrong
-        // Get the transaction weight
-        //
-        // For transactions with an empty witness, this is simply the consensus-serialized size
-        // times four. For transactions with a witness, this is the non-witness
-        // consensus-serialized size multiplied by three plus the with-witness consensus-serialized
-        // size.
-        let weight = self.unsigned_tx.weight() as f64;
-
-        // Compute the fee amount to set in total
+        // simulate witness data
+        self.unsigned_tx.input[0].witness = upper_bound_simulated_witness();
+        let vsize = self.unsigned_tx.vsize() as f64;
+        // remove witness
+        self.unsigned_tx.input[0].witness = Witness::new();
+
         let fee_rate = match strategy {
             FeeStrategy::Fixed(sat_per_kvb) => sat_per_kvb
                 .as_native_unit()
@@ -199,7 +204,7 @@ impl Fee for PartiallySignedTransaction {
                 FeePriority::High => max_inc.as_native_unit().to_float_in(Denomination::Satoshi),
             },
         };
-        let fee_amount = fee_rate / 1000f64 * weight;
+        let fee_amount = fee_rate / 1000f64 * vsize;
         let fee_amount = Amount::from_sat(fee_amount.round() as u64);
 
         // Apply the fee on the first output
@@ -222,18 +227,26 @@ impl Fee for PartiallySignedTransaction {
 
         let input_sum = get_available_input_sat(self)?.as_sat();
         let output_sum = self.unsigned_tx.output[0].value;
-        let fee = input_sum
+        let effective_fee = input_sum
             .checked_sub(output_sum)
             .ok_or(FeeStrategyError::AmountOfFeeTooHigh)?;
-        let weight = self.unsigned_tx.weight() as u64;
 
-        let effective_sat_per_kvb = SatPerKvB::from_sat(
-            (weight * 1000)
-                .checked_div(fee)
-                .ok_or(FeeStrategyError::AmountOfFeeTooLow)?,
-        );
-
-        Ok(strategy.check(&effective_sat_per_kvb))
+        // simulate witness data
+        let mut tx = self.unsigned_tx.clone();
+        tx.input[0].witness = upper_bound_simulated_witness();
+        let vsize = tx.vsize() as f64;
+
+        match strategy {
+            FeeStrategy::Fixed(sat_per_kvb) => {
+                let fee_rate = sat_per_kvb
+                    .as_native_unit()
+                    .to_float_in(Denomination::Satoshi);
+                let fee_amount = fee_rate / 1000f64 * vsize;
+                Ok(effective_fee == fee_amount.round() as u64)
+            }
+            #[cfg(feature = "fee_range")]
+            FeeStrategy::Range { min_inc, max_inc } => todo!(),
+        }
     }
 }
 

tests/transactions.rs

@@ -83,7 +83,7 @@ macro_rules! setup_txs {
             failure: DoubleKeys::new(pubkey_a1, pubkey_b1),
         };
 
-        let fee = FeeStrategy::Fixed(SatPerKvB::from_sat(1000));
+        let fee = FeeStrategy::Fixed(SatPerKvB::from_sat(1500));
         let politic = FeePriority::Low;
 
         let mut lock = LockTx::initialize(&funding, datalock.clone(), target_amount).unwrap();
@@ -102,6 +102,7 @@ macro_rules! setup_txs {
 
         // Set the fees according to the given strategy
         cancel.as_partial_mut().set_fee(&fee, politic).unwrap();
+        assert!(cancel.as_partial_mut().validate_fee(&fee).unwrap());
 
         //
         // Create refund tx
@@ -111,6 +112,7 @@ macro_rules! setup_txs {
 
         // Set the fees according to the given strategy
         refund.as_partial_mut().set_fee(&fee, politic).unwrap();
+        assert!(refund.as_partial_mut().validate_fee(&fee).unwrap());
 
         lock.verify_template(datalock.clone()).unwrap();
         cancel
@@ -166,6 +168,7 @@ macro_rules! setup_txs {
 
         // Set the fees according to the given strategy
         buy.as_partial_mut().set_fee(&fee, politic).unwrap();
+        assert!(buy.as_partial_mut().validate_fee(&fee).unwrap());
 
         buy.verify_template(new_address.clone()).unwrap();
 
@@ -203,6 +206,7 @@ macro_rules! setup_txs {
 
         // Set the fees according to the given strategy
         punish.as_partial_mut().set_fee(&fee, politic).unwrap();
+        assert!(punish.as_partial_mut().validate_fee(&fee).unwrap());
 
         //
         // Sign punish