Add option `-permitrbf` to set transaction replacement policy

laanwj commented at 10:13 am on January 21, 2016: member

Add a configuration option -permitrbf to set transaction replacement policy for the mempool.

Enabling it will enable (opt-in) RBF, disabling it will refuse all conflicting transactions.

laanwj added the label Mempool on Jan 21, 2016

btcdrak commented at 10:14 am on January 21, 2016: contributor

utACK

jonasschnelli commented at 10:15 am on January 21, 2016: contributor

utACK

jl2012 commented at 10:15 am on January 21, 2016: contributor

utACK

laanwj added this to the milestone 0.12.0 on Jan 21, 2016

laanwj commented at 10:21 am on January 21, 2016: member

0.12 release notes will need mention of this option.

bitkevin commented at 10:23 am on January 21, 2016: contributor

utACK

Add option `-permitrbf` to set transaction replacement policy

Add a configuration option `-permitrbf` to set transaction replacement policy
for the mempool.

Enabling it will enable (opt-in) RBF, disabling it will refuse all
conflicting transactions.

b768108d9c

laanwj force-pushed on Jan 21, 2016

in src/main.h: in b768108d9c

106@@ -107,6 +107,8 @@ static const bool DEFAULT_TXINDEX = false;
107 static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
108 
109 static const bool DEFAULT_TESTSAFEMODE = false;
110+/** Default for -permitrbf */
111+static const bool DEFAULT_PERMIT_REPLACEMENT = true;

wangchun commented at 10:26 am on January 21, 2016:

I think the default value should be false, as many pool owners are too lazy to change the default.

jonasschnelli commented at 10:27 am on January 21, 2016:

RBF is opt-in and I think this feature should be enabled by default.

jl2012 commented at 10:27 am on January 21, 2016:

I think default false is better at this moment. But I don’t really care local policy, anyway

btcdrak commented at 10:29 am on January 21, 2016:

@wangchun i dont mind either way but if pools are “too lazy” then it means they dont actually care about the feature. We are adding this because we thought pools wanted the ability to choose.

laanwj commented at 10:30 am on January 21, 2016:

This option is added for people who explicitly care to disable it. They won’t be too lazy to set it.

wangchun commented at 10:31 am on January 21, 2016:

For those who don’t care this feature, the software should work as it was in the old version.

sipa commented at 10:33 am on January 21, 2016:

Users who wish to use opt-in need to be able to dynamically increase fees. They don’t hurt anyone by that, especially not miners who get the larger fee.

laanwj commented at 10:35 am on January 21, 2016:

They already have to take the conscious decision to upgrade to 0.12, which introduces lots of new behavior. Some of that can be disabled using options.

wangchun commented at 10:39 am on January 21, 2016:

Then it might be safer to have opt-in fss rbf by default, and rbf tx should be relayed on the network regardless of the value this parameter.

wangchun commented at 10:54 am on January 21, 2016:

ACK

Well it might be not very easy to relay something that is not going to be accepted in mempool. But this patch is better than nothing.

ChainQuery commented at 3:19 pm on January 21, 2016:

This has been called “opt-in” RBF, if that is the case then should it not actually be opt-in rather than opt-out? I’d like to see the default set to false at least for the 12.0 release.

sipa commented at 3:22 pm on January 21, 2016:

@ChainQuery It’s opt-in for users.

jonasschnelli commented at 3:26 pm on January 21, 2016:

Please don’t confuse …

opt-in in this context stands for: optional enable RBF per transaction; so a such transaction can be replaced
-permitrbf is a configuration value that allows accepting such replacement-transaction into the nodes mempool

MarcoFalke commented at 3:26 pm on January 21, 2016:

@ChainQuery To the best of my knowledge no wallet has opted in to use rbf, yet. (Not even Bitcoin Core)

ChainQuery commented at 3:38 pm on January 21, 2016:

@sipa @MarcoFalke Fair enough, I do appreciate the addition of the -permitrbf option.

h0jeZvgoxFepBQ2C commented at 6:34 pm on January 21, 2016:

I would like to see it with default false, for just upgrading users which are not well informed its the more safe version.

kristovatlas commented at 11:25 pm on January 21, 2016:

+1 for default false to enhance its opt-in quality.

gmaxwell commented at 11:27 pm on January 21, 2016:

@kristovatlas Turning it off would remove people’s ability to use it; exactly the opposite of your suggestion.

kristovatlas commented at 11:33 pm on January 21, 2016:

@gmaxwell it would only degrade users’ ability to use it if node operators don’t want to turn it on.

Since Bitcoin Core is sensitive to contentious changes, and since Opt-In RBF is contentious, I think the default should be the least contentious option for the sake of consistency.

For those politically minded, this would also contribute into a small way to the perception that Core is responsive to the desires of major businesses in the space.

dcousens commented at 11:36 pm on January 21, 2016:

@kristovatlas there is a PR for this: #7388

sipa commented at 11:43 pm on January 21, 2016:

@kristovatlas This is not a consensus rule, so you’re free to choose a different setting than the default here.

As I’ve said on the PR being linked to, I’m certainly willing to consider changing the default if there is controversy about it. But can someone at least point out one service or wallet that is not dealing with this correctly and has no plans to do so in the near future?

gmaxwell commented at 11:44 pm on January 21, 2016:

Since Bitcoin Core is sensitive to contentious changes

No, we avoid contentious consensus rule changes (e.g. hardforks); because no one can choose to avoid the consensus behavior of the network around them. Please don’t mix up totally local behavior from the blockchain consensus rules.

Adding an option here– even though there isn’t any rational given to any of the opposition to restoring the original Bitcoin 0.1 functionality for people to choose to consequentially produce non-final transactions was already a purely political response to those desires. The continued attacks in spite of making that requested, but otherwise illogical, accommodation suggests that the complaining parties are not interested in a civilized dialog.

CodeShark commented at 10:27 am on January 21, 2016: contributor

utACK

sandakersmann commented at 10:56 am on January 21, 2016: contributor

RBF should not be in the code base at all. With that said, default should be no RBF at all. That also means no relaying and mining of RBF transactions.

dcousens commented at 10:58 am on January 21, 2016: contributor

utACK b768108, happy with default true.

MarcoFalke commented at 11:01 am on January 21, 2016: member

utACK b768108. This is not controversial because it is not changing behavior of existing 0.12.0rc1 nodes.

If someone feels like changing the default for 0.12.1 or 0.13, a separate pull might be appropriate as introducing new features and discussion about changing the behavior should be split in most cases.

laanwj commented at 11:07 am on January 21, 2016: member

RBF should not be in the code base at all. With that said, default should be no RBF at all. That also means no relaying and mining of RBF transactions.

It’s too late for that. Opt-in RBF is well-deliberated progress, not a random spur of the moment change. The RBF discussion has been going on for years, almost since the beginning of bitcoin, this was eventually the best compromise. For wallets, RBF is the solution to transactions stuck because they have attached too little fee (a very common complaint). And it is non-coercive in any way: Users can opt-in to send their transactions with RBF, or not. Receivers can see if a transaction opts in to RBF (#7222).

sandakersmann commented at 11:09 am on January 21, 2016: contributor

This is not controversial because it is not changing behavior of existing 0.12.0rc1 nodes.

The behaviour of 0.12.0rc1 nodes are controversial.

sipa commented at 11:13 am on January 21, 2016: member

The behaviour of 0.12.0rc1 nodes are controversial.

Can you explain who is hurt by it?

sandakersmann commented at 11:16 am on January 21, 2016: contributor

It’s too late for that.

Guess the community have to run Bitcoin Classic then.

For wallets, RBF is the solution to transactions stuck because they have attached too little fee (a very common complaint).

If we increase the blocksize this would not be a problem.

And it is non-coercive in any way

People accepting 0-conf would like to continue to do that.

jl2012 commented at 11:18 am on January 21, 2016: contributor

@sandakersmann I don’t think this is the proper location to discuss RBF per se. This is about the switch

sandakersmann commented at 11:18 am on January 21, 2016: contributor

Can you explain who is hurt by it?

Merchants.

btcdrak commented at 11:19 am on January 21, 2016: contributor

Tested ACK b768108

sandakersmann commented at 11:20 am on January 21, 2016: contributor

I don’t think this is the proper location to discuss RBF per se.

Ok. I will go away now.

paveljanik commented at 11:22 am on January 21, 2016: contributor

ACK https://github.com/laanwj/bitcoin/commit/b768108d9c0b83330572711aef1e569543130d5e

But yes, we should announce such changes one version before it is actually done. I.e. some form of release plan.

laanwj merged this on Jan 21, 2016

laanwj closed this on Jan 21, 2016

laanwj referenced this in commit 6f7841d545 on Jan 21, 2016

laanwj referenced this in commit da83ecd454 on Jan 21, 2016

dcousens commented at 12:43 pm on January 21, 2016: contributor

@sandakersmann if you think it’s worth while bringing up, feel free to make a PR that changes the default, however, I suspect all relevant points have been raised in this PR.

sandakersmann commented at 12:46 pm on January 21, 2016: contributor

WTF!? Did this get merged after just 3 hours and all the people that said that default should be false!

dcousens commented at 12:55 pm on January 21, 2016: contributor

@sandakersmann RBF was already enabled by default in 0.12. To change the default now would mean to change expected behaviour for 0.12, therefore, this could be merged without breaking expectations.

If you (and others) think the default should be changed, its probably best to set it up in a new PR which targets 0.13.

At least, that is my impression, @laanwj 0.12 has been set in stone now right?

btcdrak commented at 12:57 pm on January 21, 2016: contributor

@dcousens correct. This is in fact a big compromise since adding features mid release candidate is usually a forbidden change and would have had to wait until 0.12.1.

raulyaoyuan commented at 1:11 pm on January 21, 2016: none

ACK

in src/init.cpp: in b768108d9c

366@@ -367,6 +367,7 @@ std::string HelpMessage(HelpMessageMode mode)
367     strUsage += HelpMessageOpt("-onion=<ip:port>", strprintf(_("Use separate SOCKS5 proxy to reach peers via Tor hidden services (default: %s)"), "-proxy"));
368     strUsage += HelpMessageOpt("-onlynet=<net>", _("Only connect to nodes in network <net> (ipv4, ipv6 or onion)"));
369     strUsage += HelpMessageOpt("-permitbaremultisig", strprintf(_("Relay non-P2SH multisig (default: %u)"), DEFAULT_PERMIT_BAREMULTISIG));
370+    strUsage += HelpMessageOpt("-permitrbf", strprintf(_("Permit transaction replacement (default: %u)"), DEFAULT_PERMIT_REPLACEMENT));

MarcoFalke commented at 3:41 pm on January 21, 2016:

Nit: Maybe “Permit transaction replacement_s in the memory pool_”?

dcousens commented at 11:28 pm on January 21, 2016:

Agreed on nit

bitcartel commented at 5:05 pm on January 21, 2016: none

Enabling it will enable (opt-in) RBF, disabling it will refuse all conflicting transactions. @laanwj Given the circumstances, the language for this configuration option is a bit confusing. Did you consider naming the option “-disablerbf” with a default setting of false?

roybadami commented at 7:21 pm on January 21, 2016: contributor

@laanwj “Receivers can see if a transaction opts in to RBF (#7286)”

Are you sure you referenced the right pull request there? I don’t see anything related to RBF in the commits in that pull request.

sipa commented at 7:22 pm on January 21, 2016: member

It’s #7222.

dcousens commented at 11:08 pm on January 21, 2016: contributor

@bitcartel that is true, this probably should have been a --disable-rbf flag if the default was to be true. As it stands, it reads more like a default to false is expected.

gmaxwell commented at 11:14 pm on January 21, 2016: contributor

Defaults change over time. You cannot infer the default sense from the name of parameter.

dcousens commented at 11:20 pm on January 21, 2016: contributor

@gmaxwell maybe if the defaults change then the parameter should reflect that? No sense in silently breaking? Breaking changes should always be as obvious as possible IMHO.

gmaxwell commented at 11:23 pm on January 21, 2016: contributor

Changing the parameter name would silently override people’s explicit configuration and invalidate documentation.

dcousens commented at 11:30 pm on January 21, 2016: contributor

@gmaxwell it would force their explicit configuration to fail loudly [assuming deprecated/invalid parameters are rejected]. I think the invalidation of the documentation is reflective of the change, and is representative of the fact that changing the defaults should be painful because it breaks an understanding with users who omitted those options based on their defaults being what they expect.

laanwj commented at 10:32 am on January 22, 2016: member

@laanwj Given the circumstances, the language for this configuration option is a bit confusing. Did you consider naming the option “-disablerbf” with a default setting of false?

You can already do this, for every boolean parameter: -nopermitrbf.

DrahtBot locked this on Sep 8, 2021

Add option -permitrbf to set transaction replacement policy #7386

Add option `-permitrbf` to set transaction replacement policy #7386