search

bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
839 stars 299 forks source link

Inconsistent checking of RBF rules #256

Open LLFourn opened 3 years ago

LLFourn commented 3 years ago

BIP125 rules 3 and 4 are not being checked properly according to my understanding:

https://github.com/bitcoindevkit/bdk/blob/c2b2da7601fb570952cfd858f3821b3a4fd87ec9/src/wallet/mod.rs#L730-L746

It should be checking rules 3 and 4 regardless of the fee policy chosen in txbuilder.

afilini commented 3 years ago

I think we can kinda ignore rule 3 at the moment, since we don't support merging multiple unconfirmed transactions, so this:

The replacement transaction pays an absolute fee of at least the sum paid by the original transactions.

For us basically means that we should just pay at least the previous fee.

I agree on rule 4 though, the FeeAmount branch should check for *amount < details.fees + _something_.

And now that I think about it, it's kind of a chicken-and-egg problem: rule 4 says that we should pay at least the size of the new transaction, but at this point we don't know it yet. So even the part above where required_feerate is computed is wrong.. Not sure what's the best way to handle this, I've gotta think about it a bit more..

evanlinjin commented 1 year ago

Coin selection algorithms should take min_absolute_fee and target_feerate into consideration on the get-go (and only succeed when both constraints are satisfied).

bdk_core will solve this issue, WIP here: https://github.com/LLFourn/bdk_core_staging/pull/21

nondiremanuel commented 11 months ago

We discussed it in the call and decided to close this issue. Please comment if you think it should be re-opened

notmandatory commented 5 months ago

Per discussion in dev call today this doesn't seem to be fixed yet.

notmandatory commented 5 months ago

Per @evanlinjin we can fix this issue by using the new bdk_coin_select crate.

notmandatory commented 5 months ago

More notes from @evanlinjin from discord chat: "We already have the TxParams::bumping_fee field we just aren't checking them correctly. However, I am concerned about rule 4 not being enforced properly (since we are actually doing coin selection again for RBF). An easy policy (that would enforce rule 4) would be to say the new tx cannot be larger than the one we are replacing. However, this is not what we are doing with our RBF logic. We add all old inputs into TxParams::utxos which is inputs that must be spent.

Therefore, it is possible that our new transaction may be larger in weight than the old one Right now, I'm very tempted to say let's use bdk_coin_select..."

LLFourn commented 5 months ago

I volunteer to try and fix RBF API with bdk_coin_select. Don't have an ETA yet.

notmandatory commented 5 months ago

Thanks @LLFourn , if you can fix with integration of bdk_coin_select that'd be great.

notmandatory commented 3 months ago

@LLFourn Since we're not going to have have a bdk_coin_select crate for 1.0 can I defer this issue to the 2.0 milestone?

notmandatory commented 2 months ago

Yes I'd like to make the extracted bdk_coin_select one of the priorities for 2.0 so if it's better to fix this together with that I'll update the milestone.

LLFourn commented 2 months ago

Sorry ACK I haven't had time to do this work so yes you can push it to 2.0.0. Thanks.