test: BDK won't add unconf inputs when fee bumping #648
Conversation
| 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!( |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
@danielabrozzoni checking whether we only pick confirmed new inputs would be great too!
There was a problem hiding this comment.
Right, I'll add it, thanks!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@danielabrozzoni Fair, let's skip it :P I like the new test!
|
@notmandatory It seems this PR is just test additions, should the |
|
Isn't this intentional? BIP125
|
This ticket is wrongly labelled as @danielabrozzoni means that we shouldn't allow introducing new unconfirmed inputs when RBF. |
|
Yeah, I removed the "bug" label and added the "test" one. #144 was describing a bug that has been fixed since, but here I'm adding a test to check that the bug doesn't appear anymore. I'll update the review notes to make it clear! |
danielabrozzoni
force-pushed
the
144_rbf_rules
branch
from
a1518d1 to
b996ab1
Compare
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.
danielabrozzoni
force-pushed
the
144_rbf_rules
branch
2 times, most recently
from
88aea47 to
127db37
Compare
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.
Fixes bitcoindevkit#144 Also removes a leftover dbg!() in a test
danielabrozzoni
force-pushed
the
144_rbf_rules
branch
from
127db37 to
5d00f82
Compare
5 participants
Description
Closes #144
Notes to reviewers
#144 is describing a bug that doesn't seem to happen in BDK master anymore (BDK not respecting BIP125 rule 2). This PR just adds a test to check that the bug is fixed.
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: