wallet, rpc: Opt in to RBF by default #25610

achow101 · 2022-07-13T21:13:00Z

The GUI currently opts in to RBF by default, but RPCs do not, and -walletrbf is default disabled. This PR makes the default in those two places to also opt in.

The last time this was proposed (#9527), the primary objections were the novelty at the time, the inability to bump transactions, and the gui not having the option to disable rbf. In the 5 years since, RBF usage has steadily grown, with ~27% of txs opting in. The GUI has the option to enable/disable RBF, and is also defaulted to having it enabled. And we have the ability to bump RBF'd transactions in both the RPC and the GUI. So I think it makes sense to finally change the default to always opt in to RBF.

ariard · 2022-07-14T00:08:07Z

Concept ACK

DrahtBot · 2022-07-14T03:49:48Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25344 (New outputs argument for bumpfee/psbtbumpfee by rodentrabies)
#21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2022-07-14T06:24:40Z

src/rpc/rawtransaction.cpp

Suggested change

std::optional<bool> rbf;

if (!request.params[3].isNull()) {

rbf = request.params[3].isTrue();

}

const bool rbf{request.params[3].isNull() ? true : request.params[3].getBool()};

Seems complicated to use optional<bool> when it is not needed?

The use of the optional was done to allow for the current behavior with user defined sequence numbers to work. There is a check in ConstructTransaction that the sequence numbers match the replaceable option. It will throw an error if non-RBF sequences are used, but replaceable is true. Additionally, if replacable is either not specified or false (which is the same thing currently), setting a RBF sequence does not result in an error. If the default is just changed to true, then this check fails when the user sets non-RBF sequence numbers and does not specify replaceable at all. By using an optional we can retain the behavior that sequences are not checked against replaceable when replaceable is not specified.

maflcko · 2022-07-14T06:28:15Z

I've closed #25602, so Concept ACK on this change to take over.

glozow · 2022-07-14T06:47:30Z

concept ACK

darosior · 2022-07-14T07:39:32Z

Concept ACK

theStack

Concept ACK

Should these changes of defaults be mentioned in the release notes?

instagibbs · 2022-07-14T13:20:57Z

concept ACK

Sjors · 2022-07-14T13:57:16Z

No strong feelings.

Needs a clear release note, because there may still be systems that rely on RBF being off by default. Could also document that the two ways to work around the change are to set walletrbf=0 or to set the RBF parameter in the RPC call. The latter may seem like the most obvious solution, but sometimes software depends on dependencies of dependencies to actually call the RPC. (And those don't always let you pass every parameter)

maflcko · 2022-07-14T14:26:41Z

Maybe there could be a global -rpcrawtransactionsrbf option to address your concern for raw transactions?

achow101 · 2022-07-14T19:15:44Z

Added some release notes.

src/rpc/rawtransaction_util.cpp

darosior

ACK 6b59d754644a5eda6ff8b4378de5320f31cc8111

Happy to reACK if you take Marco's nit.

darosior · 2022-07-16T09:07:03Z

reACK ab3c06d

luke-jr

Concept ACK.

I find it strange that the RPCs don't default to the -walletrbf setting. Was there a reason for that?

achow101 · 2022-07-16T17:34:56Z

I find it strange that the RPCs don't default to the -walletrbf setting. Was there a reason for that?

createrawtransaction and createpsbt are non-wallet RPCs and so need to work without a wallet. The wallet RPCs already follow the -walletrbf setting.

aureleoules

ACK ab3c06d.

I verified that the rpc wallet option replaceable is enabled by default and the wallet RBF feature is enabled by default as well.

Would it make sense to remove -walletrbf=1 in the test below, to make use of this change?

bitcoin/test/functional/rpc_psbt.py

Line 38 in ab3c06d

    
           ["-walletrbf=1", "-addresstype=bech32", "-changetype=bech32"], #TODO: Remove address type restrictions once taproot has psbt extensions

glozow

utACK ab3c06d

+1 to this comment in e3c3363:

Would it make sense to remove -walletrbf=1 in the test below, to make use of this change?

and I think -walletrbf=1 can be removed for wallet_send.py as well

glozow · 2022-07-18T11:20:39Z

src/rpc/rawtransaction_util.cpp

    }

-    if (rbf && rawTx.vin.size() > 0 && !SignalsOptInRBF(CTransaction(rawTx))) {
+    if (rbf.has_value() && rbf.value() && rawTx.vin.size() > 0 && !SignalsOptInRBF(CTransaction(rawTx))) {


Understanding this as: ConstructTransaction uses rbf to both determine sequence numbers and enforce signaling on the resulting transaction, and since default rbf is true, it will require rbf on every transaction by default, even if the inputs were entirely specified by the user. We don't want to force users to now specify rbf=false if they want to manually construct a transaction that doesn't signal rbf. So this is why we change it to a tristate to represent "tx should signal rbf" vs "rbf signal doesn't matter."

maflcko · 2022-07-18T11:55:36Z

Would it make sense to remove -walletrbf=1 in the test below, to make use of this change?

One could argue that tests should document what settings they rely on, even if the value they rely on is the default value, so it could also be left as-is, but I don't have a strong opinion.

murchandamus · 2022-08-01T13:49:48Z

Post-merge ACK

wallet: Enable -walletrbf by default

e3c3363

This was referenced Jul 14, 2022

wallet, RPC: Default BIP125 signal to -mempoolfullrbf #25602

Closed

New outputs argument for bumpfee/psbtbumpfee #25344

Merged

mostafanorimisa approved these changes Jul 14, 2022

View reviewed changes

maflcko reviewed Jul 14, 2022

View reviewed changes

fanquake requested review from instagibbs and sdaftuar July 14, 2022 07:45

maflcko added Wallet RPC/REST/ZMQ labels Jul 14, 2022

theStack reviewed Jul 14, 2022

View reviewed changes

maflcko added the Needs release note label Jul 14, 2022

maflcko added this to the 24.0 milestone Jul 14, 2022

DrahtBot mentioned this pull request Jul 14, 2022

Implement BIP 370 PSBTv2 #21283

Open

maflcko removed the Needs release note label Jul 15, 2022

maflcko reviewed Jul 15, 2022

View reviewed changes

src/rpc/rawtransaction_util.cpp Outdated Show resolved Hide resolved

darosior reviewed Jul 15, 2022

View reviewed changes

achow101 added 2 commits July 15, 2022 11:46

rpc: Default rbf enabled

61d9149

doc: Release notes for default RBF

ab3c06d

achow101 force-pushed the walletrbf-default-on branch from 6b59d75 to ab3c06d Compare July 15, 2022 15:56

luke-jr reviewed Jul 16, 2022

View reviewed changes

aureleoules reviewed Jul 18, 2022

View reviewed changes

glozow reviewed Jul 18, 2022

View reviewed changes

glozow requested a review from murchandamus July 18, 2022 11:22

maflcko merged commit c5ba1d9 into bitcoin:master Aug 1, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 1, 2022

Merge bitcoin#25610: wallet, rpc: Opt in to RBF by default

f027fa8

bitcoin locked and limited conversation to collaborators Aug 1, 2023

-    std::optional<bool> rbf;
-    if (!request.params[3].isNull()) {
-        rbf = request.params[3].isTrue();
-    }
+    const bool rbf{request.params[3].isNull() ? true : request.params[3].getBool()};

wallet, rpc: Opt in to RBF by default #25610

wallet, rpc: Opt in to RBF by default #25610

Uh oh!

Conversation

achow101 commented Jul 13, 2022

Uh oh!

ariard commented Jul 14, 2022

Uh oh!

DrahtBot commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 14, 2022

Uh oh!

glozow commented Jul 14, 2022

Uh oh!

darosior commented Jul 14, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 14, 2022

Uh oh!

Sjors commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jul 14, 2022

Uh oh!

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

darosior commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 16, 2022

Uh oh!

aureleoules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 18, 2022

Uh oh!

murchandamus commented Aug 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

DrahtBot commented Jul 14, 2022 •

edited

Loading

achow101 Jul 14, 2022 •

edited

Loading

Sjors commented Jul 14, 2022 •

edited

Loading

maflcko commented Jul 14, 2022 •

edited

Loading

darosior commented Jul 16, 2022 •

edited

Loading

aureleoules left a comment •

edited

Loading