Remove -mempoolreplacement to prevent needless block prop slowness.

TheBlueMatt commented at 1:02 pm on June 8, 2019: member

At this point there is no reasonable excuse to disable opt-in RBF, and, unlike when this option was added, there are now significant issues created when disabling it (in the form of compact block reconstruction failures). Further, it breaks a lot of modern wallet behavior.

This removes an option that is:

(a) only useful when a large portion of (other) miners enforce it as well
(b) is detrimental to everyone (income for miners, RBF notifications for others) who uses it individually otherwise
(c) is effectively unused
(d) is often confused with disabling RBF (rather than just remaining stubbornly unaware of it while the rest of the network lets it through)

fanquake added the label Validation on Jun 8, 2019

meshcollider commented at 1:14 pm on June 8, 2019: contributor

Also need to remove the option from the parameter list

Remove -mempoolreplacement to prevent needless block prop slowness.

At this point there is no reasonable excuse to disable opt-in RBF,
and, unlike when this option was added, there are now significant
issues created when disabling it (in the form of compact block
reconstruction failures). Further, it breaks a lot of modern wallet
behavior.

8053e5cdad

TheBlueMatt commented at 1:32 pm on June 8, 2019: member

Fixed some dangling references.

TheBlueMatt force-pushed on Jun 8, 2019

jnewbery commented at 1:35 pm on June 8, 2019: member

Further, it breaks a lot of modern wallet behavior.

Please describe what wallet behaviour this breaks. The majority of nodes seem to use the default. What problem are caused by node operators having the option to set the RBF policy of their mempool?

This configuration option was added in #7386#issue-56728257. I can’t see any harm in leaving this here.

in doc/man/bitcoin-qt.1:491 in 8053e5cdad

487@@ -488,9 +488,6 @@ Relay and mine data carrier transactions (default: 1)
488 Maximum size of data in data carrier transactions we relay and mine
489 (default: 83)
490 .HP
491-\fB\-mempoolreplacement\fR

promag commented at 1:36 pm on June 8, 2019:

I think these are updated automatically.

promag commented at 1:39 pm on June 8, 2019: member

Won’t this break existing configurations? Maybe temporarily make -mempoolreplacement a hidden argument and add a release note?

TheBlueMatt commented at 1:41 pm on June 8, 2019: member

Please describe what wallet behaviour this breaks.

Indeed, things are mostly fine precisely because no one appears to materially use this option. However, if people were using this option it would (obviously) break a pretty important behavior wallets rely on.

I can’t see any harm in leaving this here.

I don’t think we generally leave around options which allow someone to hamstring their node’s performance for no reason. I think the burden of proof should be the other way - is there a legitimate use-case for leaving this in?

luke-jr commented at 2:01 pm on June 8, 2019: member

This seems to be an excuse to expect a uniform mempool and node policy. Concept NACK if so.

jnewbery commented at 3:26 pm on June 8, 2019: member

I don’t think we generally leave around options which allow someone to hamstring their node’s performance for no reason.

Hamstring is a pretty strong word. Yes, this makes block propagation through these nodes marginally slower due to a slightly higher number of blocktxns required, but it doesn’t prevent the node from propagating blocks and keeping up with tip

I think the burden of proof should be the other way - is there a legitimate use-case for leaving this in?

No, the burden of proof is always on the author proposing the change. If you want to argue that this option causes significant issues, then you should provide performance figures to back that up.

TheBlueMatt commented at 3:44 pm on June 8, 2019: member

Hamstring is a pretty strong word.

Not IMO, no. A blocktxn roundtrip is a huge, huge difference in block propagation speed.

No, the burden of proof is always on the author proposing the change.

I really don’t think this is true for an all-red patch (assuming there is justification that the code is unused/unnecessary, which I think is very, very clear in this case). If anything we should strongly encourage all-red patches!

TheBlueMatt commented at 3:54 pm on June 8, 2019: member

Also note that there is (apparently) some quantity of hashrate still running with this option. That is: a) likely a carryover from old drama that no longer applies and is a forgotten config option, b) interferes with some modern wallet behavior, causing them to not be able to get their transactions confirmed in a timely fashion, c) causing the given miner to lose out on reward because of some past configuration that has likely been forgotten.

jnewbery commented at 4:44 pm on June 8, 2019: member

Having spoken to others about this, I’m now a +0. I don’t think either of the reasons given in the PR description are compelling enough to remove the option, but the fact that this option could be confusing for node operators means that ideally we wouldn’t support it.

DrahtBot commented at 5:23 pm on June 8, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15891 (test: Require standard txs in regtest by default by MarcoFalke)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift commented at 8:17 am on June 9, 2019: contributor

@TheBlueMatt, the context of this PR is yesterday’s nice Breaking Bitcoin talk “Mempool analysis and simulation” where @kallewoof mentions that Antpool is not mining replacement RBF transactions (this segment of the presentation), right? I think that is worth mentioning in the OP: it makes the discussion easier to understand.

Perhaps worth trying to reach out to Antpool? Hopefully they choose to change this setting.

Empact commented at 1:38 am on June 10, 2019: member

Concept NACK

assuming there is justification that the code is unused/unnecessary, which I think is very, very clear in this case

Seems you’re begging the question. Would you evince the delays? Given opinions differ on whether to prioritize RBF over incremental confidence in zero-conf transactions, it seems worthwhile to offer an option absent overriding evidence.

In a meta-note, we should cite our claims wherever possible. Absent that there’s a risk of mistaken expectation.

sdaftuar commented at 12:08 pm on June 10, 2019: member

Concept ACK. I’m not sure it’s worth arguing over as we have plenty of more important things to do, but I do think that we should get rid of this option – I will try to explain because I suspect there’s some misunderstanding of what this option does because I don’t think this is something anyone should want to use.

The issue here is not about whether we continue the BIP 125 RBF compromise where some transactions are not replaceable while others are. Our replacement policy already gives users a choice of whether to opt their transactions in to RBF or not, and we have tools in the wallet so that transaction recipients can be aware of whether an unconfirmed transaction is subject to that replacement policy. This PR does not change any of that.

What this PR is proposing is to get rid of a command-line option that is (a) a footgun for users and (b) does not reflect what I believe to be the understanding most users have, which is that BIP-125-style replacement strategies are expected to propagate well on the network.

To explain further: (a) For non-miners, I do not believe there is any good reason to run with mempool replacement disabled. Say you’re a merchant who doesn’t wish to receive opt-in RBF transactions – we already report this information in the wallet so that you could go back to the customer and decline the transaction until it is either confirmed or replaced with a non-rbf signaling transaction. Because the -mempoolreplacement flag does not reject RBF-signaling transactions (only their replacements!) this flag is actually counterproductive for a merchant to use, as it does not prevent their wallet from reporting receipt of a RBF-signaling transaction (ie you still need to check whether received transactions descend from RBF-signaling ones), but it DOES prevent their wallet from being aware of the transaction being replaced by the rest of the network if a conflicting one is relayed, and it even prevents their wallet from being aware of a replacement that would no longer signal RBF.

For miners: of course disabling replacement reduces fee income, so this is a footgun in that a miner using this might be unaware that they are hurting their bottom line by using it. Not long ago we removed the ability for miners to use getblocktemplate without segwit-support, as we discovered some miners inadvertently were producing non-segwit blocks due to operational issues. Removing this option is a bit like removing production of non-segwit blocks – it’s almost certainly not what the miner intends to do!

But, if a miner was trying to be thoughtful about this and doing what they believe is in the best interests of the network, they might be willing to make this tradeoff for a while – however even then, I would think that a miner opposed to BIP 125 would much more likely want to censor RBF-signaling transactions altogether, rather than just censor their replacements. Imagine a merchant somewhere takes zero-conf payments and never accepts rbf-signaling transactions, and imagine there is some miner out there who is ideologically aligned as well and decides to use this command line option. Whenever that merchant receives an RBF-signaling transaction and demands the sender replace it with a non-signaling one, it finds that their ideologically-aligned miner is never able to mine that replacement transaction!

In short, I can’t think of any justification for this behavior, as I do not believe it helps accomplish any goal (even goals I do not share). Of course, this is non-consensus code and all network participants can do what they like with regard to mempool policy without breaking the network. But that brings me to my next point:

(b) Wallet users today expect that BIP 125 is largely supported, and we should encourage this. BIP 125 struck a balance between satisfying part of the community that (however misguided) wanted to preserve how they understood transaction relay to work in the past. Today I believe that users generally expect that BIP 125 transaction replacements should relay well on the network, and that this does not harm non-BIP-125 users. We should not go out of our way to make it easy for users to do something that we think is bad for the network and harmful to others.

I made the point above that a miner opposed to BIP 125 might realistically want to censor BIP-125 signaling transactions. While that would be something we could not prevent, if someone were to open a PR that implemented such a behavior behind a command-line option that defaulted off, I should hope that everyone would NACK it as incompatible with our project’s goals! I think this -mempoolreplacement option is a bit like that, except that it falls so short of accomplishing even a misguided goal that I think we should all be able to agree to get rid of it.

I don’t think this rises to the level that Luke is concerned about, namely a prelude to forcing a common relay policy on all nodes. In particular I do agree it makes sense that we offer some ways of customizing policy parameters (eg the mempool size, min relay fee, etc). Instead, I think the justification for this change is that we should not support behaviors we think are harmful to the ecosystem overall and have no legitimate use-case, and we should eliminate ways that users might inadvertently shoot themselves in the foot.

practicalswift commented at 3:51 pm on June 10, 2019: contributor

@sdaftuar Thanks for providing additional context and clarification.

Concept ACK

kallewoof commented at 8:25 am on June 11, 2019: member

Concept ACK.

For a bit of context, the miner that appeared to be using this flag (or something similar) claims it was a configuration error, which aligns with what @sdaftuar is saying.

in src/validation.cpp:491 in 8053e5cdad

484@@ -486,15 +485,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
485                 // unconfirmed ancestors anyway; doing otherwise is hopelessly
486                 // insecure.
487                 bool fReplacementOptOut = true;
488-                if (fEnableReplacement)
489+                for (const CTxIn &_txin : ptxConflicting->vin)
490                 {
491-                    for (const CTxIn &_txin : ptxConflicting->vin)
492+                    if (_txin.nSequence <= MAX_BIP125_RBF_SEQUENCE)
493                     {

MarcoFalke commented at 11:54 am on June 11, 2019:

style-nit: Might want to run this through clang-format, as you touch those lines anyway.

MarcoFalke approved

MarcoFalke commented at 11:55 am on June 11, 2019: member

ACK 8053e5cdade87550f0381d51feab81dedfec6c46

Looks like a nice simplification, removing a command line switch that has only a use-case for testing.

promag commented at 2:13 pm on June 11, 2019: member

@MarcoFalke aren’t man files updated automatically?

MarcoFalke commented at 2:16 pm on June 11, 2019: member

True, but doesn’t matter imo.

Empact commented at 3:13 pm on June 11, 2019: member

For non-miners, I do not believe there is any good reason to run with mempool replacement disabled.

IMO node operators have a right to decide how to interact with the network consistent with the protocol. So they do not need a good reason, or a valid excuse[1] to choose to disable or filter the transactions relayed or mined in some way. IMO we should design and advance the network with this in mind, rather than seek to reduce it.

Based on that, I think a removal of choice should be justified by the removal solving some significant problem, or enabling some significant benefit, for example @TheBlueMatt mentions “compact block reconstruction failures”, and “breaks a lot of modern wallet behavior”. Those could rise to the level of justification that I personally would be happy with, but those justifications should be articulated/evinced rather than just invoked.

a miner opposed to BIP 125 would much more likely want to censor RBF-signaling transactions altogether, rather than just censor their replacements.

Putting myself in the shoes of this ideological miner, I disagree: disregarding replacements to rbf-signalling transactions is the same as treating them as non-rbf-signalling transactions. That is, you’re disregarding the signalled behavior, rather than the transaction itself. An ideological miner / relayer is not opposed to transactions but to the behavior.

For miners: of course disabling replacement reduces fee income, so this is a footgun in that a miner using this might be unaware that they are hurting their bottom line by using it

I don’t think being able to opt for incrementally reduced income qualifies as a footgun. A footgun has significant unintended deleterious effects. Incremental loss of income is minor in the scope of things - a reasonable way to address that might be to log warning message.

It is also part and parcel with many opinionated miner configurations. If I set up a miner to produce blocks of segwit-only transactions, I am in all likelihood necessarily reducing my fee earnings, but that does not make the choice invalid - the choice is an expression of my prerogative as a node operator.

The set of miner configurations which optimize for income is very small, if not singular. Accepting possibly reduced miner income as the basis for removing choice seems to me to be the path @luke-jr alludes to.

Wallet users today expect that BIP 125 is largely supported, and we should encourage this.

IMO setting the default behavior to supported is a substantial and reasonable way to express our support for BIP 125. Beyond that I think it is the node operators’ prerogative as to whether BIP 125 is largely supported.

I think being Bitcoin’s reference implementation places a responsibility on the project to accommodate the preferences of node operators wherever the trade-offs allow for it.[2] In a future where we have innumerable node operators operating a global currency on our software, the maximally decentralized and trustless network is not one in which the primary determinant on how the network operates is our own interpretation of user preferences, but rather node operators’ preferences as enabled by our software.

[1] I do not believe we are the arbiters of the validity of their choices. [2] Again, I would be glad to see a further elaboration on the points originally cited.

TheBlueMatt commented at 0:41 am on June 12, 2019: member

IMO node operators have a right to decide how to interact with the network consistent with the protocol. So they do not need a good reason, or a valid excuse[1] to choose to disable or filter the transactions relayed or mined in some way.

Sure, its a P2P network, and people can (and do!) do all kinds of obnoxious things. This does not, however, in any way, mean that we should for some reason support options which are (a) clearly harmful to the node in question and (b) rather harmful if deployed on a large scale (as it will interfere with broadly-deployed wallet behavior, and not just the Bitcoin Core wallet).

Putting myself in the shoes of this ideological miner

Find a major miner who holds this belief, then lets chat. Until then, we have evidence that this option being available resulted in a major miner mistakenly misconfiguring their node, resulting in lost income for the pool, and poor acceptance of some end-user-wallet-generated transactions.

Indeed, I’m not sure this is worth arguing again, but I completely fail to understand how this is somehow worth keeping. It is NOT Bitcoin Core’s job to ensure any kind of crazy local policy someone wants is well-supported, that would be a significant waste of our time. If someone wants this policy they could already rather easily either a) comment out some code or b) use supported APIs like prioritizetransaction to emulate this behavior in their own scripts.

MarcoFalke added this to the milestone 0.19.0 on Jun 12, 2019

Empact commented at 1:40 pm on June 12, 2019: member

Indeed, I’m not sure this is worth arguing again, but I completely fail to understand how this is somehow worth keeping.

I think the removal / change in behavior should be justified or not adopted. The reverse may be easier for the group, but it seems also a significantly lower level of rigor than I would think necessary to prevent problematic changes over time. IMO burden of proof should be on the one putting the PR forward.

In short:

Please describe what wallet behaviour this breaks.

IMO the claim that a miner was using the configuration in error is not compelling. Seems educating users is work for docs, https://bitcoinops.org/ or similar.

TheBlueMatt commented at 2:01 pm on June 12, 2019: member

Sadly, to my knowledge, almost all of our education efforts towards miners have fallen completely flat (despite lots of effort). I appreciate the idealism here, but I dont think it matches up with the real world. Anyway, this is why we can’t have nice things.

TheBlueMatt closed this on Jun 12, 2019

MarcoFalke commented at 2:38 pm on June 12, 2019: member

Why was this closed? Bitcoin Core is not about maintaining every tx relay policy that was every added. If it was, we might as well re-add support for tx priority by coin-days-destroyed or other “altruistic” tx relay/mining features.

sipa commented at 4:20 pm on June 12, 2019: member

I’d like to see this reopened. @Empact I’m in agreement with you that our job is not deciding what policies should be in effect on the network, and that change needs justification. However, don’t you think there is sufficient reason to remove an option that is:

(a) only useful when a large portion of (other) miners enforce it as well
(b) is detrimental to everyone (income for miners, RBF notifications for others) who uses it individually otherwise
(c) is effectively unused
(d) is often confused with disabling RBF (rather than just remaining stubbornly unaware of it while the rest of the network lets it through)

As @MarcoFalke says, our job is not maintaining every relay policy just because it was once common. Bitcoin Core is an implementation of the Bitcoin P2P protocol, and in that protocol as used today, anyone using this option shoots themselves in the foot.

sipa commented at 4:35 pm on June 12, 2019: member

Maybe another way of looking at it: I think we should have configuration options for which we can give advice on the circumstances in which someone might want to use them. For this, I know of no reason why someone would (including “they believe RBF is evil”).

MarcoFalke reopened this on Jun 12, 2019

kallewoof commented at 4:43 pm on June 12, 2019: member

The only entity that seems to have possibly used this flag claims it was a misconfiguration. Are there any other known entities that are actively using this, on purpose, for any reason? If not, I don’t see why this should stay in the code. It has obviously confused, to monetary loss, at least one miner. That turns the question of “is removing this thing justified?” to “is keeping this justified?” in my opinion, and currently, the answer is a flat no. There is no justification to keep this.

Empact commented at 4:45 pm on June 12, 2019: member

I’m partial to argument (c), and all for removing unused functionality. How do we know it is effectively unused? How about declaring it deprecated, removing it in a future version, and reconsidering only if there is push-back?

In any case, I’m not in the anti-RBF camp (to whatever extent one exists), but I have seen incidental evidence of its existence.

Re the others:

(a) minority determinations are still useful in that they are the basis for possible future consensus behavior
(b) I would argue people should be free to engage in behavior that doesn’t maximize their own outcomes, and free to withhold support from others in the form of relay, etc.

practicalswift commented at 4:47 pm on June 12, 2019: contributor

utACK 8053e5cdade87550f0381d51feab81dedfec6c46

promag commented at 11:06 pm on June 12, 2019: member

Deprecation would save from unlikely rantings, still ACK 8053e5c.

luke-jr commented at 2:15 am on June 13, 2019: member

How does this interact with adding full RBF (which uses the same param) when there is support for merging that?

jtimon commented at 10:16 am on June 13, 2019: contributor

utACK 8053e5cdade87550f0381d51feab81dedfec6c46

Besides all the reasons not to use it, if anyone really wants to use this, they can maintain the patch themselves (or pay someone to do it), it’s a small and simple patch anyway.

How does this interact with adding full RBF (which uses the same param) when there is support for merging that?

I guess adding full-rbf as an option would bring some of the code back, that’s fine. Or are you worried about the new argument needing a different name or something like that?

MarcoFalke commented at 10:48 am on June 13, 2019: member

How does this interact with adding full RBF (which uses the same param) when there is support for merging that?

The option is added back with the same name

ajtowns commented at 0:59 am on June 18, 2019: member

ACK 8053e5cdade87550f0381d51feab81dedfec6c46 – quick code review, checked tests work

BIP125 support seemed controversial, so it made sense to have an option available to allow nodes to revert back to the old behaviour from before BIP125 was added – if everyone had decided BIP125 was a bad idea and done a “UASF”-style mass-enabling of the option, that would have worked fine and we could have pulled the code out and gone on with our lives. But that didn’t happen, and it no longer makes much sense to adopt the old behaviour in today’s world where most users/miners/nodes work fine with opt-in rbf. Maybe some other behaviour would be worth having as an option, but that needs to be implemented first. (“only mine/relay tx’s that don’t signal opt-in-rbf” could be worthwhile, but any significant adoption of that seems to me like that’d just make people start using “full-rbf for everything” relay/mining policies)

MarcoFalke referenced this in commit e2182b02b5 on Jun 18, 2019

MarcoFalke merged this on Jun 18, 2019

MarcoFalke closed this on Jun 18, 2019

laanwj commented at 2:26 pm on June 18, 2019: member

Posthumous ACK 8053e5cdade87550f0381d51feab81dedfec6c46

ryanofsky commented at 2:35 pm on June 18, 2019: member

Does this change need release notes?

DrahtBot locked this on Dec 16, 2021

Remove -mempoolreplacement to prevent needless block prop slowness. #16171

Conflicts