Skip to content
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

Substitute enable_rbf in TxBuilder with disable_rbf #791

Closed
Tracked by #39
danielabrozzoni opened this issue Oct 31, 2022 · 7 comments · Fixed by #1616
Closed
Tracked by #39

Substitute enable_rbf in TxBuilder with disable_rbf #791

danielabrozzoni opened this issue Oct 31, 2022 · 7 comments · Fixed by #1616
Assignees
Labels
api A breaking API change good first issue Good for newcomers module-wallet
Milestone

Comments

@danielabrozzoni
Copy link
Member

i.e., maybe all our txs should be RBF by default :)

@notmandatory
Copy link
Member

Concept ACK, seems to be direction bitcoin core is going with bitcoin/bitcoin#25353. Even with all the controversy I think best practice from a general wallet point of view is to enable RBF so fees can easily be bumped when needed.

@notmandatory
Copy link
Member

Since this is a breaking change we're going to hold off on this until 1.0 since that is a more obviously breaking release.

@notmandatory notmandatory added this to the Alpha 1.0.0 milestone Mar 2, 2023
@notmandatory notmandatory removed this from the 1.0.0-alpha.0 milestone Mar 3, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 17, 2024
@notmandatory notmandatory added good first issue Good for newcomers api A breaking API change module-wallet labels Mar 17, 2024
@notmandatory
Copy link
Member

A small change but I think we should push to 2.0 milestone.

@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
@FlamingSaint
Copy link

@notmandatory I would like to work on this, As far as I have understood I need to replace enable_rbf with disable_rbf in tx_builder.rs. This new function shouldn't initialize the value of rbf with some default value. Also, I assume I need to replace all instances of enable_rbf with disable_rbf.

@notmandatory
Copy link
Member

@FlamingSaint hi sure go ahead and create a PR, there might also be some additional related changes, see discord discussion here: https://discord.com/channels/753336465005608961/753367451319926827/1228063458952478792

@notmandatory
Copy link
Member

@FlamingSaint per your question on discord, yes you should use the same PR to make RBF the default for TxBuilder. So yes add disable_rbf() but since we still need a way to re-enable and enable RBF with a sequence, how about keeping the existing enable_rbf() and enable_rbf_with_sequence(RbfValue)?

Btw, let's keep the discussion here for people who don't use discord.

@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 22, 2024
@luisschwab
Copy link
Contributor

I'm picking up this issue, as it seems to have gone stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change good first issue Good for newcomers module-wallet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants