wallet, rpc: Opt in to RBF by default

achow101 commented at 9:13 pm on July 13, 2022: member

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.

wallet: Enable -walletrbf by default e3c33637ba

ariard commented at 0:08 am on July 14, 2022: member

Concept ACK

DrahtBot commented at 3:49 am on July 14, 2022: contributor

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.

mostafanorimisa approved

in src/rpc/rawtransaction.cpp:310 in 51062c581c outdated

303@@ -304,7 +304,7 @@ static RPCHelpMan createrawtransaction()
304         }, true
305     );
306 
307-    bool rbf = false;
308+    std::optional<bool> rbf;
309     if (!request.params[3].isNull()) {
310         rbf = request.params[3].isTrue();
311     }

MarcoFalke commented at 6:24 am on July 14, 2022:

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

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

achow101 commented at 2:48 pm on July 14, 2022:

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.

MarcoFalke commented at 6:28 am on July 14, 2022: member

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

glozow commented at 6:47 am on July 14, 2022: member

concept ACK

darosior commented at 7:39 am on July 14, 2022: member

Concept ACK

fanquake requested review from instagibbs on Jul 14, 2022

fanquake requested review from sdaftuar on Jul 14, 2022

MarcoFalke added the label Wallet on Jul 14, 2022

MarcoFalke added the label RPC/REST/ZMQ on Jul 14, 2022

theStack commented at 11:46 am on July 14, 2022: contributor

Concept ACK

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

MarcoFalke added the label Needs release note on Jul 14, 2022

MarcoFalke added this to the milestone 24.0 on Jul 14, 2022

instagibbs commented at 1:20 pm on July 14, 2022: member

concept ACK

Sjors commented at 1:57 pm on July 14, 2022: member

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)

MarcoFalke commented at 2:26 pm on July 14, 2022: member

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

achow101 commented at 7:15 pm on July 14, 2022: member

Added some release notes.

MarcoFalke removed the label Needs release note on Jul 15, 2022

in src/rpc/rawtransaction_util.cpp:64 in 6b59d75464 outdated

59@@ -60,7 +60,8 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
60             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative");
61 
62         uint32_t nSequence;
63-        if (rbf) {
64+
65+        if (!rbf.has_value() || (rbf.has_value() && rbf.value())) {

MarcoFalke commented at 7:56 am on July 15, 2022:

nit to simplify:

0        if (rbf.value_or(true)) {

achow101 commented at 4:43 pm on July 15, 2022:

Done

darosior commented at 8:28 am on July 15, 2022: member

ACK 6b59d754644a5eda6ff8b4378de5320f31cc8111

Happy to reACK if you take Marco’s nit.

rpc: Default rbf enabled 61d9149e78

doc: Release notes for default RBF ab3c06db1a

achow101 force-pushed on Jul 15, 2022

darosior commented at 9:07 am on July 16, 2022: member

reACK ab3c06db1aed979847158505f3df1dcea9fd6c2b

luke-jr commented at 5:23 pm on July 16, 2022: member

Concept ACK.

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

achow101 commented at 5:34 pm on July 16, 2022: member

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 commented at 9:05 am on July 18, 2022: member

ACK ab3c06db1aed979847158505f3df1dcea9fd6c2b.

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? https://github.com/bitcoin/bitcoin/blob/ab3c06db1aed979847158505f3df1dcea9fd6c2b/test/functional/rpc_psbt.py#L38

in src/rpc/rawtransaction_util.cpp:136 in 61d9149e78 outdated

132@@ -132,7 +133,7 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
133         }
134     }
135 
136-    if (rbf && rawTx.vin.size() > 0 && !SignalsOptInRBF(CTransaction(rawTx))) {
137+    if (rbf.has_value() && rbf.value() && rawTx.vin.size() > 0 && !SignalsOptInRBF(CTransaction(rawTx))) {

glozow commented at 11:20 am on July 18, 2022:

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

glozow commented at 11:22 am on July 18, 2022: member

utACK ab3c06db1a

+1 to this comment in e3c33637bac7db8ae56ab497df10911fad773981:

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 requested review from murchandamus on Jul 18, 2022

MarcoFalke commented at 11:55 am on July 18, 2022: member

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.

MarcoFalke merged this on Aug 1, 2022

MarcoFalke closed this on Aug 1, 2022

murchandamus commented at 1:49 pm on August 1, 2022: contributor

Post-merge ACK

sidhujag referenced this in commit f027fa890f on Aug 1, 2022

bitcoin locked this on Aug 1, 2023

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

Conflicts