feat(wallet)!: enable RBF by default on TxBuilder

[luisschwab]

Description

Closes #791

This PR enables RBF by default on TxBuilder

Notes to the reviewers

Changelog notice

• On TxParams, rbf becomes sequence
• Added set_exact_sequence(), so the user can define an arbitrary sequence value
• n_sequence now defaults to 0xFFFFFFFD
• Adjusted tests accordingly

Checklists

All Submissions:

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

New Features:

• I've added tests for the new feature
• I've added docs 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

[ValuedMammal]

Thanks for working on this @luisschwab

[rustaceanrob]

I am inclined to think everyone with production applications will have enable_rbf selected. I wonder how we feel about adding a deprecated macro and having the method essentially do nothing instead of removing it. That way an LSP will show the deprecation warning but this would technically not be a breaking change.

[ValuedMammal]

I have a slightly different take but I think we agree on the end goal of making RBF the default for new transactions. I think the place to address it is by fixing the logic in create_tx to first check the value of requirements.csv, then allow any Sequence value passed in by the user, or else fall back to the new default ENABLE_RBF_NO_LOCKTIME (0xFFFFFFFD). To make it simple RbfValue should implement Default but maybe not strictly necessary.

Instead of disable_rbf I think we should have a method nsequence that accepts a Sequence and internally sets the rbf value. If we go this way, it's likely we'd then have to remove the error CreateTxError::RbfSequence that tries to stop you from setting an rbf value too high. Let me know what you think.

[notmandatory]

I added this to the 1.0.0-beta milestone since it's an breaking API change that should be done for 1.0.

[ValuedMammal]

How's it going on this @luisschwab ? I hope we didn't derail progress too much with the previous discussion. I like that you made an effort not to change the existing API - for what it's worth, the bdk team had a talk and determined that if we're going to break the API as it relates to enabling RBF then it's better to do it now while still in beta. While the implementation here looks correct I'd prefer that we get the API sorted out rather than postpone it.

After thinking more about it I want to update my suggestion to instead change the TxParams rbf field to sequence: Option<Sequence> and simply remove the RbfValue type from the codebase, the rationale being that if we enable RBF by default then the TxBuilder API only needs to accommodate requesting a specific nSequence. In my opinion this is simpler, but let me know if that sounds radically off-course.

[luisschwab]

Sorry, I've been busy these last few days, but will resume work on this later today. Yes, since we're still in beta it's best to get it right rather than breaking the API later.

[evanlinjin]

@ValuedMammal I agree with your thinking. Option<Sequence> makes sense to me. People who want to signal no RBF can use 0xFFFFFFFE.

[luisschwab]

Sounds good, I'll update the PR taking this into consideration.

[evanlinjin]

@luisschwab how is progress on this so far? Do you need a hand with this?

[ValuedMammal]

Try to fix the formatting checks caught by CI, and also I left a few comments that I wonder if you or anyone has an opinion on.

[luisschwab]

Forgot to rename some things.

[luisschwab]

Rebased due to 519728c

[ValuedMammal]

ACK f6fa15e

In the interest of building the whole workspace I suppose f6fa15e could be squashed in.

[luisschwab]

Re-added test and squashed commits.

[notmandatory]

ACK f15551b

Looks good!

[notmandatory]

This change was also recommended by external code audit: "RBF signalling should probably be turned on by default. First because it could be misinterpreted by users as "this way my transaction has significantly less chances of getting replaced" while most of the network hashrate runs with full-RBF and from 28.0 it's also the Bitcoin Core default (nodes will relay replacements for transaction not explicitly signalling RBF). It is also a fingerprint as around 70% of transactions signal RBF right now, and this number is growing."