Skip to content

fix: Inconsistent checking of RBF rules. #225

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: Inconsistent checking of RBF rules. #225

wants to merge 4 commits into from

Conversation

Anunayj
Copy link

@Anunayj Anunayj commented May 2, 2025 •
edited
Loading

Description

This PR adds checks based on RBF policy (BIP125 and Bitcoin Core's Replacement Policy).
More specifically it checks transactions:

  1. Have a higher absolute fee than the original transaction
  2. Pay extra for their bandwidth.
  3. Have a higher feerate than conflicting transactions.

Fixes: #192

Notes to the reviewers

The addition of satisfaction_weight to CoinSelectionResult would be a breaking change, it isn't entirely necessary, as I could duplicate that functionality in wallet itself by making an estimated_weight(psbt: Psbt) function, but this was one of the cleaner ways to go about this, and I'm not entirely sure how acceptable breaking changes are in the project.

Changelog notice

Changed

  • RBF Policies (BIP125 and Bitcoin Core's) are now checked correctly when creating a transaction with bumping_fee.

BREAKING

  • CoinSelectionResult now has a satisfaction_weight field, giving the max weight required to satisfy the selected inputs

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt before committing
  • I ran cargo clippy before committing

New Features:

  • I've added tests for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Anunayj added 4 commits May 2, 2025 17:34
@Anunayj
fix: test values not conforming to BIP125.
6a95bdb 
Two of the values in tests were too low for RBF based on BIP125.
@Anunayj
feat(coin_selection)! Add satisfaction_weight to CoinSelectionResult
f0fba05 
BREAKING CHANGE: It might be better to add satisfaction_weight
calculation to the wallet instead, or maybe to PsbtUtils?
@Anunayj
fix: BIP125 rules not being checked properly
57da7ca 
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
test: add test for RBF edgecase when feerate is almost equal
e19feab 
This test documents the case where the replacement transaction
has a feerate that is almost equal to the original transaction
but is still RBF replaceable.

The previous implementation would wrongly fail on this test.
@Anunayj Anunayj changed the title Fix Inconsistent checking of RBF rules. fix: Inconsistent checking of RBF rules. May 3, 2025
@rustaceanrob
Copy link
Contributor

Nice job, although I am not sure how to tinker with this to make it non-breaking, but otherwise looks good. Would it be possible to migrate this to bdk_tx? Because it hasn't become an issue in production, it might be best to put this in the new module instead

@evanlinjin
Copy link
Member

There has already been work on this in bitcoindevkit/bdk-tx#1. Please take a look there. Reviews and tests will be helpful.

Note that we are trying to put a feature-freeze on TxBuilder in bdk_wallet and want to focus effort in bdk_tx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
BDK Wallet
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent checking of RBF rules
3 participants
@Anunayj @rustaceanrob @evanlinjin