search

bitcoindevkit / bdk

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

Substitute `enable_rbf` in `TxBuilder` with `disable_rbf` #791

Open danielabrozzoni opened 1 year ago

danielabrozzoni commented 1 year ago

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

notmandatory commented 1 year ago

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 commented 1 year ago

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 commented 5 months ago

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

FlamingSaint commented 5 months ago

@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 commented 5 months ago

@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 commented 4 months ago

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