From 42fde6d4575b4aea121286f884f712b1c1cf64be Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Wed, 27 Jul 2022 11:03:43 +0200 Subject: [PATCH 1/9] test: Check fee_amount in assert_fee_rate --- src/wallet/mod.rs | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 4ee3835d..ee49dfc9 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -1988,8 +1988,9 @@ pub(crate) mod test { } macro_rules! assert_fee_rate { - ($tx:expr, $fees:expr, $fee_rate:expr $( ,@dust_change $( $dust_change:expr )* )* $( ,@add_signature $( $add_signature:expr )* )* ) => ({ - let mut tx = $tx.clone(); + ($psbt:expr, $fees:expr, $fee_rate:expr $( ,@dust_change $( $dust_change:expr )* )* $( ,@add_signature $( $add_signature:expr )* )* ) => ({ + let psbt = $psbt.clone(); + let mut tx = $psbt.clone().extract_tx(); $( $( $add_signature )* for txin in &mut tx.input { @@ -2005,6 +2006,18 @@ pub(crate) mod test { dust_change = true; )* + let fee_amount = psbt + .inputs + .iter() + .fold(0, |acc, i| acc + i.witness_utxo.as_ref().unwrap().value) + - psbt + .unsigned_tx + .output + .iter() + .fold(0, |acc, o| acc + o.value); + + assert_eq!(fee_amount, $fees); + let tx_fee_rate = FeeRate::from_wu($fees, tx.weight()); let fee_rate = $fee_rate; @@ -2384,7 +2397,7 @@ pub(crate) mod test { builder.add_recipient(addr.script_pubkey(), 25_000); let (psbt, details) = builder.finish().unwrap(); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::default(), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::default(), @add_signature); } #[test] @@ -2397,7 +2410,7 @@ pub(crate) mod test { .fee_rate(FeeRate::from_sat_per_vb(5.0)); let (psbt, details) = builder.finish().unwrap(); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(5.0), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(5.0), @add_signature); } #[test] @@ -3254,7 +3267,7 @@ pub(crate) mod test { details.received ); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature); } #[test] @@ -3364,7 +3377,7 @@ pub(crate) mod test { assert_eq!(tx.output.len(), 1); assert_eq!(tx.output[0].value + details.fee.unwrap_or(0), details.sent); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature); } #[test] @@ -3575,7 +3588,7 @@ pub(crate) mod test { details.received ); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(50.0), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(50.0), @add_signature); } #[test] @@ -3715,7 +3728,7 @@ pub(crate) mod test { 75_000 - original_send_all_amount - details.fee.unwrap_or(0) ); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(50.0), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(50.0), @add_signature); } #[test] @@ -3778,7 +3791,7 @@ pub(crate) mod test { 45_000 ); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(140.0), @dust_change, @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(140.0), @dust_change, @add_signature); } #[test] @@ -3849,7 +3862,7 @@ pub(crate) mod test { details.received ); - assert_fee_rate!(psbt.extract_tx(), details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(5.0), @add_signature); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), FeeRate::from_sat_per_vb(5.0), @add_signature); } #[test] From 00d426b88546a346820c102386cd1bfff82ca8f6 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 2 Aug 2022 11:25:09 +0200 Subject: [PATCH 2/9] test: Check that the feerate is never below... ...the requested one in assert_fee_rate --- src/wallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index ee49dfc9..ddc6d7d1 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -2022,7 +2022,7 @@ pub(crate) mod test { let fee_rate = $fee_rate; if !dust_change { - assert!((tx_fee_rate - fee_rate).as_sat_vb().abs() < 0.5, "Expected fee rate of {:?}, the tx has {:?}", fee_rate, tx_fee_rate); + assert!(tx_fee_rate >= fee_rate && (tx_fee_rate - fee_rate).as_sat_vb().abs() < 0.5, "Expected fee rate of {:?}, the tx has {:?}", fee_rate, tx_fee_rate); } else { assert!(tx_fee_rate >= fee_rate, "Expected fee rate of at least {:?}, the tx has {:?}", fee_rate, tx_fee_rate); } From ac051d7ae9512883e11a89ab002ad2d2c3c55c19 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Mon, 11 Jul 2022 21:52:11 +0200 Subject: [PATCH 3/9] Calculate fee amount after output addition We would previously calculate the fee amount in two steps: 1. Add the weight of the empty transaction 2. Add the weight of each output That's unnecessary: you can just use the weight of the transaction *after* the output addition. This is clearer, but also avoids a rare bug: if there are many outputs, adding them would cause the "number of outputs" transaction parameter lenght to increase, and we wouldn't notice it. This might still happen when adding the drain output - this commit also adds a comment as a reminder. --- src/wallet/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index ddc6d7d1..dccc427d 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -736,8 +736,6 @@ where let mut outgoing: u64 = 0; let mut received: u64 = 0; - fee_amount += fee_rate.fee_wu(tx.weight()); - let recipients = params.recipients.iter().map(|(r, v)| (r, *v)); for (index, (script_pubkey, value)) in recipients.enumerate() { @@ -753,13 +751,14 @@ where script_pubkey: script_pubkey.clone(), value, }; - fee_amount += fee_rate.fee_vb(serialize(&new_out).len()); tx.output.push(new_out); outgoing += value; } + fee_amount += fee_rate.fee_wu(tx.weight()); + if params.change_policy != tx_builder::ChangeSpendPolicy::ChangeAllowed && self.change_descriptor.is_none() { @@ -851,6 +850,9 @@ where script_pubkey: drain_script, }; + // TODO: We should pay attention when adding a new output: this might increase + // the lenght of the "number of vouts" parameter by 2 bytes, potentially making + // our feerate too low tx.output.push(drain_output); } }; From 7ac87b8f99fc0897753ce57d48ea24adf495a633 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 2 Aug 2022 12:06:54 +0200 Subject: [PATCH 4/9] TXIN_BASE_WEIGHT shouldn't include the script len We would before calculate the TXIN_BASE_WEIGHT as prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_sig_len (1 bytes), but that's wrong: the script_sig_len shouldn't be included, as miniscript already includes it in the `max_satisfaction_size` calculation. Fixes #160 --- src/wallet/coin_selection.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs index ca0fc47d..55e64305 100644 --- a/src/wallet/coin_selection.rs +++ b/src/wallet/coin_selection.rs @@ -30,7 +30,7 @@ //! # use bdk::database::Database; //! # use bdk::*; //! # use bdk::wallet::coin_selection::decide_change; -//! # const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4 + 1) * 4; +//! # const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4) * 4; //! #[derive(Debug)] //! struct AlwaysSpendEverything; //! @@ -119,8 +119,8 @@ pub type DefaultCoinSelectionAlgorithm = BranchAndBoundCoinSelection; pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection; // make the tests more predictable // Base weight of a Txin, not counting the weight needed for satisfying it. -// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes) -pub(crate) const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4 + 1) * 4; +// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) +pub(crate) const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4) * 4; #[derive(Debug)] /// Remaining amount after performing coin selection @@ -731,7 +731,7 @@ mod test { use rand::seq::SliceRandom; use rand::{Rng, SeedableRng}; - const P2WPKH_WITNESS_SIZE: usize = 73 + 33 + 2; + const P2WPKH_SATISFACTION_SIZE: usize = 73 + 33 + 2 + 1; const FEE_AMOUNT: u64 = 50; @@ -743,7 +743,7 @@ mod test { )) .unwrap(); WeightedUtxo { - satisfaction_weight: P2WPKH_WITNESS_SIZE, + satisfaction_weight: P2WPKH_SATISFACTION_SIZE, utxo: Utxo::Local(LocalUtxo { outpoint, txout: TxOut { @@ -823,7 +823,7 @@ mod test { let mut res = Vec::new(); for _ in 0..utxos_number { res.push(WeightedUtxo { - satisfaction_weight: P2WPKH_WITNESS_SIZE, + satisfaction_weight: P2WPKH_SATISFACTION_SIZE, utxo: Utxo::Local(LocalUtxo { outpoint: OutPoint::from_str( "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:0", @@ -843,7 +843,7 @@ mod test { fn generate_same_value_utxos(utxos_value: u64, utxos_number: usize) -> Vec { let utxo = WeightedUtxo { - satisfaction_weight: P2WPKH_WITNESS_SIZE, + satisfaction_weight: P2WPKH_SATISFACTION_SIZE, utxo: Utxo::Local(LocalUtxo { outpoint: OutPoint::from_str( "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:0", @@ -1313,7 +1313,7 @@ mod test { assert_eq!(result.selected.len(), 1); assert_eq!(result.selected_amount(), 100_000); - let input_size = (TXIN_BASE_WEIGHT + P2WPKH_WITNESS_SIZE).vbytes(); + let input_size = (TXIN_BASE_WEIGHT + P2WPKH_SATISFACTION_SIZE).vbytes(); let epsilon = 0.5; assert!((1.0 - (result.fee_amount as f32 / input_size as f32)).abs() < epsilon); } From ae919061e2b341ae74c90f0133ba392e835cb4e1 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 12 Jul 2022 15:51:27 +0200 Subject: [PATCH 5/9] Take into account the segwit tx header when... ...selecting coins We take into account the larger segwit tx header for every transaction, not just the segwit ones. The reason for this is that we prefer to overestimate the fees for the transaction than underestimating them - the former might create txs with a slightly higher feerate than the requested one, while the latter might create txs with a slightly lower one - or worse, invalid (<1 sat/vbyte)! --- src/wallet/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index dccc427d..a56fdadf 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -759,6 +759,17 @@ where fee_amount += fee_rate.fee_wu(tx.weight()); + // Segwit transactions' header is 2WU larger than legacy txs' header, + // as they contain a witness marker (1WU) and a witness flag (1WU) (see BIP144). + // At this point we really don't know if the resulting transaction will be segwit + // or legacy, so we just add this 2WU to the fee_amount - overshooting the fee amount + // is better than undershooting it. + // If we pass a fee_amount that is slightly higher than the final fee_amount, we + // end up with a transaction with a slightly higher fee rate than the requested one. + // If, instead, we undershoot, we may end up with a feerate lower than the requested one + // - we might come up with non broadcastable txs! + fee_amount += fee_rate.fee_wu(2); + if params.change_policy != tx_builder::ChangeSpendPolicy::ChangeAllowed && self.change_descriptor.is_none() { From 50af51da5a5c906d8bf660d35a4f922ceb996068 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 2 Aug 2022 11:24:23 +0200 Subject: [PATCH 6/9] test: Fix P2WPKH_FAKE_WITNESS_SIZE We would previously push 108 bytes on a P2WPKH witness to simulate signature + pubkey. This was wrong: we should push 106 bytes instead. The max satisfaction size for a P2WPKH is 112 WU: elements in witness (1 byte, 1WU) + OP_PUSH (1 byte, 1WU) + pk (33 bytes, 33 WU) + OP_PUSH (1 byte, 1WU) + signature and sighash (72 bytes, 72 WU) + scriptsig len (1 byte, 4WU) We should push on the witness pk + signature and sighash. This is 105 WU. Since we push just once instead of twice, we add 1WU for the OP_PUSH we are omitting. --- src/wallet/mod.rs | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index a56fdadf..8084756b 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -1871,6 +1871,14 @@ pub(crate) mod test { use crate::signer::{SignOptions, SignerError}; use crate::wallet::AddressIndex::{LastUnused, New, Peek, Reset}; + // The satisfaction size of a P2WPKH is 112 WU = + // 1 (elements in witness) + 1 (OP_PUSH) + 33 (pk) + 1 (OP_PUSH) + 72 (signature + sighash) + 1*4 (script len) + // On the witness itself, we have to push once for the pk (33WU) and once for signature + sighash (72WU), for + // a total of 105 WU. + // Here, we push just once for simplicity, so we have to add an extra byte for the missing + // OP_PUSH. + const P2WPKH_FAKE_WITNESS_SIZE: usize = 106; + #[test] fn test_cache_addresses_fixed() { let db = MemoryDatabase::new(); @@ -2007,7 +2015,7 @@ pub(crate) mod test { $( $( $add_signature )* for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature } )* @@ -3236,7 +3244,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3296,7 +3304,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3362,7 +3370,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3406,7 +3414,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3462,7 +3470,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3519,7 +3527,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3560,7 +3568,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3623,7 +3631,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3694,7 +3702,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3765,7 +3773,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3826,7 +3834,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3897,7 +3905,7 @@ pub(crate) mod test { let txid = tx.txid(); // skip saving the new utxos, we know they can't be used anyways for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -3976,7 +3984,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() @@ -4020,7 +4028,7 @@ pub(crate) mod test { let mut tx = psbt.extract_tx(); let txid = tx.txid(); for txin in &mut tx.input { - txin.witness.push([0x00; 108]); // fake signature + txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature wallet .database .borrow_mut() From 2756411ef7cf0415baf2f2401e2d5a78481d0aa1 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Wed, 13 Jul 2022 18:08:41 +0200 Subject: [PATCH 7/9] test: Reproduce #660 conditions Issue #660 has been fixed by 32ae95f463f62c42c6d6aec62c1832a30298fce4, when we moved the change calculation inside the coin selection. This commit just adds a test to make sure that the problem is fixed. --- src/wallet/mod.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 8084756b..6aa8789a 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -4050,6 +4050,38 @@ pub(crate) mod test { builder.finish().unwrap(); } + #[test] + fn test_fee_amount_negative_drain_val() { + // While building the transaction, bdk would calculate the drain_value + // as + // current_delta - fee_amount - drain_fee + // using saturating_sub, meaning that if the result would end up negative, + // it'll remain to zero instead. + // This caused a bug in master where we would calculate the wrong fee + // for a transaction. + // See https://github.com/bitcoindevkit/bdk/issues/660 + let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh()); + let send_to = Address::from_str("tb1ql7w62elx9ucw4pj5lgw4l028hmuw80sndtntxt").unwrap(); + let fee_rate = FeeRate::from_sat_per_vb(2.01); + let incoming_txid = crate::populate_test_db!( + wallet.database.borrow_mut(), + testutils! (@tx ( (@external descriptors, 0) => 8859 ) (@confirmations 1)), + Some(100), + ); + + let mut builder = wallet.build_tx(); + builder + .add_recipient(send_to.script_pubkey(), 8630) + .add_utxo(OutPoint::new(incoming_txid, 0)) + .unwrap() + .enable_rbf() + .fee_rate(fee_rate); + let (psbt, details) = builder.finish().unwrap(); + + assert!(psbt.inputs.len() == 1); + assert_fee_rate!(psbt, details.fee.unwrap_or(0), fee_rate, @add_signature); + } + #[test] fn test_sign_single_xprv() { let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"); From 632dabaa07ef9c58926facf0af5190f62bb65d12 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 2 Aug 2022 11:31:31 +0200 Subject: [PATCH 8/9] test: Check tx feerate with longer signatures This commit also suppresses the `unused_mut` warning in `assert_fee_rate`, which happens because we call it without `add_signatures`. --- src/wallet/mod.rs | 63 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 6aa8789a..66d34ba2 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -2011,6 +2011,7 @@ pub(crate) mod test { macro_rules! assert_fee_rate { ($psbt:expr, $fees:expr, $fee_rate:expr $( ,@dust_change $( $dust_change:expr )* )* $( ,@add_signature $( $add_signature:expr )* )* ) => ({ let psbt = $psbt.clone(); + #[allow(unused_mut)] let mut tx = $psbt.clone().extract_tx(); $( $( $add_signature )* @@ -5088,4 +5089,66 @@ pub(crate) mod test { .current_height(maturity_time); builder.finish().unwrap(); } + + #[test] + fn test_fee_rate_sign_no_grinding_high_r() { + // Our goal is to obtain a transaction with a signature with high-R (71 bytes + // instead of 70). We then check that our fee rate and fee calculation is + // alright. + let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"); + let addr = wallet.get_address(New).unwrap(); + let fee_rate = FeeRate::from_sat_per_vb(1.0); + let mut builder = wallet.build_tx(); + let mut data = vec![0]; + builder + .drain_to(addr.script_pubkey()) + .drain_wallet() + .fee_rate(fee_rate) + .add_data(&data); + let (mut psbt, details) = builder.finish().unwrap(); + let (op_return_vout, _) = psbt + .unsigned_tx + .output + .iter() + .enumerate() + .find(|(_n, i)| i.script_pubkey.is_op_return()) + .unwrap(); + + let mut sig_len: usize = 0; + // We try to sign many different times until we find a longer signature (71 bytes) + while sig_len < 71 { + // Changing the OP_RETURN data will make the signature change (but not the fee, until + // data[0] is small enough) + data[0] += 1; + psbt.unsigned_tx.output[op_return_vout].script_pubkey = Script::new_op_return(&data); + // Clearing the previous signature + psbt.inputs[0].partial_sigs.clear(); + // Signing + wallet + .sign( + &mut psbt, + SignOptions { + remove_partial_sigs: false, + try_finalize: false, + ..Default::default() + }, + ) + .unwrap(); + // We only have one key in the partial_sigs map, this is a trick to retrieve it + let key = psbt.inputs[0].partial_sigs.keys().next().unwrap(); + sig_len = psbt.inputs[0].partial_sigs[key].sig.serialize_der().len(); + } + // Actually finalizing the transaction... + wallet + .sign( + &mut psbt, + SignOptions { + remove_partial_sigs: false, + ..Default::default() + }, + ) + .unwrap(); + // ...and checking that everything is fine + assert_fee_rate!(psbt, details.fee.unwrap_or(0), fee_rate); + } } From 419dc248b667db05295cd4c68347c4ef51f51023 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Tue, 2 Aug 2022 12:07:16 +0200 Subject: [PATCH 9/9] test: Document `test_bump_fee_add_input_change_dust` Add a rationale for the feerate in the test --- src/wallet/mod.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 66d34ba2..f5a54ac0 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -3781,6 +3781,7 @@ pub(crate) mod test { .del_utxo(&txin.previous_output) .unwrap(); } + let original_tx_weight = tx.weight(); original_details.transaction = Some(tx); wallet .database @@ -3789,7 +3790,20 @@ pub(crate) mod test { .unwrap(); let mut builder = wallet.build_fee_bump(txid).unwrap(); - builder.fee_rate(FeeRate::from_sat_per_vb(141.0)); + // We set a fee high enough that during rbf we are forced to add + // a new input and also that we have to remove the change + // that we had previously + + // We calculate the new weight as: + // original weight + // + extra input weight: 160 WU = (32 (prevout) + 4 (vout) + 4 (nsequence)) * 4 + // + input satisfaction weight: 112 WU = 106 (witness) + 2 (witness len) + (1 (script len)) * 4 + // - change output weight: 124 WU = (8 (value) + 1 (script len) + 22 (script)) * 4 + let new_tx_weight = original_tx_weight + 160 + 112 - 124; + // two inputs (50k, 25k) and one output (45k) - epsilon + // We use epsilon here to avoid asking for a slightly too high feerate + let fee_abs = 50_000 + 25_000 - 45_000 - 10; + builder.fee_rate(FeeRate::from_wu(fee_abs, new_tx_weight)); let (psbt, details) = builder.finish().unwrap(); assert_eq!(