[Wallet] Enable RBF by default in QT

[Sjors]

If there are no objections, this would supersede #11556.

Enabling RBF by default avoids the need to explain all possible use cases of RBF.

This PR does not change the default RPC wallet behavior, as this could break implementations that depend on it and it's not clear what happens when automated services suddenly switch on RBF on a large scale.

After trying various approaches, we settled on just having QT ignore -walletrbf.

Send screen:

Confirmation screen by default (with RBF):

Confirmation screen without RBF:

[Sjors]

Requesting Travis restart. Most of the environments passed, three failed for txn_clone.py, which seems unrelated.

[TheBlueMatt]

Concept ACK. I believe the test failures are, in fact, because of the change in default.

[Sjors]

@TheBlueMatt ok, I'll double check that. Any theory why they would not fail on all machines (including my own)?

[TheBlueMatt]

listtransactions fails for me locally, too.

[meshcollider]

@Sjors not all the travis machines run the full python test suite, each one serves a different purpose, but those three failing definitely means its related to this change :)

The listtransactions.py failure is due to assertions that transactions are not opt-in, such as:

bitcoin/test/functional/listtransactions.py

Line 117 in 2f959a5

assert(not is_opt_in(self.nodes[0], txid_1))

[meshcollider]

Number of unrelated whitespace changes too, you can disable your editor from doing that by default :)

[meshcollider]

Nit, braces on same line as if and else

[meshcollider]

Nit, indentation
Also perhaps add reference to BIP 125 again

[Sjors]

Will do.

[maflcko]

Please don't tag with [qt], when you are actually modifying the default for all wallet created transactions

[maflcko]

Would also be helpful to have some reasoning in OP that outlines how this will not cause user dissatisfaction because some merchants handle rbf txes differently.

[Sjors]

@MarcoFalke I'll try if I can refactor this in a way that in only impacts QT. Changing the default behavior of RPC is probably not a good idea, since there are automated integrations.

The current implementation also doesn't warn users that some merchants handle RBF differently, but this may indeed be useful when it becomes the default. Different is not always worse by the way; e.g. ShapeShift will show you a recommended fee to bump to if you want a tx to confirm faster.

@meshcollider good to know not all Travis machines run the same tests. Does make test not cover everything either?

[Sjors]

@MarcoFalke I'll take a look at how some merchants and payment processors currently handle RBF, and add that to my PR description. Unless someone has already done that recently and can link to the research...

[promag]

This PR includes 2 different things: change the default RBF value and change the UI. I was expecting to not see the UI change. I think it is enough to change DEFAULT_WALLET_RBF, update the current checkbox (should be checked) and fix the tests. The UI change could still be done in #11556.

Edit: agree that [qt] prefix doesn't apply.

[promag]

Remove these whitespace changes.

[Sjors]

Alternatively I could leave DEFAULT_WALLET_RBF alone (not touching RPC behavior), add a new DEFAULT_WALLET_UI_RBF and toggle that. In that case the QT label would be correct.

The changes in #11556 require very different wording imo. If you disable a feature by default you need to persuade users why they might want to opt-in to this functionality. You're selling them on the feature. If you enable it by default, then you need to explain why someone might object to it. You might even move the check box somewhere else in the UI, under some "advanced" section.

[meshcollider]

@Sjors no, make test doesn't run the functional python tests I don't think, you have to run test_runner.py for that :)

[jonasschnelli]

General Concept ACK. Most payment providers do wait for confirmations anyways so I think it makes sense now to use BIP125-RBF-signaling as a default. Conforming to an eventually (and unsafe) 0-conf payment receiver, one can then set the RBF disable flag.

Code wise, this is incomplete.
I think introducing a UI based default makes little sense. IMO we should continue this with...

• fix sendtoaddress, sendmany, fundrawtransaction, bumpfee and swap the default there (rename from enable to disable)
• It's an API change, so please add a release notes part

[maflcko]

I am fine if you change the default globally. But then remove the qt tag and add some release notes.

[promag]

(rename from enable to disable)

@jonasschnelli what should be renamed (the RPC option is replaceable)?

It's an API change, so please add a release notes part

Is it an API change because the default value changed? The default value can already be changed with -walletrbf. This also happens with, for instance, -keypool and keypoolrefill RPC. Agree in adding release note but more regarding -walletrbf and the impact in coin control, these RPC and the wallet transactions.

I still think the UI change is independent of this, and subject to other discussion (aim for easier reviews).

[jonasschnelli]

Things like....

\"replaceable\"       (boolean, optional, default true) Whether the new transaction should still be\n"
            "                         marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
            "                         be left unchanged from the original. If false, any input sequence numbers in the\n"
            "                         original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
            "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
            "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
            "                         are replaceable).\n"

This change does also change the default of the RPC layer which may have wide consequences for existing users.
If it's an API change or a local policy change is questionable. However, upgrading can have wide consequences for wallet users.

I think we should always mention to default state (based on -walletrbf?) in the RPC help commands.

[promag]

Yes, that is missing.

[Sjors]

I'm not comfortable (yet) changing code at the RPC level. So what I can do is change this PR to work only at the QT level. Someone else can make a PR that changes it all the way down, which would supersede this. Then you can all choose between #11556, this and that other PR.

[Sjors]

Another point to consider: most users of QT (probably) don't launch it from the command line, so they're unlikely to use -walletrbf.

[Sjors]

I pushed a new version: there's now a new setting DEFAULT_WALLET_QT_RBF which is true. This setting is used by BitcoinQT, unless it's launched with -walletrbf. RPC behavior remains unchanged.

Whitespace is gone now, but I'm still going through the other feedback.

[Sjors]

Rebased. Tests work. Updated screenshots. Explicitly mention BIP-125.

[Sjors]

If we can't separate the behavior, I don't think we should change the default at all, until we understand what the impact is on automated services with large transaction volumes. Or unless we're sure they read the release notes.

[jonasschnelli]

I think if you run large volume business, you either use Qt or much more likely headless/RPC. You certainly will (should!) read default changes in the release notes during a software upgrade.

The split use case would be for someone running both "worlds" (GUI/RPC) with different use cases which I think is unlikely.

[RHavar]

Seems it would make the fees escalate a little less if clients use more conservative estimates.

Yeah, agree. The current situation is absolutely insane (and tragically foreseeable). While obviously not the cause of the clusterfuck we're in, having clients aggressively blindly outbid each other is not helping.

However, I'm still rather skeptical this is anything more than hack. If you want people to low ball fees, and then progressively bump them -- it really should be reliable. In my wallet now I have dozens of transactions that core isn't capable of fee-bumping due to either no change, or the receiver has spent/swept the output.

It would also make sense to build this more first class. e.g. Generate 10 transactions with a progressively increasing nlockTime as well as fee rate. (although this is starting to get a bit off topic)

If we can't separate the behavior, I don't think we should change the default at all, until we understand what the impact is on automated services with large transaction volumes

There's not going to really be any. A year ago or so, it would've been very different when a number of block-explorers and stuff used to reject them (until they confirmed) or show scary warnings. But there's nothing like that today. (Although they still show scary warnings if you actually do a fee bump, warning about double-spends)

[Sjors]

Alright, so we had some more back and forth on IRC. The new version I just pushed:

• drops -guiwalletrbf; the GUI will just use RBF by default
• keeps the original -walletrbf flag, off by default, but now now only impacts RPC

[jonasschnelli]

Nice.
utACK dcd50b90b94fbf2295a122693570218932bf15b4

Consider also updating the original PR description. It will be part of the git history once it's merged.

[Sjors]

@jonasschnelli fixed the PR description.

[laanwj]

utACK dcd50b90b94fbf2295a122693570218932bf15b4

[laanwj]

Travis fails on trailing whitespace :(

diff --git a/doc/release-notes.md b/doc/release-notes.md
@@ -69,0 +70,8 @@ will only create hierarchical deterministic (HD) wallets.
+The send screen now uses BIP-125 RBF by default, regardless of `-walletrbf`.
+There is a checkbox to mark the transaction as final.
^---- failure generated from contrib/devtools/lint-whitespace.sh

[Sjors]

@laanwj fixed

Atom either automatically fixes all whitespace in a file or nothing at all. I haven't been able to tweak it to only fix whitespace in lines that I'm working on, as per the code style guidelines.

Running TRAVIS_COMMIT_RANGE=91eeaa...5cbbbd71 contrib/devtools/lint-whitespace.sh locally is also not super practical, though could probably be automated.

Installing linters helps spotting missing whitespace in time, so that mitigates the issue (I hadn't done that yet on my new machine). Although in the case of linter-markdown, it takes a few additional steps to make it care about whitespace. I might make a PR with some hints later.

[Sjors]

Travis still unhappy, not sure why:

[laanwj]

Seems Travis is happy now, probably a transient issue.

[laanwj]

Tested ACK 5cbbbd7.