Skip to content

Inconsistent checking of RBF rules #192

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.

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

Open
Tracked by #76
LLFourn opened this issue Dec 30, 2020 · 13 comments · May be fixed by #225
Open
Tracked by #76

Inconsistent checking of RBF rules #192

LLFourn opened this issue Dec 30, 2020 · 13 comments · May be fixed by #225

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Dec 30, 2020

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
Copy link
Member

afilini commented Jan 5, 2021

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
Copy link
Member

evanlinjin commented Sep 14, 2022

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: LLFourn/bdk_core_staging#21

@nondiremanuel
Copy link

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

@notmandatory
Copy link
Member

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

@notmandatory notmandatory reopened this Mar 26, 2024
@notmandatory
Copy link
Member

notmandatory commented Mar 26, 2024

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

@notmandatory
Copy link
Member

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..."

@nondiremanuel nondiremanuel moved this from Done to Todo in BDK Wallet Mar 26, 2024
@LLFourn LLFourn self-assigned this Mar 27, 2024
@LLFourn
Copy link
Contributor Author

LLFourn commented Mar 27, 2024

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

@notmandatory
Copy link
Member

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

@notmandatory
Copy link
Member

@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
Copy link
Member

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.

@notmandatory notmandatory added this to the 2.0.0 milestone Jun 17, 2024
@LLFourn
Copy link
Contributor Author

LLFourn commented Jun 27, 2024

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

@notmandatory notmandatory removed this from the 2.0.0 milestone Mar 13, 2025
@ValuedMammal
Copy link
Collaborator

We can wait until we have the Psbt in hand, get the fee from the psbt and compute an estimated feerate based on the weight of the unsigned tx and the combined satisfaction weights, then proceed to check rules 3 and 4 by comparing the fee/feerate we calculate with the fee of the previous transaction assuming we're in bumping-fee mode.

@notmandatory notmandatory transferred this issue from bitcoindevkit/bdk Apr 7, 2025
Anunayj added a commit to Anunayj/bdk_wallet that referenced this issue May 2, 2025
This commit adds RBF policy based on BIP125 and Bitcoin Core's
Replacement Policy. More specifically replacement transactions
should:
 - Have a higher absolute fee than the original transaction
   (incentivizes miners).
 - Pay enough for their bandwidth (prevents DoS).
 - Have a higher feerate than conflicting transactions
   (BitcoinCore policy, incentivizes miners)
 
The old implementation was checking bip125 rules partially 
based on the fee policy chosen in txbuilder, and also was
following a wrong assumption that 
	feerate >= previous_feerate + BROADCAST_MIN 

Also fixes a test based on the above assumption.

Fixes: bitcoindevkit#192
@Anunayj Anunayj linked a pull request May 2, 2025 that will close this issue
8 tasks
@Anunayj
Copy link

Anunayj commented May 2, 2025

I realize I should've probably checked in before starting to work on this, I hope this was up for grabs as Rob mentioned this in his list of possible things to work on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

7 participants