From 9955ef54a54cfb436ea772c18e0da0e39477a0a2 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 15 Jul 2024 18:35:49 -0400 Subject: [PATCH] Apply bitcoin fee per vsize, not per weight unit This enables more precision. --- coins/bitcoin/src/wallet/send.rs | 65 +++++++++++++++++-------------- coins/bitcoin/tests/wallet.rs | 2 +- processor/src/networks/bitcoin.rs | 7 +--- 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 1980a5548..9a723523a 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -61,11 +61,11 @@ pub struct SignableTransaction { } impl SignableTransaction { - fn calculate_weight( + fn calculate_weight_vbytes( inputs: usize, payments: &[(ScriptBuf, u64)], change: Option<&ScriptBuf>, - ) -> u64 { + ) -> (u64, u64) { // Expand this a full transaction in order to use the bitcoin library's weight function let mut tx = Transaction { version: Version(2), @@ -99,7 +99,33 @@ impl SignableTransaction { // the value is fixed size (so any value could be used here) tx.output.push(TxOut { value: Amount::ZERO, script_pubkey: change.clone() }); } - u64::from(tx.weight()) + + let weight = tx.weight(); + + // Now calculate the size in vbytes + + /* + "Virtual transaction size" is weight ceildiv 4 per + https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki + + https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04 + /src/policy/policy.cpp#L295-L298 + implements this almost as expected, with an additional consideration to signature operations + + Signature operations (the second argument of the following call) do not count Taproot + signatures per https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#cite_ref-11-0 + + We don't risk running afoul of the Taproot signature limit as it allows at least one per + input, which is all we use + */ + ( + weight.to_wu(), + u64::try_from(bitcoin::policy::get_virtual_tx_size( + i64::try_from(weight.to_wu()).unwrap(), + 0i64, + )) + .unwrap(), + ) } /// Returns the fee necessary for this transaction to achieve the fee rate specified at @@ -128,7 +154,7 @@ impl SignableTransaction { payments: &[(ScriptBuf, u64)], change: Option, data: Option>, - fee_per_weight: u64, + fee_per_vbyte: u64, ) -> Result { if inputs.is_empty() { Err(TransactionError::NoInputs)?; @@ -177,34 +203,14 @@ impl SignableTransaction { }) } - let mut weight = Self::calculate_weight(tx_ins.len(), payments, None); - let mut needed_fee = fee_per_weight * weight; - - // "Virtual transaction size" is weight ceildiv 4 per - // https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki + let (mut weight, vbytes) = Self::calculate_weight_vbytes(tx_ins.len(), payments, None); - // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/ - // src/policy/policy.cpp#L295-L298 - // implements this as expected - - // Technically, it takes whatever's greater, the weight or the amount of signature operations - // multiplied by DEFAULT_BYTES_PER_SIGOP (20) - // We only use 1 signature per input, and our inputs have a weight exceeding 20 - // Accordingly, our inputs' weight will always be greater than the cost of the signature ops - let vsize = weight.div_ceil(4); - debug_assert_eq!( - u64::try_from(bitcoin::policy::get_virtual_tx_size( - weight.try_into().unwrap(), - tx_ins.len().try_into().unwrap() - )) - .unwrap(), - vsize - ); + let mut needed_fee = fee_per_vbyte * vbytes; // Technically, if there isn't change, this TX may still pay enough of a fee to pass the // minimum fee. Such edge cases aren't worth programming when they go against intent, as the // specified fee rate is too low to be valid // bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE is in sats/kilo-vbyte - if needed_fee < ((u64::from(bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE) * vsize) / 1000) { + if needed_fee < ((u64::from(bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE) * vbytes) / 1000) { Err(TransactionError::TooLowFee)?; } @@ -214,8 +220,9 @@ impl SignableTransaction { // If there's a change address, check if there's change to give it if let Some(change) = change { - let weight_with_change = Self::calculate_weight(tx_ins.len(), payments, Some(&change)); - let fee_with_change = fee_per_weight * weight_with_change; + let (weight_with_change, vbytes_with_change) = + Self::calculate_weight_vbytes(tx_ins.len(), payments, Some(&change)); + let fee_with_change = fee_per_vbyte * vbytes_with_change; if let Some(value) = input_sat.checked_sub(payment_sat + fee_with_change) { if value >= DUST { tx_outs.push(TxOut { value: Amount::from_sat(value), script_pubkey: change }); diff --git a/coins/bitcoin/tests/wallet.rs b/coins/bitcoin/tests/wallet.rs index 9db004f57..4b77e61a4 100644 --- a/coins/bitcoin/tests/wallet.rs +++ b/coins/bitcoin/tests/wallet.rs @@ -303,7 +303,7 @@ async_sequential! { } // Make sure the change is correct - assert_eq!(needed_fee, u64::from(tx.weight()) * FEE); + assert_eq!(needed_fee, u64::try_from(tx.vsize()).unwrap() * FEE); let input_value = output.value() + offset_output.value(); let output_value = tx.output.iter().map(|output| output.value.to_sat()).sum::(); assert_eq!(input_value - output_value, needed_fee); diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index 43c266c4c..ae62f4c8f 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -405,7 +405,7 @@ impl Bitcoin { .to_sat(); } let out = tx.output.iter().map(|output| output.value.to_sat()).sum::(); - fees.push((in_value - out) / tx.weight().to_wu()); + fees.push((in_value - out) / u64::try_from(tx.vsize()).unwrap()); } } fees.sort(); @@ -413,11 +413,6 @@ impl Bitcoin { // The DUST constant documentation notes a relay rule practically enforcing a // 1000 sat/kilo-vbyte minimum fee. - // - // 1000 sat/kilo-vbyte is 1000 sat/4-kilo-weight (250 sat/kilo-weight). - // Since bitcoin-serai takes fee per weight, we'd need to pass 0.25 to achieve this fee rate. - // Accordingly, setting 1 is 4x the current relay rule minimum (and should be more than safe). - // TODO: Rewrite to fee_per_vbyte, not fee_per_weight? Ok(Fee(fee.max(1))) }