Replace mempoolfullrbf with more compatible and flexible mempoolreplacement (plus more tests) #25626

Followup to #25353 using traditional mempoolreplacement option that is clearer, more compatible (with Core 0.12-0.18 & Knots), and flexible with other policies.

Also runs test_opt_in against "full RBF" mode (expectations adjusted).

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25648 (refactor: Remove all policy globals by MarcoFalke)
• #21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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.

ACK https://github.com/bitcoin/bitcoin/pull/25626/commits/475d87b5342a93244a5a53a7066929262184084b

Need a janitorial maintainer to merge this with 1 ACK like https://github.com/bitcoin/bitcoin/pull/25153

Need a janitorial maintainer to merge this with 1 ACK like #25153

25153 did not change the user-facing binary at all, nor the internal code structure. This one does change both the code structure as well as the end-user behaviour and experience.

I think I share the opinion of @Xekyo here: #25353 (review). As of today, mempool "full RBF" sounds sufficiently established across the ecosystem's minds, than switching to another name might provoke the opposite effect of confusing people than anything else.

-mempoolfullrbf is the switch. This PR implements what is already in use and has been for years.

-mempoolfullrbf is the switch. This PR implements what is already in use and has been for years.

I don't know if a similar feature present in another bitcoin software should bind this project to be compliant with it and under which conditions. I'm concerned such principle would make us subject to tweak our code in function of any change requested by a weird Erlang or Prolog implementation. I'll let other contributors or users weigh in their opinions.

If there's no reason to be incompatible, it's always better to be compatible.

AFAICT this is a rebase of #25373, changing the interface to achieve the same thing. #25373 was closed in favor of #25353 which was favored by reviewers. If people change their mind and want this interface instead, sure, but that doesn't seem to be the case atm.

more compatible (with Core 0.12-0.18 & Knots)

I see no dev/contrib guidelines or precedent suggesting we should prioritize being compatible with versions 0.12-0.18 (all no longer maintained for at least a year) or Knots.

AFAICT this is a rebase of #25373, changing the interface to achieve the same thing. #25373 was closed in favor of #25353 which was favored by reviewers.

#25373 was the years-old code, while #25353 was newer code based on recent refactoring, and for that reason better. But the PR author refused to address this issue, so it is here as a followup instead.

I see no dev/contrib guidelines or precedent suggesting we should prioritize being compatible with versions 0.12-0.18 (all no longer maintained for at least a year) or Knots.

If you want it in the guidelines, feel free to add it... For one example, there's the prioritisetransaction RPC method. In any case, prioritising compatibility over incompatibility is plainly obvious. The only reason to prioritise incompatibility, is if there is an active interest in harming Bitcoin by enforcing a centralised monopoly over controlling it.

NACK, there are many issues, but to name a few:

• fullrbf is a simple boolean setting, but the new one turns it (or at least the docs) into a string setting that is harder to understand for users
• It is unclear what other policies could be supported, given that the setting appears wholly undocumented in Knots: https://github.com/bitcoinknots/bitcoin/blob/23.x-knots/src/init.cpp#L588
• Even if there were other policies, they'd require at least one miner to support them. Ideally a good portion of the network, see also #25600#pullrequestreview-1041170369 . Absent a use case (or a single user) for those policies, it would be better to not add support for them for now.
• All versions of Bitcoin Core and Bitcoin Knots are already fully compatible to disable rbf, see #25373 (comment) . So writing "dead" code to achieve a goal which is already achieved will just confuse devs and users. Thus may actually decrease effective compatibility due to user confusion.
• If there was a need to make Bitcoin Knots compatible, it can be done with a 3-line "settings-alias" patch in Bitcoin Knots itself. Instead of a +70-35 patch in Bitcoin Core. See #25373 (comment)

fullrbf is a simple boolean setting, but the new one turns it (or at least the docs) into a string setting that is harder to understand for users

String used instead of boolean won't be confusing for users as lot of config options already use string. The values could be confusing for some users. I think you had even agreed to add the option with same name in 2019: #16171 (comment).

Even if there were other policies, they'd require at least one miner to support them. Ideally a good portion of the network, see also #25600#pullrequestreview-1041170369 . Absent a use case (or a single user) for those policies, it would be better to not add support for them for now.

Users, miners etc. only try policies that are available in a bitcoin core based on the trend since RBF was implemented in core. So developers are deciding the policies not the users. If full RBF really increases miners revenue as claimed in #25353 they would be running bitcoin knots in last few years instead of core which isn't true. It would be interesting to ask some mining pools if they care to experiment with RBF policies or use the ones available in core.

So writing "dead" code to achieve a goal which is already achieved will just confuse devs and users. Thus may actually decrease effective compatibility due to user confusion.

It is a workaround and a few other config options can be removed if such workarounds are less confusing for users.

I think you had even agreed to add the option with same name in 2019:

This was under the assumption that the setting has a easily recognizable value, such as "fullrbf", not "fee,-optin".

This was under the assumption that the setting has a easily recognizable value, such as "fullrbf", not "fee,-optin".

"fee,-optin" describes the policy clearly. "fullrbf" does not at all.

I think you had even agreed to add the option with same name in 2019:

This was under the assumption that the setting has a easily recognizable value, such as "fullrbf", not "fee,-optin".

I agree, NACK for this pull request with these values. With bitcoin core at least I can share my opinion on such changes. Can't do with Knots although I like it for a few reasons and we can have different options, values etc. 🌿

This was under the assumption that the setting has a easily recognizable value, such as "fullrbf", not "fee,-optin".

"fee,-optin" describes the policy clearly. "fullrbf" does not at all.

Nobody needs full description in config values

A value that makes sense, one word they can understand or search

With bitcoin core at least I can share my opinion on such changes. Can't do with Knots

Sure you can. Almost everything in Knots has an associated PR (typically here, so it can be included in Core too), and exceptions could use the issue tracker.

Closing for now due to controversy. This has 16 comments, but not a single approval or concept ACK. Happy to reopen if there is one.