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

Merged
merged 1 commit into from Jan 21, 2016

Conversation

Projects
None yet
@laanwj
Member

laanwj commented Jan 21, 2016

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 laanwj added the Mempool label Jan 21, 2016

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 21, 2016

Member

utACK

Member

btcdrak commented Jan 21, 2016

utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Jan 21, 2016

utACK

@jl2012

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jan 21, 2016

Member

utACK

Member

jl2012 commented Jan 21, 2016

utACK

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 21, 2016

Member

0.12 release notes will need mention of this option.

Member

laanwj commented Jan 21, 2016

0.12 release notes will need mention of this option.

@bitkevin

This comment has been minimized.

Show comment
Hide comment
@bitkevin

bitkevin Jan 21, 2016

Contributor

utACK

Contributor

bitkevin commented Jan 21, 2016

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.
@@ -107,6 +107,8 @@ static const bool DEFAULT_TXINDEX = false;
static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
static const bool DEFAULT_TESTSAFEMODE = false;
+/** Default for -permitrbf */
+static const bool DEFAULT_PERMIT_REPLACEMENT = true;

This comment has been minimized.

@wangchun

wangchun Jan 21, 2016

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

@wangchun

wangchun Jan 21, 2016

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 21, 2016

Member

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

@jonasschnelli

jonasschnelli Jan 21, 2016

Member

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

This comment has been minimized.

@jl2012

jl2012 Jan 21, 2016

Member

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

@jl2012

jl2012 Jan 21, 2016

Member

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

This comment has been minimized.

@btcdrak

btcdrak Jan 21, 2016

Member

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

@btcdrak

btcdrak Jan 21, 2016

Member

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

This comment has been minimized.

@laanwj

laanwj Jan 21, 2016

Member

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

@laanwj

laanwj Jan 21, 2016

Member

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

This comment has been minimized.

@gmaxwell

gmaxwell Jan 21, 2016

Member

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

@gmaxwell

gmaxwell Jan 21, 2016

Member

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

This comment has been minimized.

@kristovatlas

kristovatlas Jan 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.

@kristovatlas

kristovatlas Jan 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.

This comment has been minimized.

@dcousens

dcousens Jan 21, 2016

Contributor

@kristovatlas there is a PR for this: #7388

@dcousens

dcousens Jan 21, 2016

Contributor

@kristovatlas there is a PR for this: #7388

This comment has been minimized.

@sipa

sipa Jan 21, 2016

Member
@sipa

sipa via email Jan 21, 2016

Member

This comment has been minimized.

@gmaxwell

gmaxwell Jan 21, 2016

Member

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.

@gmaxwell

gmaxwell Jan 21, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@CodeShark

CodeShark Jan 21, 2016

Contributor

utACK

Contributor

CodeShark commented Jan 21, 2016

utACK

@sandakersmann

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 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.

Contributor

sandakersmann commented Jan 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 21, 2016

Contributor

utACK b768108, happy with default true.

Contributor

dcousens commented Jan 21, 2016

utACK b768108, happy with default true.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 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.

Member

MarcoFalke commented Jan 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 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).

Member

laanwj commented Jan 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 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.

Contributor

sandakersmann commented Jan 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jan 21, 2016

Member
Member

sipa commented Jan 21, 2016

@sandakersmann

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 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.

Contributor

sandakersmann commented Jan 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jan 21, 2016

Member

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

Member

jl2012 commented Jan 21, 2016

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

@sandakersmann

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 21, 2016

Contributor

Can you explain who is hurt by it?

Merchants.

Contributor

sandakersmann commented Jan 21, 2016

Can you explain who is hurt by it?

Merchants.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 21, 2016

Member

Tested ACK b768108

Member

btcdrak commented Jan 21, 2016

Tested ACK b768108

@sandakersmann

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 21, 2016

Contributor

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

Ok. I will go away now.

Contributor

sandakersmann commented Jan 21, 2016

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

Ok. I will go away now.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jan 21, 2016

Contributor

ACK laanwj@b768108

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

Contributor

paveljanik commented Jan 21, 2016

ACK laanwj@b768108

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

@laanwj laanwj merged commit b768108 into bitcoin:master Jan 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 21, 2016

Merge #7386: Add option `-permitrbf` to set transaction replacement p…
…olicy


b768108 Add option `-permitrbf` to set transaction replacement policy (Wladimir J. van der Laan)

laanwj added a commit that referenced this pull request Jan 21, 2016

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.

Conflicts:
	src/init.cpp
	src/main.cpp
	src/main.h

Github-Pull: #7386
Rebased-From: b768108
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 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.

Contributor

dcousens commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sandakersmann

sandakersmann Jan 21, 2016

Contributor

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

Contributor

sandakersmann commented Jan 21, 2016

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

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 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?

Contributor

dcousens commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 21, 2016

Member

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

Member

btcdrak commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment

ACK

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 21, 2016

Member

Nit: Maybe "Permit transaction replacement_s in the memory pool_"?

@MarcoFalke

MarcoFalke Jan 21, 2016

Member

Nit: Maybe "Permit transaction replacement_s in the memory pool_"?

This comment has been minimized.

@dcousens

dcousens Jan 21, 2016

Contributor

Agreed on nit

@dcousens

dcousens Jan 21, 2016

Contributor

Agreed on nit

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Jan 21, 2016

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?

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

This comment has been minimized.

Show comment
Hide comment
@roybadami

roybadami Jan 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.

Contributor

roybadami commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jan 21, 2016

Member
Member

sipa commented Jan 21, 2016

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 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.

Contributor

dcousens commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 21, 2016

Member

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

Member

gmaxwell commented Jan 21, 2016

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

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 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.

Contributor

dcousens commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 21, 2016

Member

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

Member

gmaxwell commented Jan 21, 2016

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

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 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.

Contributor

dcousens commented Jan 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 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.

Member

laanwj commented Jan 22, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment