diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs index ca0fc47d8..55e64305d 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); } diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 4ee3835d9..f5a54ac00 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,25 @@ 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()); + + // 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() { @@ -851,6 +861,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); } }; @@ -1858,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(); @@ -1988,12 +2009,14 @@ 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(); + #[allow(unused_mut)] + let mut tx = $psbt.clone().extract_tx(); $( $( $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 } )* @@ -2005,11 +2028,23 @@ 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; 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); } @@ -2384,7 +2419,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 +2432,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] @@ -3210,7 +3245,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() @@ -3254,7 +3289,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] @@ -3270,7 +3305,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() @@ -3336,7 +3371,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() @@ -3364,7 +3399,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] @@ -3380,7 +3415,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() @@ -3436,7 +3471,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() @@ -3493,7 +3528,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() @@ -3534,7 +3569,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() @@ -3575,7 +3610,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] @@ -3597,7 +3632,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() @@ -3668,7 +3703,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() @@ -3715,7 +3750,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] @@ -3739,13 +3774,14 @@ 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() .del_utxo(&txin.previous_output) .unwrap(); } + let original_tx_weight = tx.weight(); original_details.transaction = Some(tx); wallet .database @@ -3754,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!( @@ -3778,7 +3827,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] @@ -3800,7 +3849,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() @@ -3849,7 +3898,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] @@ -3871,7 +3920,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() @@ -3950,7 +3999,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() @@ -3994,7 +4043,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() @@ -4016,6 +4065,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/*)"); @@ -5022,4 +5103,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); + } }