[Wallet] Enable RBF by default in QT

Sjors commented at 1:00 pm on November 4, 2017: member

~~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:

fanquake added the label GUI on Nov 4, 2017

Sjors commented at 2:01 pm on November 4, 2017: member

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

TheBlueMatt commented at 5:52 pm on November 4, 2017: member

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

Sjors commented at 7:53 pm on November 4, 2017: member

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

TheBlueMatt commented at 0:46 am on November 5, 2017: member

listtransactions fails for me locally, too.

meshcollider commented at 2:32 am on November 5, 2017: contributor

@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: https://github.com/bitcoin/bitcoin/blob/2f959a58744d42859d74579220922e25ac3d2925/test/functional/listtransactions.py#L117

in src/qt/sendcoinsdialog.cpp:351 in be758fe027 outdated

350-        questionString.append(tr("This transaction signals replaceability (optin-RBF)."));
351-        questionString.append("</span>");
352+      questionString.append(tr("You can increase the fee later (signals Replace-By-Fee)."));
353     }
354+    else
355+    {

meshcollider commented at 2:47 am on November 5, 2017:

Nit, braces on same line as if and else

in src/qt/sendcoinsdialog.cpp:348 in be758fe027 outdated

347+    if (!ui->disableRBF->isChecked())
348     {
349-        questionString.append("<hr /><span>");
350-        questionString.append(tr("This transaction signals replaceability (optin-RBF)."));
351-        questionString.append("</span>");
352+      questionString.append(tr("You can increase the fee later (signals Replace-By-Fee)."));

meshcollider commented at 2:47 am on November 5, 2017:

Nit, indentation Also perhaps add reference to BIP 125 again

Sjors commented at 5:31 am on November 5, 2017:

Will do.

meshcollider commented at 2:49 am on November 5, 2017: contributor

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

MarcoFalke commented at 4:26 am on November 5, 2017: member

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

MarcoFalke commented at 4:29 am on November 5, 2017: member

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 commented at 5:32 am on November 5, 2017: member

@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 commented at 5:51 am on November 5, 2017: member

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

in src/wallet/wallet.h:259 in be758fe027 outdated

261@@ -262,7 +262,7 @@ class CMerkleTx
262     bool IsCoinBase() const { return tx->IsCoinBase(); }
263 };
264 
265-/**

promag commented at 10:51 am on November 5, 2017:

Remove these whitespace changes.

promag commented at 10:56 am on November 5, 2017: member

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.

Sjors commented at 11:09 am on November 5, 2017: member

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 commented at 11:47 am on November 5, 2017: contributor

@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 commented at 10:18 pm on November 5, 2017: contributor

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

MarcoFalke commented at 10:37 pm on November 5, 2017: member

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

promag commented at 11:37 pm on November 5, 2017: member

(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 commented at 2:31 am on November 6, 2017: contributor

Things like….

0\"replaceable\"       (boolean, optional, default true) Whether the new transaction should still be\n"
1            "                         marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
2            "                         be left unchanged from the original. If false, any input sequence numbers in the\n"
3            "                         original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
4            "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
5            "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
6            "                         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 commented at 2:41 am on November 6, 2017: member

Yes, that is missing.

Sjors commented at 8:15 am on November 6, 2017: member

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 commented at 8:16 am on November 6, 2017: member

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 force-pushed on Nov 6, 2017

Sjors renamed this:
~~[Qt] Enable RBF by default~~
[Wallet] Enable RBF by default in QT
on Nov 6, 2017

Sjors commented at 12:04 pm on November 6, 2017: member

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 force-pushed on Nov 6, 2017

Sjors commented at 1:17 pm on November 6, 2017: member

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

Sjors force-pushed on Nov 6, 2017

ryanofsky commented at 4:15 pm on November 6, 2017: member

Haven’t really looked at this yet, but there was a previous PR to try to switch the rbf default (for both qt and rpc) in #9527, and some of that discussion might apply here.

Sjors commented at 4:48 pm on November 6, 2017: member

@ryanofsky that previous PR turned on RBF by default for both the wallet and RPC.

A number of objections were raised. I’ll address the ones that I think are relevant: @luke-jr (and @instagibbs):

Probably shouldn’t be merged until something can actually use it (preferably from the GUI).

That’s the case now. @RHavar:

I was supportive of bip125 because it was marketed, sold and even titled: “Opt-in”. But this simple change in defaults really makes it “Opt-out” and will end up causing huge impacts to people who are currently relying on 0-conf transactions, as suddenly everyone is going to be accidentally sending “opt-in” RBF.

I suppose that’s unchanged. I think opt-in refers to the fact that it’s a soft fork? I think the best way to counter bad press is to make the feature work great and have users and (most) merchants love it.

There were some issues with usability at the time, which I’m guessing have been addressed since?

There was also discussion about RBF vs. CPFP. Given that the RBF check box made it into the UI, I don’t think that’s relevant (at least not for the scope of this issue). Although perhaps we could warn users that using RBF breaks the ability of CPFP for the recipient (does it?). I could imagine a merchant using CPFP for large purchases as a customer service gesture. @TheBlueMatt:

Obviously I’d agree we shouldn’t do this unless bumpfee goes in.

That’s there now. @gmaxwell:

As far as your zero-conf comments, that is why my comment here and on the bumpfee PR talk about being able to after-the-fact turn it off.

Is this currently possible? If not, should we wait for that? @RHavar:

If this is going to be the default, it seems that bumpfee first at least be able to handle all transactions (e.g. ones without change)?

I don’t understand RBF really well, but if transactions without change can’t use RBF, it would be good UX to grey-out and the checkbox for those transactions (and set it to disabled). We don’t want the wallet to throw errors with default settings. Does it mean you can’t send the transaction at all, or just not bump it? Both are problematic though.

Likewise it would also make sense to have “reverttransaction” as a prerequisite too. It would help people fully realize how revertible bip125 transactions are

I’d like that feature too, but I think it can be added later.

By the way, the bump fee feature is pretty well hidden in the UI, but that’s another issue…

Some original objections might still hold since @laanwj closed it, but others have been addressed since early this year. So it seems useful to discuss this again

in src/wallet/wallet.h:66 in 58250a5567 outdated

63@@ -63,6 +64,7 @@ static const bool DEFAULT_WALLET_REJECT_LONG_CHAINS = false;
64 static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6;
65 //! -walletrbf default
66 static const bool DEFAULT_WALLET_RBF = false;
67+static const bool DEFAULT_WALLET_QT_RBF = true;

MarcoFalke commented at 6:12 pm on November 7, 2017:

Eh. I guess we don’t want Qt code to end up here

Sjors commented at 6:43 pm on November 7, 2017:

I’m not sure how to disentangle this. The -walletrbf is cast to a boolean in init.cpp, so QT can’t tell if that flag was present or not.

laanwj commented at 9:49 am on December 21, 2017:

What about a separate option for the GUI, GUI-only (so documented in utilitydialog.cpp instead of init.cpp), that defaults to -walletrbf? This would allow it to be completely disentangled from the core.

in src/qt/walletmodel.cpp:755 in 58250a5567 outdated

753@@ -754,5 +754,5 @@ int WalletModel::getDefaultConfirmTarget() const
754 
755 bool WalletModel::getDefaultWalletRbf() const

MarcoFalke commented at 6:13 pm on November 7, 2017:

No need for this wrapper if the member is qt-only

MarcoFalke commented at 6:13 pm on November 7, 2017: member

utACK 58250a5567e8d99cd00321a2c33b30d141b2935d

RHavar commented at 6:17 pm on November 7, 2017: contributor

I don’t understand RBF really well, but if transactions without change can’t use RBF, it would be good UX to grey-out and the checkbox for those transactions (and set it to disabled).

That’s pretty fragile and complex. You need to run coin-selection to know if the transaction is going to have no change or not. I strongly suspect it’ll be easier to just fix the cases of bumping transactions without change

MarcoFalke commented at 6:20 pm on November 7, 2017: member

fix the cases of bumping transactions without change

Should be a separate pull, though.

instagibbs commented at 6:20 pm on November 7, 2017: member

@RHavar I think he’s referring to the “bump fee” button itself, not the “enable bip125” button?

RHavar commented at 6:50 pm on November 7, 2017: contributor

@instagibbs he did say “checkbox” though, which makes it sound like it’s the original transaction (and from a users point of view it doesn’t really make sense to use bip125 for a transaction that you can’t fee bump anyway). But a setting “enable bip125 (if we have change)” is the logical behavior, but annoying to explain.

–

All that said, I’m still very against this change. The reiterate my objections:

Bip125 was explicitly sold as “opt-in” to turn it to “opt-out” is rather distasteful, and has a lot of externalities.

I would drop this objection if there was a significant (>5%) of hash power that was maliciously mining 0-conf double spends. But there simply isn’t.

My latest project has in the last weeks processed 279 transactions worth 339 BTC with 0-confirmations transaction (after passing a few safety checks) without a single successful double-spend. I’m aware it’s probably going to break horribly in the future, but it would be appreciate if yall didn’t go out of your way to break shit that works and gives users a good experience.

And my other objection is, that core users can’t even take full advantage of bip125. Unless there’s an exposed simple “bump fee” (that works reliably) and “revert transaction”. It seems totally premature to expose users (and the rest of the bitcoin ecosystem) to all the disadvantage of bip125, without at the very least allowing users fully take advantage of it.

Sjors commented at 7:08 pm on November 7, 2017: member

@RHavar I’m still trying to understand why RBF would break your application. If you trust customers to not double spend coins, why would they behave any different with RBF enabled? Just because it’s marginally easier to cheat?

The bump fee feature is there. Just a bit hard to find. You need to right click on the question mark next to the transaction.

Revert transaction would be the kind of feature I would understand merchants not being excited about.

RHavar commented at 7:30 pm on November 7, 2017: contributor

If you trust customers to not double spend coins

I don’t trust them. I don’t have any of their details, and if they could successfully double spend even 2% of the time, they could turn a profit. However, they would require the assistance of a malicious miner (who could do it costlessly). However no miners are doing it at the moment (and I hope this stays for a long time; even though it probably won’t).

The bump fee feature is there. Just a bit hard to find. You need to right click on the question mark next to the transaction.

It doesn’t work reliably though (e.g. the case where there’s no change), which was my point.

Revert transaction would be the kind of feature I would understand merchants not being excited about.

Why would a merchant care? If a transaction is bip125, any sane merchant is going to require at least 1 conf (and likely +1 conf than usual) anyway. If the transaction is bip125, it’s trivial for a technical user (AKA the type doing double spend attacks) to do anyway, exposing it in the UI changes nothing (and in fact, helps educate users of the dangers of accepting bip125 transactions without confs)

Sjors commented at 7:35 pm on November 7, 2017: member

So your concern is that RBF makes it easier to double spend? But the feature is already there. I would expect a customer with malicious intent to just tick the box. Or are you worried about a more opportunistic thief; someone who didn’t realize this was possible, but because they see the bump-fee feature, suddenly they start reading up on BIP-125, spin up the RPC client and redirect their transaction?

RHavar commented at 7:55 pm on November 7, 2017: contributor

@Sjors bip125 is detectible by merchants, this doesn’t change their risk profile. Merchants need to treat bip125 transactions differently and trust them less (e.g. require an extra conf) regardless. By changing this to the default, you just needlessly break smooth workflows for users.

There’s so many more constructive ways to help users experience in the era of crazy fees (like fix fee calculations when spending unconfirmed inputs, so CPFP actually works) or finish the bumpfee implementation, that I can’t help but feel this feels just like vandalism (although I’m not questioning your motive, just the end result).

Sjors commented at 8:09 pm on November 7, 2017: member

Users can’t use bump fee if they didn’t tick the box. I suspect they won’t even know that they could have used RBF until they experience a stuck transaction for the first time. The wallet recommends lower fees when RBF is enabled, but again, users don’t know that and so they pay higher fees despite the RBF feature being available. That drives the fee up for everyone (not if QT is only a small fraction of users, but larger wallets might be reluctant to support RBF if even Core doesn’t have it on by default).

What is the issue with the bump fee implementation you’re referring to?

It seems the trade-off here is such that users either get a bad experience because their transaction is stuck for ages, or a bad experience because their merchant has to wait for confirmations. I don’t know which one is more common, so I don’t know what the average impact is. In terms of worst case, having to wait a few blocks is far better than having to wait several days. So this may be a situation of slightly worse average case, but significantly less bad worst case.

CPFP seems of out of scope, but it’s also quite expensive; unless you needed to move the received funds anyway, you end up paying for two transactions rather than just one.

RHavar commented at 9:01 pm on November 7, 2017: contributor

Users can’t use bump fee if they didn’t tick the box. I suspect they won’t even know that they could have used RBF until they experience a stuck transaction for the first time.

I think this is more of an argument for supporting a sort of limited RBF in all cases as long as all output amounts are unchanged or increased.

users don’t know that and so they pay higher fees

I think this is more of a UI issue. When showing a fee it could give a prediction for different percentiles of probability and explain the transaction is final or not.

What is the issue with the bump fee implementation you’re referring to?

You can’t bumpfee on a transaction that doesn’t have change (this is just a pure implementation limitation) or a transaction in which you or the receiver has spent (which makes sense, there’s a lot complexity here)

CPFP seems of out of scope

Why? It’s often a good alternative to what you are suggesting. Especially if you don’t want to (or can’t) break a transaction chain (e.g. the receiver has already spent your funds, but it hasn’t confirmed)

but it’s also quite expensive; unless you needed to move the received funds anyway

The sender can do it too if there’s change. Core’s coinselection is sufficiently unoptimized that forcing the use of particular inputs has almost always 0 cost. But yeah, you generally need to be wanting to make a new transaction anyway for it to be cost effective.

petertodd commented at 2:56 pm on November 8, 2017: contributor

Re: @RHavar

“I don’t have any of their details, and if they could successfully double spend even 2% of the time, they could turn a profit. However, they would require the assistance of a malicious miner (who could do it costlessly). However no miners are doing it at the moment (and I hope this stays for a long time; even though it probably won’t).”

Not sure what exactly your service actually is, but not only do more than 2% of miners run full-RBF - and thus will replace a transaction even if it hasn’t set the opt-in RBF bit - a significant chunk of the ones who don’t have sufficiently non-standard “IsStandard()” rules that they might as well be running full-RBF.

petertodd commented at 2:56 pm on November 8, 2017: contributor

Concept ACK

RHavar commented at 3:02 pm on November 8, 2017: contributor

Not sure what exactly your service actually is, but not only do more than 2% of miners run full-RBF - and thus will replace a transaction even if it hasn’t set the opt-in RBF bit

I haven’t observed this at all. What miners or mining pools are doing this? Are you sure you’re not confusing a mining pool that uses a reasonably small mempool with full-RBF?

I did see some full-RBF behavior a while back, I think it was a year or so (?) ago (going by memory) but it was very short-lived (like for 2 or 3 days max)

If you are seeing full-RBF behavior, could you provide me with two txid’s that had this in the last 6 months. (and something where the relay times were more than 30 seconds apart, so they’re not races). And I’ll dig through my logs and see what I observed

a significant chunk of the ones who don’t have sufficiently non-standard “IsStandard()” rules that they might as well be running full-RBF

Yeah, I’ve noticed this. But it’s something that that I haven’t had too much trouble accounting for

petertodd commented at 3:13 pm on November 8, 2017: contributor

@RHavar Are you running a full-RBF node? E.g. https://github.com/petertodd/bitcoin/tree/replace-by-fee-v0.15.0.1 Quite possible that you’re just not noticing it.

I say they’re running full-RBF, not because I’ve personally witnessed it, but rather based on the pools that have reached out to me and told me they’re running it (including running my full-rbf fork).

Yeah, I’ve noticed this. But it’s something that that I haven’t had too much trouble accounting for

How do you account for it?

instagibbs commented at 3:14 pm on November 8, 2017: member

It seems the trade-off here is such that users either get a bad experience because their transaction is stuck for ages, or a bad experience because their merchant has to wait for confirmations.

Agreed. We should just document this well so merchants can tell their users why their deposits are delayed, and how to uncheck it if they want. Being stuck for literal weeks with no recourse(other than “run Peter Todd’s full rbf node/tools”) is worse in my estimation

concept ACK

RHavar commented at 3:22 pm on November 8, 2017: contributor

Quite possible that you’re just not noticing it.

I don’t run a full-rbf node, but I don’t really need to. While I do often see full-rbf attempts, I’m only concerned about ones that actually confirm in a block (and aren’t the result of a race, or getting dropped by a small mempool)

I say they’re running full-RBF, not because I’ve personally witnessed it

Well I’m actually actively monitoring for it, and not seeing it. If it’s happening, it should be immediately obvious and scammers will be quick to take advantage of it. In fact @petertodd you have my full permission to take advantage of up to 0.3 BTC by abusing 0-confs on www.bustadice.com . You can consider it a bounty =) All you need to do is have a success rate of ~2% to be able to turn a profit.

In the mean time, I think we shouldn’t make decisions based on hearsay, when if it was happening it would be quite apparent and demonstrable.

And I understanding braking this flow makes side-chain/offchain/centralizated-services/etc more attractive; but I really would greatly appreciated if you could hold off.

Sjors commented at 3:27 pm on November 8, 2017: member

@petertodd @RHavar this experiment would be interesting to watch, but doesn’t tell us much about the actual occurrence. @RHavar not having RBF by default increases fees and long term stuck transactions for the entire network, yet only benefits 0-conf services (marginally, because you can just ask users not to use that feature). It’s not fair to only focus on second layer stuff.

RHavar commented at 3:34 pm on November 8, 2017: contributor

Being stuck for literal weeks with no recourse(other than “run Peter Todd’s full rbf node/tools”) is worse in my estimation

That’s a bit stretched, if there’s change they can spend form it to prioritize the transaction. If there’s no change, bumpfee doesn’t even currently work.

but doesn’t tell us much about the actual occurrence.

Well if he can profitably take advantage of it, it means miners are doing full-RBF more than 2% of the time – and I will concede that 0-conf is already sufficiently broken that it’s not worth preserving.

not having RBF by default increases fees for the entire network, yet only benefits 0-conf services.

There’s certainly a huge amount of potential benefits of a wallet that does full RBF. I think a great example of that would be if your wallet generated ~10 or so transactions, each with a higher fee (and higher nLockn time) according to a schedule so it could automatically bump transactions until they confirm, ensuring you can pay the least possible fees.

Same thing about reliable feebumping, and reverting transactions.

If these benefits were actually realized, I think it would might be appropriate to make replaceability opt-out. Right now it’s like shooting your horse because in 10 years cars will replace them.

Sjors commented at 3:43 pm on November 8, 2017: member

I think a great example of that would be if your wallet generated ~10 or so transactions, each with a higher fee (and higher nLockn time) according to a schedule so it could automatically bump transactions until they confirm, ensuring you can pay the least possible fees.

Yes, this would be very cool. I don’t know if it requires keeping your node running for the full duration, but at least the signing could be done in one go. Perhaps a first step woud be an RPC command like “fee_burst_send [amount] [max fee] [deadline]”.

instagibbs commented at 3:48 pm on November 8, 2017: member

There is no currently coded “auto-CPFP” currently, and debate on how to do that as a feature. There are very manual ways of doing this, sure.

The only persuasive argument here is that some users depositing in 0-conf second layer systems will have to wait(and only if they have no change since they can rbf and unmark it). Malicious users can’t even double-spend using this without additional tooling.

I think it’s a bad tradeoff. but I’ll let others weigh in instead.

Sjors commented at 3:53 pm on November 8, 2017: member

@instagibbs are there second layer proposals where using anything less than 6 confirmations is considered even remotely secure? I think @RHavar reference to second layer solutions referred to a preference for using second layer systems themselves for fast transactions, rather than supporting fast transaction on the layer 1 (using zero conf without opt-in RBF).

instagibbs commented at 3:54 pm on November 8, 2017: member

@Sjors for better or for worse, many exchanges take <6 confirms, and some services take 0-conf. I do not begrudge this.

I was using “second layer systems” as anything built on top, including Bitcoin casinos.

Sjors commented at 3:57 pm on November 8, 2017: member

@instagibbs I’ll let @RHavar clarify, but I would suggest calling things like Lightning “second layer”, while calling exchanges and casinos “off-chain” (or something else), just for clarity.

RHavar commented at 4:00 pm on November 8, 2017: contributor

I use the same definition as @instagibbs which I think is pretty universal. I think him and I agree about everything except which trade-off is worth making.

are there second layer proposals where using anything less than 6 confirmations is considered even remotely secure?

Sure. Why not? 6 isn’t some magical number. It’s up to everyone to decide what risk tolerance to accept (which will greatly depend on how miners are behaving). If a transaction is replaceable, it’s pretty rational to require +1 confirmations than you otherwise would. FWIW I’ve processed 60991 BTC of anonymous deposits at 1 conf, and never had any issues.

If miners were being real dicks, then even 6 confirmations is pretty damn weak. But it is so far my estimation that miners have thus-far acted incredibly honestly and avoided engaging in any nonsense like this.

If you @Sjors think you can abuse this, you have my full permission to abuse up to 20 bitcoin in a 1-conf double-spend against bustadice.com (and consider it a bounty) =)

There is no currently coded “auto-CPFP” currently, and debate on how to do that as a feature. There are very manual ways of doing this, sure.

Sure, and also the addition of “first seen safe RBF” is something I’d get behind too. 99% of the time this is just as efficient as full RBF (assuming you don’t do off-peak consolidation, and need to eventually consolidate your unspent anyway)

That said, I’m not volunteering to do any of the work. So I won’t complain too loudly if this commit is merged. I just would really hate to see breaking stuff that already works.

Sjors commented at 4:23 pm on November 8, 2017: member

are there second layer proposals where using anything less than 6 confirmations is considered even remotely secure?

I use the same definition as @instagibbs which I think is pretty universal. I think him and I agree about everything except which trade-off is worth making.

Using the same term for things like Lightning and custodial services makes no sense to me. They have completely different threat models.

And I understanding braking this flow makes side-chain/offchain/centralizated-services/etc more attractive; but I really would greatly appreciated if you could hold off.

That’s the remark I was referring to when I asked my question about 6 confirmations. Custodial services can certainly do 0 conf, but my question is if non-custodial second layer solutions like side-chains or lightning do.

Sjors commented at 4:29 pm on November 8, 2017: member

@RHavar I’m also not allowed to use your website where I live, due to some effective lobbying by a state owned corporation with a monopoly on gambling. :-)

RHavar commented at 4:39 pm on November 8, 2017: contributor

Using the same term for things like Lightning and custodial services makes no sense to me.

You’re free to use a more specific term

That’s the remark I was referring to when I asked my question about 6 confirmations. Custodial services can certainly do 0 conf, but my question is if non-custodial second layer solutions like side-chains or lightning do.

I’m not sure it’s a distinction worth making. There’s no universal safe or unsafe. I’d be happy right now to accept a payment from a payment channel where the source has a single confirmation (and even 0, if everything looked sane). Other people might want 6. If a sizable amount of miners were acting adversarially, I wouldn’t touch anything with out a hell of a lot more than that.

Being custodial or not doesn’t really make much of a difference, AFAICT

petertodd commented at 5:00 pm on November 8, 2017: contributor

@RHavar Looks like your site is down.

RHavar commented at 5:06 pm on November 8, 2017: contributor

@RHavar Looks like your site is down.

Edit: I’m an idiot. @petertodd the link was wrong. Should’ve been: https://www.bustadice.com

laanwj commented at 11:47 am on November 9, 2017: member

Changing defaults always gives so much discussion, wasted IMO.

Sjors commented at 12:06 pm on November 9, 2017: member

@laanwj if there’s no agreement on this PR, I can point that out in the #11556 discussion. I’ll still try to address the remaining code quality issues.

laanwj commented at 1:21 pm on November 9, 2017: member

Yes, good idea to leave the controversial part in a separate PR at least :)

jonasschnelli commented at 11:31 pm on November 29, 2017: contributor

What’s the state of this PR? I think doing it GUI only makes sense for now.

Code-wise, some things are unaddressed:

Move code away from core classes. I think the only change you want to have in the non GUI files is that help line for -walletrbf (we should mention that the default value for QT is 1).
Try to set -walletrbf=1 in src/qt/bitcoin.cpp (GuiMain) or in createOptionsModel() in case it’s unset.

Sjors force-pushed on Nov 30, 2017

Sjors commented at 12:07 pm on November 30, 2017: member

I pushed a new version. This led to a conflict, so I also rebased.

QT now processes -walletrbf, so I no longer need to change the non-GUI files. The exception is DEFAULT_WALLET_QT_RBF, which is needed by wallet/init.cpp to print the help message. There might be a convoluted way to avoid that, by using HelpMessageMode=HMM_BITCOIN_QT to suppress -walletrbf and instead show this flag under UI Options.
I’m not sure if qt/sendcoinsdialog.cpp is the right place for gArgs.GetBoolArg("-walletrbf", DEFAULT_WALLET_QT_RBF), but that’s the same pattern qt/guiutil.cpp and qt/intro.cpp use.
- @jonasschnelli did you mean BitcoinApplication instead of GuiMain? I’m not sure where that would go, and how to then access that from sendcoinsdialog (bitcoin.cpp doesn’t have a header file).
- I looked into OptionsModel, but as far as I understand what that class is doing, it seems more suitable for global settings that can be toggled. E.g. the display currency can be toggled by the user through a setting, but there’s no setting for the user to enable whether or not the RBF checkbox should be on by default. I could make it remember the last choice, but that would increase the scope of this PR.
@MarcoFalke I removed getDefaultWalletRbf from WalletModel
Added a release note.

Sjors force-pushed on Nov 30, 2017

in src/qt/sendcoinsdialog.cpp:188 in 90244ed4ad outdated

186         updateMinFeeLabel();
187         updateSmartFeeLabel();
188 
189         // set default rbf checkbox state
190-        ui->optInRBF->setCheckState(model->getDefaultWalletRbf() ? Qt::Checked : Qt::Unchecked);
191+        const bool fWalletQtRbf = gArgs.GetBoolArg("-walletrbf", DEFAULT_WALLET_QT_RBF);

TheBlueMatt commented at 8:37 pm on December 4, 2017:

I believe we cache a bunch of the states of other UI elements in the send dialog (eg feerate selectiton), is it worth doing something similar here?

Sjors commented at 9:00 pm on December 4, 2017:

Do you mean remembering what the user selected before? Does that use the OptionsModel?

TheBlueMatt commented at 9:34 pm on December 4, 2017:

I believe it uses the OptionsModel, yes.

TheBlueMatt commented at 8:38 pm on December 4, 2017: member

Concept ACK changing the Qt default - the usability improvement from providing RBF is very significant, and the only users who may be adversely affected are those paying to services that take 0conf, which appear to be ever fewer (with the exception of in-person shops, which likely will take it either way).

RHavar commented at 9:12 pm on December 4, 2017: contributor

and the only users who may be adversely affected are those paying to services that take 0conf, which appear to be ever fewer

This is simply not true, and hugely dismissive of the real impacts this change will cause.

Just today I sent an organization $10k USD by paying them through coinbase. Coinbase guaranteed the exchange rate as long as they saw the transaction on the network with 15 minutes (even if it takes a few hours to confirm). It’s a great user experience for all involved (I know exactly how much bitcoin to send) and the seller knows exactly how much USD they will get (after it’s all confirmed, and what not) and coinbase gets their cut.

Now if you throw in replaceable transactions, everything falls apart. I can trivially revert the transaction if the exchange rate moves in a way beneficial for me (and even tell the vendor I will resend the funds as a diff transaction).

I mean, the way things are going eventually bitcoin is going to be a pure settlement layer and none of this will really matter. But it’s not nearly that yet, so I just don’t get where all this motivation to vandalize bitcoin payments is coming from.

The concern is ostensibly for user-experience, so while I realize time and energy isn’t fungible – but it would be so much better spent on improving other things (like CPFP workflow, and FSS-RBF and things without such negative effects)

TheBlueMatt commented at 9:34 pm on December 4, 2017: member

@RHavar Sadly, neither CPFP nor FSS-RBF are realistic replacements (CPFP has real additional cost, and FSS-RBF, at least as originally proposed, is highly restrictive on types of transactions that can be replaced). Given the community’s seeming unwillingness to actually work together, those targeting very short confirmation windows almost certainly don’t have any options except RBF right now that have reasonable usability. There may be cases where not using RBF is preferable, and I’ve suggested many times that people who want that put something like no125=1 in their URIs do so, and we should honor such things, but I dont see a mad rush to do that, plus users can easily still turn it off. But for average payments (the vast majority of which require some confirmations either way), RBF is such a better user experience that having it off by default just causes issues for users.

If something like this gets turned on more commonly, and major providers see an issue, I’m sure they’d be happy to look into URI options to inform clients to turn it off. But, in general, my daily wallet sets RBF off for all payments and all the exchange-rate-guarantee folks seem to have no problem taking my transactions, I assume because the exchange-rate-movement issues just dont appear in the wild (and if they did and they needed something immediately, I’m sure there are lots of folks who’d happily offer short-term options contracts to them at scale for a fraction of the fee someone like Coinbase takes in on payments).

petertodd commented at 9:41 pm on December 4, 2017: contributor

@RHavar In practice, Coinbase appears to accept opt-in RBF payments just fine for the times I’ve used them recently (buying hotel/plane tickets via CheapAir, and five minutes ago to buy some Reddit Gold as a test). They of course are just one of many reasons why people send Bitcoin; even if Coinbase has the behavior you mention - which doesn’t appear to be true - most Bitcoin exchanges won’t even consider a deposit until it has confirmed under any circumstance, making the point irrelevant with them.

Bitcoin Core is used by a relatively technical audience, so I see no harm in some users possibly wanting to untick a checkmark a minority of the time.

Sjors force-pushed on Dec 5, 2017

Sjors commented at 8:58 am on December 5, 2017: member

Rebased after #11556 was merged. @TheBlueMatt I created a placeholder issue for your no125=1 suggestion.

I’ll look into using OptionsModel to remember the users previous choice. Not sure if that should be blocking though.

Sjors commented at 1:40 pm on December 5, 2017: member

Travis fails, but test/functional/zapwallettxes.py passes for me locally.

If the user selects “Subtract fee from amount”, this selection is not remembered for the next transaction. Same goes for selecting e.g. mBTC instead of BTC. It seems that OptionsModel is (mostly) for settings that can be changed through the Options dialog. E.g you can change the default display currency in that dialog and that setting is not affected by selecting a different currency for a specific transaction.

So I don’t think we should remember the users previous choice for using RBF.

TheBlueMatt commented at 4:29 pm on December 5, 2017: member

Kicked travis as it seems almost certainly unrelated, but #11831 should fix the race, I believe.

in src/qt/forms/sendcoinsdialog.ui:1114 in c463aa366f outdated

1112-              <string>Allow increasing fee</string>
1113+              <string>Disable Replace-By-Fee</string>
1114              </property>
1115              <property name="toolTip">
1116-              <string>This allows you to increase the fee later if the transaction takes a long time to confirm. This will also cause the recommended fee to be lower. ("Replace-By-Fee", BIP 125)</string>
1117+              <string>Not using Replace-By-Fee (BIP-125) marks the transaction as final. This will also cause the recommended fee to be higher.</string>

TheBlueMatt commented at 4:38 pm on December 5, 2017:

Heh, I’m less a fan of the copy here now. Maybe “Not using Replace-By-Fee (BIP-125) prevents you from increasing a transaction’s fee after it is sent. This may cause the recommended fee to be higher to compensate for the increased risk a transaction is delayed.” or something like that?

Sjors commented at 6:01 pm on December 5, 2017:

I changed it to:

Without Replace-By-Fee (BIP-125) you can’t increase a transaction’s fee after it is sent. A higher fee may be recommended to compensate for increased transaction delay risk.

Updated screenshot above.

Sjors force-pushed on Dec 5, 2017

morcos commented at 2:32 pm on December 11, 2017: member

Sorry for chiming in late, but a couple of comments:

Concept ACK. I’m strongly in favor of making RBF default to on, I realize there are some downsides, but I think they are significantly outweighed by the fact that accurate fee estimation is essentially an impossible task and being able to adjust fee afterwards is the single biggest usability improvement we can offer now.
I think we should flip RPC and QT at the same time. I recognize there are some small risks with flipping the RPC default, but that can just be release noted. Separating them is just introducing unnecessary configuration complexity. It will also be confusing to users that use RPC while running QT and have their transactions go out differently. Fundamentally the easiest and best thing to do is just change DEFAULT_WALLET_RBF to true.
I question flipping the meaning of the checkbox, I realize it’s kind of silly for it to be always defaulted to checked, but are we not worried some people will just be used to expecting the checkbox to be checked for RBF? I’m fine being outvoted on this aspect though if we think its short term pain for long term gain.

Sjors commented at 2:59 pm on December 11, 2017: member

@RHavar can you add your five cents to the bitcoin-dev list discussion about a -no150 flag? In particular regarding @luke-jr’s point. @MarcoFalke:

2 - The additional complexity is only three lines of code. It would seem trivial to switch the RPC default over one version later, when people are more familiar with it. The 0.16 release notes (assuming this makes it in) could warn that that’s coming in the next version.

Regarding:

confusing to users that use RPC while running QT and have their transactions go out differently":

Assuming you mean “differently then when they run RPC without QT”: that shouldn’t be the case. I don’t think users should expect the RPC (inside QT or otherwise) to behave like the UI in every way.

3 - I’m fine with changing the word “disable” to “enable” in the checkbox and checking it by default. That wouldn’t require changing any other copy, not even the tooltip. Any objections?

jonasschnelli commented at 7:23 pm on December 11, 2017: contributor

I kinda have to agree with @morcos. My first review also mentioned that we shouldn’t to a distinction between RPC/GUI (https://github.com/bitcoin/bitcoin/pull/11605#issuecomment-342010545).

I’m fine with both, but would prefer a complete default switchover.

laanwj referenced this in commit d48ab83f00 on Dec 12, 2017

RHavar commented at 4:18 pm on December 15, 2017: contributor

Regrettably, I’m going to drop my objection to this change. Bitcoin as a payment network is pretty much dead (congratulations boys!) so there is becoming little point in trying to protect 0-confs. Low-balling fees and fee-bumping will lead to a healthier fee market, so might as well do it.

It should also be imho considered a very high priority to fix bumpfee to handle the case without change. (Contrary to popular belief this isn’t the edge case, but rather the norm (~80%) when simulating the transaction load a bitcoin service gets when using non-naive (read: proper constraint optimizer) input (coin) and output (which of the batch to send) selection ).

Sjors commented at 4:57 pm on December 15, 2017: member

@RHavar thanks. Among my ever growing todo list is to make the bump fee functionality better, especially in the UI. There was some recent IRC discussion about this as well.

I’m still reluctant about changing the RPC default, which we also chatted about. Once thing I’d like to (see someone else) do is go through popular wallets and services to see how they treat RBF payments and how they treat bumped fees. When it’s a relatively small user base, Bitcoin Core QT users, that’s one thing. At least the sender probably knows a bit more than average about Bitcoin and can community directly with the recipient. But when every Bitcoin ATM’s or other automated process that runs Core starts doing this there’s a problem.

in src/wallet/init.cpp:34 in 6bd1224099 outdated

30@@ -31,7 +31,7 @@ std::string GetWalletHelpString(bool showDebug)
31     strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet on startup"));
32     strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
33     strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
34-    strUsage += HelpMessageOpt("-walletrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_WALLET_RBF));
35+    strUsage += HelpMessageOpt("-walletrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u for RPC, %u for QT)"), DEFAULT_WALLET_RBF, DEFAULT_WALLET_QT_RBF));

laanwj commented at 9:30 am on December 21, 2017:

Please use “GUI” not “QT” when referring to the GUI in messages. It’s irrelevant what widget toolkit is used.

Sjors commented at 9:36 am on December 21, 2017:

Will fix.

laanwj commented at 9:42 am on December 21, 2017: member

I’m starting to warm up to the idea of changing this default. Seems it would make the fees escalate a little less if clients use more conservative estimates.

Sjors force-pushed on Dec 21, 2017

Sjors commented at 10:33 am on December 21, 2017: member

I flipped the checkbox: it’s now checked and says “Enabled”. I adjusted the tooltip to reflect that a bit; it explains why RBF is useful and warns of a least one potential downside of disabling it (higher fees). This passes the strobe test better (removes a bunch of double negatives). See new screenshots.

I renamed the variable enableRBF to enableRBF. @laanwj suggested:

What about a separate option for the GUI, GUI-only (so documented in utilitydialog.cpp instead of init.cpp), that defaults to -walletrbf? This would allow it to be completely disentangled from the core.

E.g. -guiwalletrbf? I’m worried that would just make it more confusing, unless we also rename -walletrbf to -rpcwalletrbf.

I’m generally in favor of treating the RPC and GUI as different products, since they target very different users. The only factor that should constrain that is code complexity. From that perspective it would make sense to have different defaults and use -walletX vs. -rpcX settings.

I can’t think of many examples outside RBF where this would make sense. Only memory usage comes to mind; a GUI could use a lot more than a daemon, since people either don’t leave it open all the time, or at least are used to quitting applications when their computer gets too slow.

It also seems quite easy to add -guiwalletrbf at a later stage and undo the entanglement.

laanwj commented at 10:38 am on December 21, 2017: member

E.g. -guiwalletrbf? I’m worried that would just make it more confusing, unless we also rename -walletrbf to -rpcwalletrbf.

The difference is that here, it’s effectively two different things already. One changes the default in the GUI, one changes the default that’s used with RPC. There is no overlap in code between what the options do.

I really dislike the (default: %u for RPC, %u for GUI) help message because it leaves one thing open: what if you use the GUI, but type commands in the debug console?

And I agree with regard to -rpcwalletrbf but it’s too late for that.

At least the GUI option could be a nice setting in the options dialog. GUI users tend to prefer that to adding things to bitcoin.conf.

Sjors commented at 10:46 am on December 21, 2017: member

Perhaps we can add -rpcwalletrbf as an alias for -walletrbf and deprecate the latter (with zero rush). I could then replace (default: %u for RPC, %u for GUI) with a warning that this setting applies to RPC (and debug console) only. And then add -guiwalletrbf.

Sjors force-pushed on Dec 21, 2017

Sjors commented at 11:52 am on December 21, 2017: member

I implemented -guiwalletrbf, added -rpcwalletrbf and a deprecation warning for -walletrbf. I added [RPC] to the commit name. This issue probably needs an RPC tag now.

RPC and debug console won’t use RBF by default and honor the -rpcwalletrbf (or -walletrb) flag.

Sjors force-pushed on Dec 21, 2017

laanwj commented at 11:59 am on December 21, 2017: member

RPC and debug console won’t use RBF by default and honor the -rpcwalletrbf (or -walletrb) flag.

Sounds good to me, as it’s documented. Doing it otherwise would introduce new complications between core and GUI code that aren’t worth it.

I’m not sure the deprecation (nor rename) is worth it either. Soon enough I expect that RBF will be the default for both GUI and RPC, and there’s no longer need for two separate options nor defaults.

Sjors force-pushed on Dec 21, 2017

Sjors commented at 12:11 pm on December 21, 2017: member

I pushed some whitespace and variable name fixes, as well as repaired the bumpfee.py test.

I’m open to not doing the rename / deprecation, but if there’s not too much downside to it, I do think it adds clarity.

jonasschnelli commented at 7:24 pm on December 21, 2017: contributor

I think the consensus is that we enabled RBF by default now. Merging this as it is would cause confusion. I think we should move the PR towards enabling it everywhere.

Sjors commented at 7:27 pm on December 21, 2017: member

@jonasschnelli I think this needs to be addressed first (copied from above):

I’m still reluctant about changing the RPC default, which we also chatted about. One thing I’d like to (see someone else) do is go through popular wallets and services to see how they treat RBF payments and how they treat bumped fees. When it’s a relatively small user base, Bitcoin Core QT users, that’s one thing. At least the sender probably knows a bit more than average about Bitcoin and can community directly with the recipient. But when every Bitcoin ATM’s or other automated process that runs Core starts doing this there’s a problem.

jonasschnelli commented at 7:31 pm on December 21, 2017: contributor

Merging this would mean we have to get rid of the -rpcwalletrbg -guiwalletrbf split later?

Sjors commented at 7:34 pm on December 21, 2017: member

No, we would get rid of the -*walletrbf flag entirely. If there’s any reason to keep that flag, then it needs to be separate for QT and GUI.

jonasschnelli commented at 7:36 pm on December 21, 2017: contributor

I don’t think a split between GUI and RPC defaults is desirable for something we want to have enabled by default long term anyways (on both sides). Splits between GUI and RPC should be avoided where possible to lower confusion.

Sjors commented at 7:37 pm on December 21, 2017: member

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 commented at 7:44 pm on December 21, 2017: contributor

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 commented at 8:21 pm on December 21, 2017: contributor

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 force-pushed on Dec 21, 2017

Sjors commented at 8:33 pm on December 21, 2017: member

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 commented at 8:35 pm on December 21, 2017: contributor

Nice. utACK dcd50b90b94fbf2295a122693570218932bf15b4

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

Sjors commented at 8:43 pm on December 21, 2017: member

@jonasschnelli fixed the PR description.

laanwj commented at 8:45 pm on December 21, 2017: member

utACK dcd50b90b94fbf2295a122693570218932bf15b4

laanwj commented at 9:15 pm on December 21, 2017: member

Travis fails on trailing whitespace :(

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

[Wallet] Use RBF by default in QT only

GUI wallet uses RBF by default, regardless of -walletrbf.

RPC and debug console in the GUI remain unchanged; they don't
use RBF by default, unless launched with -walletrbf=1.

5cbbbd7143

Sjors force-pushed on Dec 22, 2017

Sjors commented at 8:45 am on December 22, 2017: member

@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 commented at 9:05 am on December 22, 2017: member

Travis still unhappy, not sure why:

laanwj commented at 11:59 am on December 22, 2017: member

Seems Travis is happy now, probably a transient issue.

laanwj commented at 12:15 pm on December 22, 2017: member

Tested ACK 5cbbbd7.

laanwj merged this on Dec 22, 2017

laanwj closed this on Dec 22, 2017

laanwj referenced this in commit f19ca129ff on Dec 22, 2017

Sjors deleted the branch on Dec 22, 2017

PastaPastaPasta referenced this in commit 60475f1272 on Jun 25, 2019

barrystyle referenced this in commit c084ef2d60 on Jan 22, 2020

PastaPastaPasta referenced this in commit 64aba48c41 on Feb 13, 2020

PastaPastaPasta referenced this in commit 8e1572d59f on Feb 27, 2020

PastaPastaPasta referenced this in commit 3855979343 on Feb 27, 2020

ckti referenced this in commit 102b6d70dc on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

[Wallet] Enable RBF by default in QT #11605