Support ignoring "opt-in" flag for RBF (aka full RBF) #25373

luke-jr commented at 7:52 PM on June 14, 2022: member

Here's full RBF support, as included in Knots since 2016 (not including the service bit), including functional tests.

Two additional commits have been added, which can be squashed, but kept separate for now, for review purposes:

The first refactors the full RBF test to use a function parameter rather than avoid rearranging the self.nodes list. By separating this out, the test changes should be easier to review.
The second removes never-replace support. I think it's strictly better to support never-replace (as seen, it is trivial), but if reviewers disagree, this can be squashed into the PR to avoid re-supporting it.

laanwj added the label TX fees and policy on Jun 14, 2022

in src/policy/settings.h:24 in c6decd6283 outdated

  20 | @@ -21,6 +21,7 @@ extern CFeeRate dustRelayFee;
  21 |  extern CFeeRate minRelayTxFee;
  22 |  extern unsigned int nBytesPerSigOp;
  23 |  extern bool fIsBareMultisigStd;
  24 | +extern bool gReplacementHonourOptOut;

ariard commented at 10:01 PM on June 14, 2022:

As raised in #25353 and an observation I share, I think mempool settings would be better off encapsulated in a new struct rather than to add yet another global.

luke-jr commented at 10:48 PM on June 14, 2022:

Out of scope for this PR

in src/init.cpp:561 in c6decd6283 outdated

 554 | @@ -555,6 +555,7 @@ void SetupServerArgs(ArgsManager& argsman)
 555 |      argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 556 |      argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 557 |      argsman.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 558 | +    argsman.AddArg("-mempoolreplacement", strprintf("Set to either \"fee,optin\" to honour RBF opt-out signal, or \"fee,-optin\" to always RBF aka full RBF (default: %s)", DEFAULT_REPLACEMENT_HONOUR_OPTOUT ? "fee,optin" : "fee,-optin"), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

ariard commented at 10:14 PM on June 14, 2022:

Logically the two patches are similar, beyond the implementation approach, the main difference is in the name setting pickup and the description. It sounds to me a minor point as the more substantial efforts are to document correctly the full-rbf behavior, the interactions with existing softwares (not only standard wallet but also second-layers) and communicate them well towards the community and node operators.

Though if I should make a comment on the naming itself, mempoolreplacement is confusing to me as firstly there is no false option. Secondly referring to the fee partial argument is redundant as it's already implied by RBF and thirdly the optin versus -optin is more likely to bewilder the operators and lead them to misconfigure their nodes.

And lastly, after this PR, they do have to take action by setting -mempoolreplacement="fee,optin" to keep the previous node behavior running, in contrary to #25353 where only the interested users have to do something.

unknown commented at 10:44 PM on June 14, 2022:

    argsman.AddArg("-mempoolreplacement", strprintf("Set to either \"optin\" to honour RBF opt-out signal, or \"full\" to always RBF aka full RBF (default: %s)", DEFAULT_REPLACEMENT_HONOUR_OPTOUT ? "optin" : "full"), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

ariard commented at 10:47 PM on June 14, 2022:

That's an improvement. Though still don't make this approach non-invasive for current users AFAICT.

luke-jr commented at 10:52 PM on June 14, 2022:

"full" is not a good description of the behaviour.

ariard commented at 10:21 PM on June 14, 2022: member

Honestly, I don't really care which patch gets in. Both of them enables to build full-rbf peers topology to serve the use-cases I'm interested with.

However, I would note this patch is requesting config action from the node operators to keep the default behavior running. I think there will be an additional communication work with this patch towards users to explain they have to think more now about their node settings...

luke-jr commented at 10:52 PM on June 14, 2022: member

However, I would note this patch is requesting config action from the node operators to keep the default behavior running. @ariard This isn't true...

I think there will be an additional communication work with this patch towards users to explain they have to think more now about their node settings...

Users should already think about that. This PR doesn't change that.

Though if I should make a comment on the naming itself, mempoolreplacement is confusing to me as firstly there is no false option.

There is. Just not supported by this version of Core.

Secondly referring to the fee partial argument is redundant as it's already implied by RBF

RBF is not the only possible replacement policy.

and thirdly the optin versus -optin is more likely to bewilder the operators and lead them to misconfigure their nodes.

?

And lastly, after this PR, they do have to take action by setting -mempoolreplacement="fee,optin" to keep the previous node behavior running,

No. fee,optin remains the default.

ariard commented at 11:27 PM on June 14, 2022: member

@ariard This isn't true...

Right I withdraw what I said this patch does not request additional config action from the node operators. Because when -mempoolreplacement is true it implies RBF opt-out is enforced (a replacement policy). However when -mempoolreplacement is false it implies full-RBF (also a replacement policy). I think the patch is confusing by not using the constant DEFAULT_REPLACEMENT_HONOUR_OPTOUT here : https://github.com/bitcoin/bitcoin/pull/25373/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1045...

Anyway, I reiterate I don't care which patch gets in and I'll let other reviewers express which approach they prefer. I won't comment further. If it's just a config naming issue after all, it's not worth my energy to argue on :shrug:

petertodd commented at 6:04 AM on June 15, 2022: contributor

Since we're not adding never-replace support to Bitcoin Core, I think the approach in #25353 of just having a simple -fullrbf option is much simpler for users to understand.

Of course, I'd still say the better option is to just remove opt-in RBF entirely rather than having the intermediate step of an option.

DrahtBot commented at 8:29 AM on June 15, 2022: 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:

#25522 (test: Remove -acceptnonstdtxn=1 from feature_rbf.py by MarcoFalke)
#25353 (Add a -mempoolfullrbf node setting by ariard)
#25302 (build: Check usages of #if defined(...) by brokenprogrammer)
#25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)

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.

darosior commented at 9:06 AM on June 15, 2022: member

Concept ACK. Whether this one or #25353, i think we ought to give users the possibility to run an incentive-compatible policy.

Regarding the approach, i prefer #25353. The goal to provide users with more tools to choose their own mempool policy is understandable, but the possibilities are endless. Not accepting higher fee replacement transactions is not miner-incentive compatible and i have not seen users request support for this feature. In light of this, i think the -fullrbf command line option is a better, canonical, approach to provide the possibility to run the only reasonable policy wrt mempool replacement.

unknown commented at 11:01 AM on June 15, 2022: none

Concept ACK

This is better than #25353 although not removing support for never-replace policy could have been better.

The values for config option are confusing even for CLI users. Even in knots I prefer changing RBF policy using GUI for testing:

michaelfolkson commented at 11:02 AM on June 15, 2022: contributor

Concept ACK. Haven't assessed which I prefer on #25353 or this but will probably be a weak preference (if any).

Thanks for opening this anyway, wasn't aware that Knots had been supporting this since 2016

luke-jr commented at 2:15 PM on June 15, 2022: member

Whether this one or #25353, i think we ought to give users the possibility to run an incentive-compatible policy.

Note that RBF is not incentive-based for nodes, only for miners. Nodes do not benefit whatsoever from transactions (other than their own) being replacable.

Since we're not adding never-replace support to Bitcoin Core, I think the approach in #25353 of just having a simple -fullrbf option is much simpler for users to understand.

Other implementations (including both old Bitcoin Core versions and current Knots versions) already use -mempoolreplacement. Using a new option makes no sense except to create compatibility issues.

darosior commented at 2:23 PM on June 15, 2022: member

Note that RBF is not incentive-based for nodes, only for miners. Nodes do not benefit whatsoever from transactions (other than their own) being replacable.

They do? Compact block relay, fee estimation, etc.. As a node you very much want to have your mempool be as close as possible to miners' ones. And the best guess you have to achieve that is to assume they are incentivized by fees internal to transactions and run a miner-incentive-compatible mempool policy. But i'm sure you know that already. What makes you say nodes don't care? ------- Original Message ------- Le mercredi 15 juin 2022 à 4:15 PM, Luke Dashjr @.***> a écrit :

Whether this one or #25353, i think we ought to give users the possibility to run an incentive-compatible policy.

Note that RBF is not incentive-based for nodes, only for miners. Nodes do not benefit whatsoever from transactions (other than their own) being replacable.

Since we're not adding never-replace support to Bitcoin Core, I think the approach in #25353 of just having a simple -fullrbf option is much simpler for users to understand.

Other implementations (including both old Bitcoin Core versions and current Knots versions) already use -mempoolreplacement. Using a new option makes no sense except to create compatibility issues.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

luke-jr commented at 4:54 PM on June 15, 2022: member

They do? Compact block relay, fee estimation, etc.. As a node you very much want to have your mempool be as close as possible to miners' ones. And the best guess you have to achieve that is to assume they are incentivized by fees internal to transactions and run a miner-incentive-compatible mempool policy.

This is backward. Miners have an incentive to use policies matching nodes, not vice-versa.

MarcoFalke commented at 5:10 PM on June 15, 2022: member

Other implementations (including both old Bitcoin Core versions and current Knots versions) already use -mempoolreplacement. Using a new option makes no sense except to create compatibility issues.

We don't support long EOL versions of Bitcoin Core. And for the compatibility concerns, I don't see why a three line patch in Knots to support a setting alias is a strong reason for us to pick one thing or the other. This is no different from being able to set the chain via -regtest or -chain=regtest, which are aliases for each other.

Personally I prefer #25353 because the name of the setting and the value is easier for users to understand, but I don't really care too much.

MarcoFalke commented at 4:03 PM on June 16, 2022: member

Also, for disabling replacements completely: It is already possible today to disable it in Bitcoin Core and Knots in a fully compatible way by simply setting the incremental relay fee to infinity.

unknown approved

unknown commented at 3:03 AM on June 17, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25373/commits/c6decd62837afd8a9e016fa4790b3d25ed7a107a

nit:

Values for config options are confusing and could be changed with something that is one word or one integer.
Would prefer if support for never-replace was not removed however workaround suggested by @MarcoFalke could work for now or Knots.

DrahtBot added the label Needs rebase on Jun 20, 2022

BitcoinErrorLog commented at 5:38 PM on June 22, 2022: none

Hard NACK. This is sliding further down the slope of RBF-only on by default, and eventually prevents normal original usage of Bitcoin, and creates further uncertainty and optionality for merchants to accept 0-conf payments within their own risk tolerance. I appreciate the off-by-default designation, but generally RBF is not a popularly used feature (despite some apps having it on by default already) and does not deserve constant creeping into a cultural norm.

ariard commented at 1:08 AM on June 23, 2022: member

@BitcoinErrorLog

If you do have substantial concerns about this PR or the sister PR (#25353), both proposing a full RBF setting in Bitcoin Core while differing in their implementations, I would invite you to express them on this mail list thread : https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html. To give wider publicity for the conversation rather than lost in GH, I'll follow-up there.

I fully understand the uncertainty and optionality for 0-conf merchants of the existence of a full-rbf topology on the p2p network. However, the rational behind my proposal isn't motivated by making full-RBF a cultural norm or whatever but to solve a really tangible and naive DoS issue affecting the funding flow of p2p coinjoin, on-chain DLCs, dual-funded channels, etc. I think it's always interesting as a community to have a conversation on how we should approach the design and deployment of policy in case of conflicting use-cases (i,e "X is made worst by making better Y").

Note, RBF is also a fundamental piece of Lightning security model w.r.t time-sensitive transactions [0]

[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2018-November/001697.html

ghost commented at 8:39 PM on June 26, 2022: none

This is sliding further down the slope of RBF-only on by default

I would NACK it when its proposed in future as default not sure about others.

and eventually prevents normal original usage of Bitcoin, and creates further uncertainty and optionality for merchants to accept 0-conf payments within their own risk tolerance.

Original bitcoin had lot of issues that have been fixed in last years. Zero confirmation payment accepted as final is a gamble and protocol cannot prohibit people from gambling. They can do it differently when 2 RBF policies exist.

I appreciate the off-by-default designation, but generally RBF is not a popularly used feature (despite some apps having it on by default already) and does not deserve constant creeping into a cultural norm.

Even I ACKed it because its not default and agree RBF signalling, replacements etc. are less used. @ariard claims this would increase once we have full-rbf and a few things make sense. Not everything though however people are free to use different RBF policies. There is no cultural norm. Users can already try any RBF policy and knots provides some options including full-rbf.

Full RBF could fix some issues including vulnerable LN, DLC and coinjoin although these are not consensus rules and users, miners, etc. (including attackers) can use any policy. Or some of these projects adjust to multiple RBF policies in future.

Finally NACKing this PR is useless, should either NACK #25353 or share your opinions on mailing list as suggested by @ariard

Restore -mempoolreplacement option to allow disabling opt-in RBF

This partially reverts commit 8053e5cdade87550f0381d51feab81dedfec6c46.

7831d5294d

Make it possible to unconditionally RBF with mempoolreplacement=fee,-optin d5d9230fca

QA: feature_rbf: Test full RBF mode 54527b816c

squashme! QA: feature_rbf: Avoid reordering self.nodes b3de8a4060

Remove support for never-replace policy eb6bb1e352

luke-jr force-pushed on Jun 29, 2022

luke-jr commented at 4:57 PM on June 29, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Jun 29, 2022

fanquake commented at 3:57 PM on July 7, 2022: member

I'm going to close this for now, as it's fairly clear (given the number of ACKs and Concept ACKs) that if Core is going ahead with a full RBF setting, it's going to be happen via #25353. This PR could always be re-opened in future if that PR doesn't end up being merged, however I don't think we need to keep a competing PR open at this point.

fanquake closed this on Jul 7, 2022

DrahtBot locked this on Jul 7, 2023