From 1d9fdd01faf3a0f17c57381a7c54d136e9d69ffe Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Thu, 30 Jun 2022 20:31:38 +0200 Subject: [PATCH 1/3] Remove wrong TODO comment in build_fee_bump The proposed solution is bad for privacy as well. Let's call the initial change output, which is normally shrink when you fee bump, change#1, and the extra output aforementioned change#2 (as, in this case, it's going to be a change output as well). If you add change#2 you might not revel change#1, but you're still revealing change#2. You're not improving your privacy, and you're wasting money in fees. --- src/wallet/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 4abe2574..213cc60d 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -899,8 +899,6 @@ where /// # Ok::<(), bdk::Error>(()) /// ``` // TODO: support for merging multiple transactions while bumping the fees - // TODO: option to force addition of an extra output? seems bad for privacy to update the - // change pub fn build_fee_bump( &self, txid: Txid, From 98748906f6799041341227de33bec20e8c6ef4b0 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Fri, 1 Jul 2022 10:56:48 +0200 Subject: [PATCH 2/3] test: fix populate_test_db conf calculation populate_test_db would previously give back a transaction with N + 1 confirmations when you asked for N. This commit also fixes test_spend_coinbase, which would improperly ask for a transaction with 0 confirmations instead of 1. --- src/database/memory.rs | 11 +++++++---- src/wallet/mod.rs | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/database/memory.rs b/src/database/memory.rs index cc56c94d..7d806eb4 100644 --- a/src/database/memory.rs +++ b/src/database/memory.rs @@ -512,10 +512,13 @@ macro_rules! populate_test_db { }; let txid = tx.txid(); - let confirmation_time = tx_meta.min_confirmations.map(|conf| $crate::BlockTime { - height: current_height.unwrap().checked_sub(conf as u32).unwrap(), - timestamp: 0, - }); + let confirmation_time = tx_meta + .min_confirmations + .and_then(|v| if v == 0 { None } else { Some(v) }) + .map(|conf| $crate::BlockTime { + height: current_height.unwrap().checked_sub(conf as u32).unwrap() + 1, + timestamp: 0, + }); let tx_details = $crate::TransactionDetails { transaction: Some(tx.clone()), diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 213cc60d..36cc1622 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -4723,7 +4723,7 @@ pub(crate) mod test { crate::populate_test_db!( wallet.database.borrow_mut(), - testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 0)), + testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)), Some(confirmation_time), (@coinbase true) ); From 5d00f8238886a993ef21056e5b3e216a4aae6951 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Fri, 1 Jul 2022 11:11:11 +0200 Subject: [PATCH 3/3] test that BDK won't add unconf inputs when fee bumping Fixes #144 Also removes a leftover dbg!() in a test --- src/wallet/mod.rs | 94 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 36cc1622..d7d055b0 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -2262,7 +2262,6 @@ pub(crate) mod test { .drain_to(drain_addr.script_pubkey()) .drain_wallet(); let (psbt, details) = builder.finish().unwrap(); - dbg!(&psbt); let outputs = psbt.unsigned_tx.output; assert_eq!(outputs.len(), 2); @@ -3859,6 +3858,99 @@ pub(crate) mod test { assert_eq!(details.fee.unwrap_or(0), 250); } + #[test] + #[should_panic(expected = "InsufficientFunds")] + fn test_bump_fee_unconfirmed_inputs_only() { + // We try to bump the fee, but: + // - We can't reduce the change, as we have no change + // - All our UTXOs are unconfirmed + // So, we fail with "InsufficientFunds", as per RBF rule 2: + // The replacement transaction may only include an unconfirmed input + // if that input was included in one of the original transactions. + let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh()); + let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); + let mut builder = wallet.build_tx(); + builder + .drain_wallet() + .drain_to(addr.script_pubkey()) + .enable_rbf(); + let (psbt, mut original_details) = builder.finish().unwrap(); + // Now we receive one transaction with 0 confirmations. We won't be able to use that for + // fee bumping, as it's still unconfirmed! + crate::populate_test_db!( + wallet.database.borrow_mut(), + testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 0)), + Some(100), + ); + let mut tx = psbt.extract_tx(); + let txid = tx.txid(); + for txin in &mut tx.input { + txin.witness.push([0x00; 108]); // fake signature + wallet + .database + .borrow_mut() + .del_utxo(&txin.previous_output) + .unwrap(); + } + original_details.transaction = Some(tx); + wallet + .database + .borrow_mut() + .set_tx(&original_details) + .unwrap(); + + let mut builder = wallet.build_fee_bump(txid).unwrap(); + builder.fee_rate(FeeRate::from_sat_per_vb(25.0)); + builder.finish().unwrap(); + } + + #[test] + fn test_bump_fee_unconfirmed_input() { + // We create a tx draining the wallet and spending one confirmed + // and one unconfirmed UTXO. We check that we can fee bump normally + // (BIP125 rule 2 only apply to newly added unconfirmed input, you can + // always fee bump with an unconfirmed input if it was included in the + // original transaction) + let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh()); + let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); + // We receive a tx with 0 confirmations, which will be used as an input + // in the drain tx. + crate::populate_test_db!( + wallet.database.borrow_mut(), + testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 0)), + Some(100), + ); + let mut builder = wallet.build_tx(); + builder + .drain_wallet() + .drain_to(addr.script_pubkey()) + .enable_rbf(); + let (psbt, mut original_details) = builder.finish().unwrap(); + let mut tx = psbt.extract_tx(); + let txid = tx.txid(); + for txin in &mut tx.input { + txin.witness.push([0x00; 108]); // fake signature + wallet + .database + .borrow_mut() + .del_utxo(&txin.previous_output) + .unwrap(); + } + original_details.transaction = Some(tx); + wallet + .database + .borrow_mut() + .set_tx(&original_details) + .unwrap(); + + let mut builder = wallet.build_fee_bump(txid).unwrap(); + builder + .fee_rate(FeeRate::from_sat_per_vb(15.0)) + .allow_shrinking(addr.script_pubkey()) + .unwrap(); + builder.finish().unwrap(); + } + #[test] fn test_sign_single_xprv() { let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");