Skip to content

test: BDK won't add unconf inputs when fee bumping #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
afilini merged 3 commits into from
Jul 6, 2022
Merged

test: BDK won't add unconf inputs when fee bumping #648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d9fdd0
Remove wrong TODO comment in build_fee_bump
danielabrozzoni Jun 30, 2022
9874890
test: fix populate_test_db conf calculation
danielabrozzoni Jul 1, 2022
5d00f82
test that BDK won't add unconf inputs when fee bumping
danielabrozzoni Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/database/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down
98 changes: 94 additions & 4 deletions src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2264,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);
Expand Down Expand Up @@ -3861,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!(
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to test replacing txs with different amounts of inputs (where inputs are of various states of unconfirmed/confirmed)?

Not sure if it should be included for the scope of this PR though...

Copy link
Copy Markdown
Member Author

@danielabrozzoni danielabrozzoni Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the "end goal" of such test would be. What are you testing exactly? That we would choose only confirmed inputs for fee bumping?

Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we can test whether BDK allows for RBF of tx with unconfirmed parent.

Given:
* Wallet with 2 UTXOs (1 confirmed, 1 unconfirmed).
* We attempt to "drain wallet", resulting in a tx with 2 inputs 
  (1 unconfirmed, 1 confirmed) and 1 output to drain address.

When: 
* Attempt RBF of "drain tx".

Expect:
* Successful RBF of "drain tx".

Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielabrozzoni checking whether we only pick confirmed new inputs would be great too!

Copy link
Copy Markdown
Member Author

@danielabrozzoni danielabrozzoni Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll add it, thanks!

Copy link
Copy Markdown
Member Author

@danielabrozzoni danielabrozzoni Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we can test whether BDK allows for RBF of tx with unconfirmed parent. [...]

I've added test_bump_fee_unconfirmed_input testing this scenario

checking whether we only pick confirmed new inputs would be great too!

Mh, I think this is already tested (at least partially) in test_bump_fee_unconfirmed_inputs_only, and I can't seem to find a better way to test it... Maybe I could send a lot of unconfirmed inputs and one unconfirmed output to the wallet, and check that BDK will only pick the confirmed input for fee bumping. What do you think? Is it a test worth adding in your opinion?

Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielabrozzoni Fair, let's skip it :P I like the new test!

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/*)");
Expand Down Expand Up @@ -4725,7 +4815,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)
);
Expand Down