Add a `-mempoolfullrbf` node setting

ariard commented at 0:57 am on June 13, 2022: member

This is ready for review.

Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, …) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].

This PR implements a simple fullrbf setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays false, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one’s application requirements, without arguing a change of the default behavior.

I posted on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I’m proposing to express them on the future thread.

[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html

Follow-up suggestions :

soft-enable opt-in RBF in the wallet : #25353 (comment)
p2p discovery and additional outbound connection to full-rbf peers : #25353 (comment)
follow-ups: https://github.com/bitcoin/bitcoin/pull/25575

DrahtBot added the label Validation on Jun 13, 2022

in src/validation.cpp:752 in 1baf9c59d2 outdated

745@@ -746,7 +746,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
746                 // Applications relying on first-seen mempool behavior should
747                 // check all unconfirmed ancestors; otherwise an opt-in ancestor
748                 // might be replaced, causing removal of this descendant.
749-                if (!SignalsOptInRBF(*ptxConflicting)) {
750+                //
751+                // If replaceability signaling is ignored due to node setting,
752+                // replacement is always allowed.
753+                if (!SignalsOptInRBF(*ptxConflicting) && !gArgs.GetBoolArg("-fullrbf", DEFAULT_TRANSACTION_REPLACEMENT)) {

laanwj commented at 7:31 am on June 13, 2022:

I would prefer parsing this setting once in the initialization, then passing it in to validation, instead of caling GetBoolArg for every transaction validation. (this is also better in line with @dongcarl ’s modularization work)

MarcoFalke commented at 8:19 am on June 13, 2022:

It would also be nice to return the fullrbf policy setting in the getmempoolinfo RPC, along with the other policy settings (maxmempool, mempoolminfee, minrelaytxfee)

darosior commented at 9:28 am on June 13, 2022:

+1, i think it should be a member of ATMPArgs.

ariard commented at 1:25 am on June 14, 2022:

Yes sounds good. I’m thinking to introduce a new PolicySettings capturing fullrbf and other things like maxmempool, mempoolminfee, etc. It would be parsed at init with a InitPolicySettings() and given to NodeContext that way making accessible to the rpcs. However, I might have to give copy of a PolicySettings to PeerManagerImpl to as we need access to the policy for current ProcessTransaction() from the p2p network. Then in AcceptToMemoryPool we can initialize ATMPArgs in function of PolicySettings.

I’ll play with that direction and see how it turns out.

glozow commented at 1:53 pm on June 15, 2022:

Agree, definitely shouldn’t be querying gArgs here. Could also be passed in to the MemPoolAccept constructor since it wouldn’t change across different submissions.

I’m thinking to introduce a new PolicySettings capturing fullrbf and other things like maxmempool, mempoolminfee, etc. It would be parsed at init with a InitPolicySettings() and given to NodeContext that way making accessible to the rpcs

sounds like a great idea 🚀

dongcarl commented at 9:23 pm on June 17, 2022:

Feel free to reference #25290 for how I do the ::Options parsing/passing. Although we should resolve this before the PR is merged, it is an implementation detail and can be addressed later in the review cycle if there are more important things to sort out first.

ariard commented at 5:07 pm on June 20, 2022:

Added a commit.

ariard commented at 5:10 pm on June 20, 2022:

The MemPoolOptions wheel made there sounds really good to me so I went to rebase this PR on top of #25290 and just added a m_full_rbf.

Conceptually, I think it’s even better to have the policy options in the mempool struct rather than ATMPArgs as the settings expected to be static across the run of a bitcoind instance.

MarcoFalke approved

MarcoFalke commented at 8:24 am on June 13, 2022: member

lgtm. Should be uncontroversial, given that the default is “unchanged” and remains false.

Maybe as a follow up -fullrbf could turn on the wallet BIP125 flag softly?

MarcoFalke removed the label Validation on Jun 13, 2022

MarcoFalke added the label TX fees and policy on Jun 13, 2022

MarcoFalke added the label Needs release note on Jun 13, 2022

in test/functional/feature_rbf.py:629 in 1baf9c59d2 outdated

624@@ -622,5 +625,31 @@ def test_replacement_relay_fee(self):
625         tx.vout[0].nValue -= 1
626         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
627 
628+    def test_fullrbf(self):
629+        confirmed_utxo = self.wallet.get_utxo()

MarcoFalke commented at 8:30 am on June 13, 2022:

I don’t think this is guaranteed to be confirmed, based on the previous tests, unless a block is mined with all outstanding txs before this call? See also the test failure.

ariard commented at 5:07 pm on June 20, 2022:

See comment answering to Jon review. tldr #22698

michaelfolkson commented at 9:12 am on June 13, 2022: contributor

Concept ACK. Mempool and pinning attacks are a significant concern for multiparty protocols and I agree that Core could help alleviate these concerns (even if it is unable to resolve them entirely).

Just to check my understanding (and what I’m Concept ACKing) here though. There are various combinations of RBF settings.

Transaction signals for RBF, Node doesn’t facilitate RBF (Pre BIP 125 versions of Bitcoin Core)
Transaction signals for RBF, Node facilitates RBF (current version of Core and post BIP 125 versions)
Transaction doesn’t signal for RBF, Node doesn’t facilitate RBF (current version of Core, still the default after this PR)
Transaction doesn’t signal for RBF, Node facilitates RBF regardless (this PR assuming the user changes the “Full RBF” default)
Core wallet doesn’t create transactions that signal RBF by default (current version of Core wallet)
Core wallet creates transactions that signal for RBF by default (possible future mode of Core wallet unless RBF signaling becomes an irrelevance)

Multi-party funded transactions can already signal for RBF and can already change the default of the Core wallet (if they use the Core wallet). Hence whether the node facilitates RBF if the transaction doesn’t signal for RBF isn’t a big deal. It is a minor improvement in the case that the transaction mistakenly didn’t signal for RBF (or was unable to for some reason) but generally the transaction should be able to signal for RBF.

The biggest long term win for multiparty protocols would be nodes facilitating RBF by default but this is a big change. Facilitating “Full RBF” by default would be marginally better still and this PR may be a (small) stepping stone in that direction.

edit: Ok I misunderstood this. I always thought a node could choose not to facilitate RBF even if the transaction was signaling for RBF. But on post BIP125 versions of Bitcoin Core this isn’t the case.

darosior commented at 9:35 am on June 13, 2022: member

Concept ACK. I think it’s good to provide an option for miners to collect more fees, and node operators to run a miner-incentive compatible policy.

fanquake requested review from glozow on Jun 13, 2022

fanquake requested review from instagibbs on Jun 13, 2022

in test/functional/feature_rbf.py:734 in 1baf9c59d2 outdated

635+            from_node=self.nodes[0],
636+            utxo_to_spend=confirmed_utxo,
637+            sequence=SEQUENCE_FINAL,
638+            fee_rate=Decimal('0.01'),
639+        )
640+        assert_equal(False, self.nodes[0].getmempoolentry(optout_tx['txid'])['bip125-replaceable'])

jonatack commented at 10:14 am on June 13, 2022:

This test assertion is failing (AssertionError: not(False == True)) for me.

brunoerg commented at 4:31 pm on June 13, 2022:

Same in CI:

 0 2022-06-13T01:18:21.530000Z TestFramework (ERROR): Assertion failed 
 1                                   Traceback (most recent call last):
 2                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
 3                                       self.run_test()
 4                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_rbf.py", line 98, in run_test
 5                                       self.test_fullrbf()
 6                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_rbf.py", line 734, in test_fullrbf
 7                                       assert_equal(False, self.nodes[0].getmempoolentry(optout_tx['txid'])['bip125-replaceable'])
 8                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 51, in assert_equal
 9                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
10                                   AssertionError: not(False == True)

ariard commented at 1:27 am on June 14, 2022:

Test works locally though see Marco point above, could be the source failure if the parent isn’t confirmed. I’ll investigate.

ariard commented at 5:06 pm on June 20, 2022:

Test failure can be explained by the following reasoning.

If the outpoint is not confirmed, the parent transaction opt-in flags is going to be considered by our RPC code. This is discrepancy between our RPC and mempool logic, see #22698. Ensuring the spent outpoint is always confirmed should solve the issue, as only explicit signaling is considered in that case.

in test/functional/feature_rbf.py:727 in 1baf9c59d2 outdated

624@@ -622,5 +625,31 @@ def test_replacement_relay_fee(self):
625         tx.vout[0].nValue -= 1
626         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
627 
628+    def test_fullrbf(self):
629+        confirmed_utxo = self.wallet.get_utxo()
630+
631+        self.restart_node(0, extra_args=["-fullrbf"])
632+
633+        # Create an explictily opt-out transaction

jonatack commented at 10:15 am on June 13, 2022:

0        # Create an explicitly opt-out transaction

MarcoFalke commented at 6:59 am on June 30, 2022:

not yet fixed?

instagibbs commented at 2:09 pm on June 30, 2022:

concur

ariard commented at 3:35 am on July 1, 2022:

finally fixed

in src/validation.h:66 in 1baf9c59d2 outdated

61@@ -62,6 +62,8 @@ static const unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT = 101;
62 static const unsigned int DEFAULT_DESCENDANT_LIMIT = 25;
63 /** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
64 static const unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT = 101;
65+/** Default for -fullrbf, if the transaction opt-in replaceability is enforced */
66+static const bool DEFAULT_TRANSACTION_REPLACEMENT = false;

fanquake commented at 11:21 am on June 13, 2022:

0static constexpr bool DEFAULT_TRANSACTION_REPLACEMENT{false};

instagibbs commented at 1:47 pm on June 13, 2022:

I’d be more specific https://github.com/instagibbs/bitcoin/commit/27b1393438de56e4271d5d3de46a5de972d59fa6#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R530

DEFAULT_FULL_RBF

ariard commented at 5:04 pm on June 20, 2022:

Went with DEFAULT_MEMPOOL_FULL_RBF.

fanquake commented at 11:21 am on June 13, 2022: member

Concept ACK

naumenkogs commented at 11:49 am on June 13, 2022: member

Concept ACK. I think this change is immediately good.

instagibbs commented at 12:04 pm on June 13, 2022: member

Will review but feel free to take stuff from my branch here:

https://github.com/instagibbs/bitcoin/tree/full_rbf_2022

been running this locally for testing and measurements

in src/init.cpp:556 in 1baf9c59d2 outdated

552@@ -553,6 +553,7 @@ void SetupServerArgs(ArgsManager& argsman)
553     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);
554     argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
555     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);
556+    argsman.AddArg("-fullrbf", strprintf("Accept transaction replacement without opt-in replaceability inspection (default: %u)", DEFAULT_TRANSACTION_REPLACEMENT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

instagibbs commented at 1:44 pm on June 13, 2022:

Would be nice to have a document to point to since this and any description I try to make it too vague

ariard commented at 1:28 am on June 14, 2022:

Yes, I’m thinking to modify doc/policy/mempool-replacements.md in consequence.

luke-jr commented at 3:02 am on June 14, 2022:

Concept NACK on a new option. The -mempoolreplacement option was designed for this use case, and has been long-supported for it in Knots.

unknown commented at 3:50 am on June 14, 2022:

It was removed in https://github.com/bitcoin/bitcoin/pull/16171

laanwj commented at 9:11 am on June 14, 2022:

It’s a valid discussion how the option should be called but NACKing this based on disagreement on the option name is silly imo.

unknown commented at 11:34 am on June 14, 2022:

After reading the comments in PR#16171, I think it should be renamed to -mempoolreplacement, DEFAULT_TRANSACTION_REPLACEMENT const can be an integer with:

0: Disable RBF 1: Opt-in (Default) 2: Full RBF

MarcoFalke commented at 12:08 pm on June 14, 2022:

0: Disable RBF

I think this is getting a bit out of hand to re-add a largely unused setting that was removed with overall support, see also #16171 (comment)

No opinion on how the new setting should be named.

michaelfolkson commented at 12:17 pm on June 14, 2022:

Right, if people want to shedpaint names perhaps open a separate issue to do so. I don’t think that needs to be discussed on this PR.

unknown commented at 12:42 pm on June 14, 2022:

0: Disable RBF

I think this is getting a bit out of hand to re-add a largely unused setting that was removed with overall support, see also #16171 (comment)

No opinion on how the new setting should be named. @MarcoFalke

Some users might be using it or use it once its available as config option in core. Providing options for the users to configure replacement or disable it is a better approach. The pull request had 2 NACKs and was reopened to merge it.

The new option proposed in this pull request might also have less usage if opt-in remains the default.

The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, …) without solutions introducing an overhead cost or centralization vectors afaik .

Rationale provided in this PR is not a good enough reason to add full RBF config. There might be certain projects that are vulnerable to RBF and prefer nodes disable it. However, core cannot fix issues for these vulnerable projects based on different RBF policies. Instead, the projects should fix their issues. Full RBF is still a good config if some users want to replace all transactions irrespective of signaling and we should expect different mempool policies used in future.

michaelfolkson commented at 12:47 pm on June 14, 2022:

@1440000bytes: The suggestion of reversing #16171 would be a (new) separate PR. (It was merged 3 years ago.) A rename discussion for this particular PR would be a (new) issue.

instagibbs commented at 1:43 pm on June 14, 2022:

I have seen zero desire from users to revert the removal of the older option. Assuming we don’t want to support features no one is asking for, it also means we should pick an argument name that hasn’t existed before to remove ambiguity.

luke-jr commented at 3:07 pm on June 14, 2022:

-mempoolreplacement=-optin is non-ambiguous.

instagibbs commented at 3:13 pm on June 14, 2022:

Right, we can get even more specific perhaps. I don’t personally care too much.

ariard commented at 10:46 pm on June 14, 2022:

@1440000bytes

After reading the comments in PR#16171, I think it should be renamed to -mempoolreplacement, DEFAULT_TRANSACTION_REPLACEMENT const can be an integer with:

0: Disable RBF 1: Opt-in (Default) 2: Full RBF

As I’m raising the concern in #25373, (re)-introducing a config setting to keep the default, current node behavior running is a burden we’re inflicting on users. So I believe the -fullrbf approach is less invasive as only the interested users have to tweak their settings.

The new option proposed in this pull request might also have less usage if opt-in remains the default.

As noted in OP, the purpose of this option is for now to enable easy experimentation and observe if there is real demand for full-rbf from newer class of bitcoin softwares. If there is a minority usage, it would have achieved its design goal. It’s okay to have heterogeneity.

Instead, the projects should fix their issues.

The same argument can be made too w.r.t to the “certain projects that are vulnerable to RBF and prefer nodes disable it” they could “fix their issues” ? I think ultimately when we have Bitcoin applications with conflicting requirements, the best we can do is to offer policy flexibility and let operators picking up the one suiting their needs. Though I would note in the present argumentation, multi-party funded use-cases are the ones making the assumption that the transaction-relay policy of the p2p nodes is aligned with miners-incentives (i.e full-rbf). If you have more conceptual disagreement, I’ll invite you to express them on the ML thread, to give a better publicity to the discussion, I’ll answer for sure.

ariard commented at 5:04 pm on June 20, 2022:

Added a mention in doc/policy/mempool-replacements.md, let me know if you have wording.

fanquake commented at 8:58 am on June 30, 2022:

Marking this as resolved as the conversation was just restarted down-thread: #25353 (review).

dunxen commented at 1:51 pm on June 13, 2022: contributor

Concept ACK

benthecarman commented at 4:58 pm on June 13, 2022: contributor

Concept ACK

ariard commented at 1:50 am on June 14, 2022: member

Thanks for the reviews, sent the related post to the ML, if we have more node operators or users with thoughts on fullrbf. In the meanwhile, I’ll see to encapsulate the node arguments related to policy better as per laanwj suggestion, perhaps in its own refactor PR.

@MarcoFalke

Maybe as a follow up -fullrbf could turn on the wallet BIP125 flag softly?

I think if you’re running a PR with -fullrbf and your wallet does not signal opt-in RBF, your issued transactions are going to replace each other, even if they do not propagate on the p2p network, as it’s dependent on being connected to full-rbf peers. If your wallet does signal opt-in RBF, -fullrbf should not change that much things for your own transactions propagation. So yes, I believe the former case should be documented at least, as it can be confusing for a user than -fullrbf “overrides” the opt-in flag. @michaelfolkson

Multi-party funded transactions can already signal for RBF and can already change the default of the Core wallet (if they use the Core wallet). Hence whether the node facilitates RBF if the transaction doesn’t signal for RBF isn’t a big deal. It is a minor improvement in the case that the transaction mistakenly didn’t signal for RBF (or was unable to for some reason) but generally the transaction should be able to signal for RBF.

There is a malicious exploitation where a participant to a multi-party transaction does not signal RBF for a double-spend of the contributed input. If this double-spend is propagated before the honest multi-party transactions that one will stuck in the issuer mempool. From then, the honest participant are left with two options, double-spend their own contributed inputs after a protocol timeout, and thus loosing the utility of their coins during this delay OR fee-bump the multi-party transaction as from their view of the network they might attribute the lack of confirmation to a low-grade feerate. This second reaction might be even worst in term of damages than the first one, as the malicious participant might replace its own double-spend to “unlock” the propagation of the fee-bumped-at-max multi-party transaction, far above the recent blocks feerate. Indeed, the fee-bumping strategies are certainly known by the attacker for open source software. For more details, see the first link in OP subsection “RBF Opt-out by a Counterparty Double-Spend”.

ghost commented at 2:14 am on June 14, 2022: none

This PR implements a simple fullrbf setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays false, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one’s application requirements, without arguing a change of the default behavior.

~~Concept ACK~~

MarcoFalke commented at 9:07 am on June 14, 2022: member

I think if you’re running a PR with -fullrbf and your wallet does not signal opt-in RBF, your issued transactions are going to replace each other, even if they do not propagate on the p2p network, as it’s dependent on being connected to full-rbf peers.

Right, so I am suggesting to soft-enable opt-in RBF in the wallet on -fullrbf, so that wallet-created txs will propagate easier. But this can be done in a follow-up. I just wanted to mention it, so that it isn’t forgotten.

achow101 commented at 6:58 pm on June 14, 2022: member

Concept ACK

luke-jr commented at 9:26 pm on June 14, 2022: member

Full RBF support as supported in Knots since 2016: #25373

unknown changes_requested

unknown commented at 10:49 pm on June 14, 2022: none

~~Approach NACK~~

I do not agree with the rationale provided in this pull request and the approach. I had initially Concept ACKed the PR for the quoted text that mentioned “new setting simply offers more flexibility in a node transaction-relay policy suiting one’s application requirements, without arguing a change of the default behavior”.

After doing some research and reading the comments in #16171, I realized a generalized name related to replacements for this config option as integer or string instead of boolean would be better. We could have 3 basic RBF options for now: 0 Disable RBF, 1 Opt-in(default) and 2 Full RBF

It doesn’t look like these basic options are going to be implemented or config option renamed and if -fullrbf is added we will need more options when some projects are vulnerable to different RBF policies.

Disabling something is a basic requirement which is available for several other config options. However, in future we might even have different RBF policies as these aren’t consensus rules. Example:

0: Disable RBF 1: Opt-in RBF v1 (default) 2: Full RBF 3: Opt-in RBF v2 (with inherited signaling #22698) 4: Opt-in RBF v3 (one or more improvements suggested on mailing list)

ariard commented at 11:44 pm on June 14, 2022: member

After doing some research and reading the comments in #16171, I realized a generalized name related to replacements for this config option as integer or string instead of boolean would be better. We could have 3 basic RBF options for now: 0 Disable RBF, 1 Opt-in(default) and 2 Full RBF

If we think there is a user demand for a full-range of settings w.r.t to replacement policies, included to disable RBF and even rejects opt-in transactions, I’m ready to implement it as long as we keep the default behavior as “1 Opt-in” and there is a full-rbf option. I don’t think the code complexity to deal with multiple settings prevents the flexibility increase. I think it requires more documentation to explain the replacement behavior to the end-user but it sounds reasonable to me.

As we observe new Bitcoin applications requiring different policies we might benefit from this generalized approach. I would say as long as the replacement policy is not completely irrational with miner-incentives, it might makes sense to support (e.g a yet-to-be-designed more privacy-preserving replacement policy).

That said, I’ll let other contributors express an opinion on the generalized -mempoolreplacement (or whatever the name we all agree on) approach before to give it a try.

petertodd commented at 6:33 am on June 15, 2022: contributor

I think if you’re running a PR with -fullrbf and your wallet does not signal opt-in RBF, your issued transactions are going to replace each other, even if they do not propagate on the p2p network, as it’s dependent on being connected to full-rbf peers.

This issue can be mitigated by adding support for the bit 26 service bit already supported in knots and my prior full-rbf patch, and adding it to GetDesirableServiceFlags to preferentially connect with other nodes with full-rbf.

DrahtBot commented at 8:40 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:

#25487 ([kernel 3b/n] Make {Dump,Load}Mempool CTxMemPool methods, decouple from ArgsManager by dongcarl)
#22867 (test: Extend test coverage of BIP125 and document confusing/inconsistent behavior by mjdietzx)

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.

glozow commented at 1:48 pm on June 15, 2022: member

Concept ACK

This is more incentive-compatible for miners, and thus a more performant policy for non-mining nodes wrt compact block reconstruction, fee estimation, etc.

laanwj commented at 2:01 pm on June 15, 2022: member

For sake of containing the scope a little bit: I think discussing other mempool replacement policies is off-topic here. This is about adding an option for full RBF, and only that. If you want other options, please open a new PR.

Making the option name more general to accommodate other policies (even if we don’t merge them into bitcoin core) might make sense, sure.

To add some new shed paint: what about prefixing it with mempool eg -mempoolfullrbf=1, or -mempoolrbf=full (paired with -mempoolrbf=optin). This makes it clear that it’s a mempool option and distinguishes it from -walletrbf.

ghost commented at 3:43 pm on June 15, 2022: none

Attacks described in the ML posts also indicate we should move to full RBF sooner rather than later.

Attacks based on policies that users can change and still do not break any consensus rules will always be possible.

Bitcoin Core has used RBF since 2015 (https://github.com/bitcoin/bitcoin/pull/6871 was included in v0.12). This table suggests that substantial advocacy effort has been made to help businesses support receiving and sending replaceable transactions. These stats (from @0xB10C) show that 25-30% of transactions now signal RBF. These tables (also @0xB10C sent me a few months ago) show that ~4000 transactions are replaced per day, and 2-3% of transactions are replacements:

25-30% and 2-3% in 7 years is very low. Not sure how does this support full RBF.

This buffer has lasted for 7 years. It seems like RBF support is widespread, plenty of tooling exists, and they would still have another year before this is made the default in Bitcoin Core.

Is this speculation or discussed elsewhere that full RBF will be made default in an year? I think users and miners still have a choice to run bitcoind with patches (some already do), knots and old releases.

For sake of containing the scope a little bit: I think discussing other mempool replacement policies is off-topic here. This is about adding an option for full RBF, and only that. If you want other options, please open a new PR.

Making the option name more general to accommodate other policies (even if we don’t merge them into bitcoin core) might make sense, sure.

Other policies were mainly mentioned so that they could be easily accommodated in future if required. I do not believe full RBF fixes everything and users should never try other policies for replacement.

If some developers want to experiment with one RBF policy, I think it makes sense to discuss an option that could be used to disable all RBF policies if the user needs it. Or else they could always use a software that provides such basic options.

instagibbs commented at 4:08 pm on June 15, 2022: member

Let’s please keep discussion to the PR at hand which does not remove existing options, or lock in future plans to do so. (mailing list discussion is a fine place for a more extended discussion about plans)

MarcoFalke commented at 5:17 pm on June 15, 2022: member

I’d also recommend to keep this pull request minimal to only the policy setting and leave wallet changes and p2p changes (if any) to future pull request.

ghost commented at 0:49 am on June 16, 2022: none

Let’s please keep discussion to the PR at hand which does not remove existing options, or lock in future plans to do so. (mailing list discussion is a fine place for a more extended discussion about plans)

I never proposed to remove existing options, future plans to make full-rbf as default were mentioned in #25353#pullrequestreview-1007540701, I replied to it with a question and alternatives for users.

Here’s the mailing list post: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020576.html

ariard commented at 1:34 am on June 16, 2022: member

@petertodd

This issue can be mitigated by adding support for the bit 26 service bit already supported in knots and my prior full-rbf patch, and adding it to GetDesirableServiceFlags to preferentially connect with other nodes with full-rbf

Yes, as I noted on the mailing list this is a potential direction, after we have consensus on the full-rbf policy setting. Though I would prefer to make changes step-by-step to ease the review scope and burden. Adding to the list of of follow-ups.

It seems like RBF support is widespread, plenty of tooling exists, and they would still have another year before this is made the default in Bitcoin Core. @glozow

It seems like RBF support is widespread, plenty of tooling exists, and they would still have another year before this is made the default in Bitcoin Core.

I know I’ve advocated in the past to turn RBF support by default in the past. Though after gathering a lot of feedbacks, this approach of offering the policy flexiblity to the interested users only and favoring a full-rbf gradual deployment sounds better to me. As a follow-up, if we add p2p logic to connect to few “full-rbf” service-bit signaling peers and recommend to the ~17000 LN nodes operators, likely (hopefully!) running bitcoind as a backend, that should be okay to guarantee a good propagation to miners (and yes reaching out to few mining pools ops to explain the income increase brought by full-rbf). Unless we observe a significant impact on compact blocks reconstruction, personally I’m really fine waiting another multi-years development cycle before to propose a default change, or even let opt-in forever the default as it is. @laanwj

To add some new shed paint: what about prefixing it with mempool eg -mempoolfullrbf=1, or -mempoolrbf=full (paired with -mempoolrbf=optin). This makes it clear that it’s a mempool option and distinguishes it from -walletrbf.

Good idea to prefix, though beyond no strong opinion on the shed paint color. @1440000bytes

Attacks based on policies that users can change and still do not break any consensus rules will always be possible.

IIUC, you’re saying that a user is always free to change her bitcoind settings or add a patch to alter the behavior in a way detrimental to a Bitcoin application running on top ? E.g, in the LN case enabling blocksonly which would deprive the node from an accurate feerate, and thus decrease the efficiency of time-sensitive transactions (e.g a HTLC-timeout). If that’s your point, I think that’s an issue in the L2s release process, in the documentation provided towards those operators to lock-in some bitcoind settings or even further recommend them to use eclipse-hardened source of blocks (note: if you’re running a big LN infrastructure, including to receive payment-only, please do not rely on a lightclient only as a source of blocks).

If you’re saying than upper layers or Bitcoin applications should not make any assumptions on the underlying base-layer beyond the consensus rules, including the component behaviors enabling the liveliness of such consensus (i.e the tx-relay/block-relay p2p network), I believe it’s a rather limited approach of Bitcoin protocol design. If we look out on Internet history, the upper layers of the stack are full of assumptions on the ones under, e.g TCP assuming packet loss or reordering are synonyms of network layer congestion. Actually, the subject of inter-layers assumptions have been always documented and discussed by the Internet community, e.g see RFC 3439. And I don’t think we’ll able to save on such constructive and iterative dialogue in the Bitcoin community too.

Does it mean any “wildcard” assumption should be made by the upper layers on the underlying one ?

Of course not. While it’s still an area of research what are the correct assumptions than protocol and applications designers can make on the Bitcoin base-layer, it sounds there is an emerging rule of thumb that the transaction-relay policy adopted by the wide majority of the p2p network are compatible with the miners incentives. When it’s not the case like with full-rbf (or package relay is another good example), I believe it’s reasonable to request extension and modification of the transaction-relay policy, bridging the gap between the economically-optimal-for-all state and the roughness of the current policy.

Indeed, if a second-layer like Lightning starts to make assumption non-compatible with miners incentives, I agree that would be the burden of the LN community to fix the protocol design. That’s not the present case.

25-30% and 2-3% in 7 years is very low. Not sure how does this support full RBF.

Once again, the proposed change is only targeting educated users aiming to deploy full RBF for their application specific needs. If the majority of Bitcoin users is not interested, that’s okay. It’s a policy rule, not a consensus one.

Is this speculation or discussed elsewhere that full RBF will be made default in an year? I think users and miners still have a choice to run bitcoind with patches (some already do), knots and old releases.

I made such proposal last year though as noted in OP, after collecting feedbacks, I’m thinking this approach is smoother and achieve the goal to mitigate against multi-party funded transactions DoS.

Other policies were mainly mentioned so that they could be easily accommodated in future if required. I do not believe full RBF fixes everything and users should never try other policies for replacement

I’ve never argued than full RBF fixes everything and I hope I’ve been clear that I’m aiming to solve a precise issue (multi-party funded transaction flow). That it solves wider issues around the ecosystem, like wallet fingerprint, that’s nice too.

If anyone would like to propose future policy, I would say let’s discuss it when we have a concrete proposal.

If some developers want to #25353 (review) with one RBF policy, I think it makes sense to discuss an option that could be used to disable all RBF policies if the user needs it.

Personally, I’m okay to add a disable flag in this current PR, it’s a super straightforward code change, it doesn’t require a lot of engineering effort. However, I sense it’s not a shared opinion among other contributors.

Anyway, seen your reply on the thread, I’ll answer there for the conceptual part of this PR.

unknown approved

unknown commented at 3:01 am on June 17, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25353/commits/1baf9c59d2db516ede20d0ed080d2c73eedbdb89

I retract NACK for the approach after reading the last email by @ariard . Rationale for this pull request and most of things in email makes sense. Still need to discuss a few things however I can do that on mailing list.

nit:

Suggestions for renaming the config option mempoolreplacement, mempoolrbf and mempoolrbfpolicy
Would prefer if an option to disable all RBF policies existed although a workaround suggested by @MarcoFalke could also be used. I will use Knots if there are any issues with workaround, when I need it as Core and knots on same machine using a data directory works without any issues.

ariard force-pushed on Jun 20, 2022

ariard commented at 5:03 pm on June 20, 2022: member

Updated at 4a56c1b2

Built on top of #25290 to reuse the MemPoolOptions introduced there. It encapsulates nicely the new mempool setting as suggested here. If you’re interested by this PR, please review #25290 first.

Other changes :

rename the setting as mempoolfullrbf
address the test failure by ensuring the tested utxo is confirmed, the previous failure is assignable to the known discrepancy between our RPC and mempool logics
mention the new setting in doc/policy/mempool-replacements.md
mention the new setting in release notes
update getmempoolinfo with new setting

ariard renamed this:
~~Add a `-fullrbf` node setting~~
Add a `-mempoolfullrbf` node setting
on Jun 20, 2022

DrahtBot added the label Needs rebase on Jun 20, 2022

in src/rpc/mempool.cpp:685 in d9988a7652 outdated

680@@ -680,7 +681,8 @@ static RPCHelpMan getmempoolinfo()
681                 {RPCResult::Type::NUM, "maxmempool", "Maximum memory usage for the mempool"},
682                 {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kvB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"},
683                 {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
684-                {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"}
685+                {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
686+                {RPCResult::Type::BOOL, "mempoolfullrbf", "True if the mempool accepts RBF without replaceability signaling inspection"}

MarcoFalke commented at 7:51 am on June 21, 2022:

style nit in d9988a76521309f7b1c9092d82672a2bc9d96b4b: Missing trailing comma

ariard commented at 5:08 pm on June 21, 2022:

Fixed in 49d6046.

MarcoFalke removed the label Needs release note on Jun 21, 2022

ariard force-pushed on Jun 21, 2022

ariard commented at 5:08 pm on June 21, 2022: member

Updated at 6a8f6cd, on top of #25290 latest push. Style nit should have been addressed.

DrahtBot removed the label Needs rebase on Jun 21, 2022

DrahtBot added the label Needs rebase on Jun 27, 2022

Xekyo commented at 8:23 pm on June 28, 2022: member

Concept ACK

fanquake commented at 10:21 am on June 29, 2022: member

@ariard #25290 is now in. Could you re-rebase and update the PR description?

in src/init.cpp:561 in 6a8f6cd76c outdated

557@@ -555,6 +558,7 @@ void SetupServerArgs(ArgsManager& argsman)
558     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);
559     argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
560     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);
561+    argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without replaceability signaling inspection (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

luke-jr commented at 4:58 pm on June 29, 2022:

This still needs to be fixed to -mempoolreplacement=fee,-optin

ariard commented at 0:40 am on June 30, 2022:

Personally, I found the string arg confusing over a boolean. If we have more contributors weighting on -mempoolreplacement, I’ll think about a change.

If you comment is motivated as maintaining config file compatibility across full-node implementations, as I raised on IRC I think the project philosophy isn’t really clear on what to do. I invite you to open an issue to document the cross-implementation compatibility config file issues and gather more thoughts on that specific topic.

luke-jr changes_requested

in src/txmempool.cpp:461 in 6a8f6cd76c outdated

456+CTxMemPool::CTxMemPool(const Options& opts)
457+    : m_check_ratio(opts.check_ratio),
458+      minerPolicyEstimator(opts.estimator),
459+      m_max_size(opts.max_size_bytes),
460+      m_expiry(std::chrono::hours{opts.expiry}),
461+      m_full_rbf(opts.full_rbf),

luke-jr commented at 5:00 pm on June 29, 2022:

This seems like bad design. CTxMemPool should just store a copy of Options

ariard commented at 0:44 am on June 30, 2022:

This design is straightly inherited from #25290 where it has been given a chance to be discussed and reviewed extensively. I’ll invite you to let thoughts there and explain why storing a copy of Options is better. It can be addressed as a follow-up.

ariard force-pushed on Jun 30, 2022

ariard commented at 0:36 am on June 30, 2022: member

Rebased and updated at 72810ae4. This is ready for review.

DrahtBot removed the label Needs rebase on Jun 30, 2022

in doc/policy/mempool-replacements.md:80 in 72810ae4bf outdated

75@@ -74,3 +76,6 @@ This set of rules is similar but distinct from BIP125.
76 
77 * RBF enabled by default in the wallet GUI as of **v0.18.1** ([PR
78   #11605](https://github.com/bitcoin/bitcoin/pull/11605)).
79+
80+* Full replace-by-fee enabled as a configurable mempool policy as of **v.0.24.0** ([PR

MarcoFalke commented at 6:56 am on June 30, 2022:

0* Full replace-by-fee enabled as a configurable mempool policy as of **v24.0** ([PR

instagibbs commented at 2:11 pm on June 30, 2022:

concur (wish I could just ACK someone’s comment to note I mean the same thing…)

ariard commented at 3:35 am on July 1, 2022:

Updated.

concur (wish I could just ACK someone’s comment to note I mean the same thing…)

On my side, I generally do a thumbs up to convey I ACK someone’s comment.

in doc/policy/mempool-replacements.md:81 in 72810ae4bf outdated

75@@ -74,3 +76,6 @@ This set of rules is similar but distinct from BIP125.
76 
77 * RBF enabled by default in the wallet GUI as of **v0.18.1** ([PR
78   #11605](https://github.com/bitcoin/bitcoin/pull/11605)).
79+
80+* Full replace-by-fee enabled as a configurable mempool policy as of **v.0.24.0** ([PR
81+  #25353](https://github.com/bitcoin/bitcoin/pull/25290)).

MarcoFalke commented at 6:57 am on June 30, 2022:

wrong link?

ariard commented at 3:36 am on July 1, 2022:

Updated.

in src/txmempool.cpp:461 in 72810ae4bf outdated

457@@ -458,6 +458,7 @@ CTxMemPool::CTxMemPool(const Options& opts)
458       minerPolicyEstimator{opts.estimator},
459       m_max_size_bytes{opts.max_size_bytes},
460       m_expiry{opts.expiry},
461+      m_full_rbf(opts.full_rbf),

MarcoFalke commented at 6:59 am on June 30, 2022:

nit: Use {}?

MarcoFalke approved

MarcoFalke commented at 7:00 am on June 30, 2022: member

lgtm

MarcoFalke added this to the milestone 24.0 on Jun 30, 2022

in doc/release-notes.md:89 in 72810ae4bf outdated

85@@ -86,6 +86,10 @@ Changes to GUI or wallet related settings can be found in the GUI or Wallet sect
86 New settings
87 ------------
88 
89+- A new `mempoolfullrbf` has been added, which enables the mempool to

dergoegge commented at 10:00 am on June 30, 2022:

0- A new `mempoolfullrbf` option has been added, which enables the mempool to

in src/kernel/mempool_options.h:36 in 72810ae4bf outdated

32@@ -31,6 +33,7 @@ struct MemPoolOptions {
33     int check_ratio{0};
34     int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000};
35     std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}};
36+    bool full_rbf = DEFAULT_MEMPOOL_FULL_RBF;

dergoegge commented at 10:01 am on June 30, 2022:

0    bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF};

dergoegge commented at 10:22 am on June 30, 2022: member

Concept ACK

in src/init.cpp:561 in 088a4be466 outdated

557@@ -558,6 +558,7 @@ void SetupServerArgs(ArgsManager& argsman)
558     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);
559     argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
560     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);
561+    argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without replaceability signaling inspection (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

instagibbs commented at 2:05 pm on June 30, 2022:

mu-nit: “Accept transaction replace-by-fee without requiring replaceability signaling” is how I’d say it

in test/functional/feature_rbf.py:743 in 088a4be466 outdated

738+                sequence=SEQUENCE_FINAL,
739+                fee_rate=Decimal('0.02'),
740+        )
741+
742+        # Test if optout_tx can be replaced
743+        res = self.nodes[0].testmempoolaccept(rawtxs=[conflicting_tx['hex']])[0]

instagibbs commented at 2:10 pm on June 30, 2022:

is there a real benefit for testmempoolaccept vs submission? Would be nice to explicitly check that the first tx got booted from mempool rather than implicitly

ariard commented at 3:39 am on July 1, 2022:

Updated with sendrawtransaction and testing that optout_tx is not in the mempool after. On a more general note, I think for now it should be equivalent to test with either sendrawtransaction / testmempoolaccept though they might differ more as we those code paths start to be more special-cased with package/set of transactions validation.

instagibbs commented at 2:11 pm on June 30, 2022: member

looks good, just a few minor cleanups left

ariard force-pushed on Jul 1, 2022

ariard commented at 3:40 am on July 1, 2022: member

Updated at bb3d7ea

All last review comments should have been addressed.

MarcoFalke commented at 6:28 am on July 1, 2022: member

cr ACK bb3d7ea1124fe642a10900ecb83ba88c544291e8

in src/rpc/mempool.cpp:687 in bb3d7ea112 outdated

683@@ -683,6 +684,7 @@ static RPCHelpMan getmempoolinfo()
684                 {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
685                 {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or BIP 125 replacement in " + CURRENCY_UNIT + "/kvB"},
686                 {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
687+                {RPCResult::Type::BOOL, "mempoolfullrbf", "True if the mempool accepts RBF without replaceability signaling inspection"},

MarcoFalke commented at 6:29 am on July 1, 2022:

nit: if you retouch could rename to “fullrbf”, as getmempoolinfo should already be clear enough that the fields are about the mempool.

ariard commented at 3:03 pm on July 5, 2022:

modified

in test/functional/feature_rbf.py:744 in 7c1938e8bf outdated

739+                fee_rate=Decimal('0.02'),
740+        )
741+
742+        # Test if optout_tx can be replaced
743+        self.nodes[0].sendrawtransaction(conflicting_tx['hex'], 0)
744+        #res = self.nodes[0].testmempoolaccept(rawtxs=[conflicting_tx['hex']])[0]

naumenkogs commented at 8:51 am on July 5, 2022:

Here and below, the commented LOC should be deleted?

ariard commented at 3:03 pm on July 5, 2022:

Yep, deleted

ariard force-pushed on Jul 5, 2022

ariard commented at 3:09 pm on July 5, 2022: member

Updated at 36870cf

All last review comments should have been addressed.

MarcoFalke commented at 3:12 pm on July 5, 2022: member

recr ACK 36870cfda5ffcfebfbec41eba092b9ec1faf4c58

instagibbs commented at 3:22 pm on July 5, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/25353/commits/36870cfda5ffcfebfbec41eba092b9ec1faf4c58

naumenkogs commented at 7:50 am on July 6, 2022: member

Code review ack 36870cfda5ffcfebfbec41eba092b9ec1faf4c58

in src/validation.cpp:746 in bab505e71c outdated

739@@ -740,7 +740,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
740                 // Applications relying on first-seen mempool behavior should
741                 // check all unconfirmed ancestors; otherwise an opt-in ancestor
742                 // might be replaced, causing removal of this descendant.
743-                if (!SignalsOptInRBF(*ptxConflicting)) {
744+                //
745+                // If replaceability signaling is ignored due to node setting,
746+                // replacement is always allowed.
747+                if (!SignalsOptInRBF(*ptxConflicting) && !m_pool.m_full_rbf) {

darosior commented at 10:27 am on July 6, 2022:

nit: maybe evaluate !m_pool.m_full_rbf first to avoid iterating over the txins if not necessary.

ariard commented at 0:59 am on July 7, 2022:

Done.

darosior commented at 10:29 am on July 6, 2022: member

utACK 36870cfda5ffcfebfbec41eba092b9ec1faf4c58

in src/kernel/mempool_options.h:18 in bab505e71c outdated

14@@ -15,6 +15,8 @@ class CBlockPolicyEstimator;
15 static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300};
16 /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
17 static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336};
18+/** Default for -mempoolfullrbf, if the transaction replaceability signaling is enforced */

mzumsande commented at 6:52 pm on July 6, 2022:

nit: the comment says “if (…) signaling is enforced”, but the bool is true when signalling is ignored and false if it’s enforced.

ariard commented at 0:59 am on July 7, 2022:

Let me know if not good with new wording “s/enforced/ignored/g”.

Introduce `mempoolfullrbf` node setting.

This new node policy setting enables to accept replaced-by-fee
transaction without inspection of the replaceability signaling
as described in BIP125 "explicit signaling".

If turns on, the node mempool accepts transaction replacement
as described in `policy/mempool-replacements.md`.

The default setting value is `false`, implying opt-in RBF
is enforced.

3e27e31727

Update getmempoolinfo RPC with `mempoolfullrbf` aae66ab43d

Mention `mempoolfullrbf` in policy/mempool-replacements.md 4c9666bd73

ariard force-pushed on Jul 7, 2022

ariard commented at 1:00 am on July 7, 2022: member

Updated at 4c9666b.

All last review comments should have been addressed.

in doc/policy/mempool-replacements.md:18 in 4c9666bd73

14@@ -15,6 +15,8 @@ other consensus and policy rules, each of the following conditions are met:
15 
16    *Rationale*: See [BIP125
17    explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).
18+   The Bitcoin Core implementation offers a node setting (`mempoolfullrbf`) to allow transaction

glozow commented at 9:42 am on July 7, 2022:

nit in 4c9666bd73645b94ae81be1faf7a0b8237dd6e99, since this is in the Bitcoin Core documentation

0   Use the `-mempoolfullrbf` setting to allow transaction

in test/functional/feature_rbf.py:726 in 4c9666bd73

721+        txid = self.wallet.send_self_transfer(from_node=self.nodes[0])['txid']
722+        self.generate(self.nodes[0], 1)
723+        confirmed_utxo = self.wallet.get_utxo(txid=txid)
724+
725+        self.restart_node(0, extra_args=["-mempoolfullrbf=1"])
726+

glozow commented at 9:44 am on July 7, 2022:

in aae66ab43d794bdfaa2dade91760cc55861c9693, it could make sense to add this line so we have coverage for the new RPC result?

0        assert self.nodes[0].getmempoolinfo()["fullrbf"]

in test/functional/feature_rbf.py:731 in 4c9666bd73

726+
727+        # Create an explicitly opt-out transaction
728+        optout_tx = self.wallet.send_self_transfer(
729+            from_node=self.nodes[0],
730+            utxo_to_spend=confirmed_utxo,
731+            sequence=SEQUENCE_FINAL,

glozow commented at 9:59 am on July 7, 2022:

tiny nit in 3e27e317270fdc2dd02794fea9da016387699636, I think BIP125_SEQUENCE_NUMBER would be a bit more explicit

0            sequence=BIP125_SEQUENCE_NUMBER + 1,

in doc/release-notes.md:91 in 4c9666bd73

85@@ -86,6 +86,10 @@ Changes to GUI or wallet related settings can be found in the GUI or Wallet sect
86 New settings
87 ------------
88 
89+- A new `mempoolfullrbf` option has been added, which enables the mempool to
90+  accept transaction replacement without enforcing the opt-in replaceability
91+  signal. (#25353)

glozow commented at 10:12 am on July 7, 2022:

nit in 4c9666bd73645b94ae81be1faf7a0b8237dd6e99, Just in case we add another way of signaling replaceability…

0  accept transaction replacement without enforcing BIP125 opt-in replaceability
1  signal. (#25353)

glozow commented at 10:15 am on July 7, 2022: member

ACK 4c9666bd73645b94ae81be1faf7a0b8237dd6e99, a few nits which are non-blocking.

instagibbs commented at 1:04 pm on July 7, 2022: member

reACK https://github.com/bitcoin/bitcoin/pull/25353/commits/4c9666bd73645b94ae81be1faf7a0b8237dd6e99

in test/functional/feature_rbf.py:723 in 4c9666bd73

716@@ -714,5 +717,33 @@ def test_replacement_relay_fee(self):
717         tx.vout[0].nValue -= 1
718         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
719 
720+    def test_fullrbf(self):
721+        txid = self.wallet.send_self_transfer(from_node=self.nodes[0])['txid']
722+        self.generate(self.nodes[0], 1)
723+        confirmed_utxo = self.wallet.get_utxo(txid=txid)

MarcoFalke commented at 2:19 pm on July 7, 2022:

Sorry for the merge conflict, but now you might be able to write this a bit shorter:

0        confirmed_utxo = self.make_utxo(...)

Let me know if you have any questions.

MarcoFalke commented at 3:44 pm on July 7, 2022:

Oh nvm. There is no merge conflict. I can fix this up after merge, if needed.

in test/functional/feature_rbf.py:746 in 4c9666bd73

741+
742+        # Send the replacement transaction, conflicting with the optout_tx.
743+        self.nodes[0].sendrawtransaction(conflicting_tx['hex'], 0)
744+
745+        # Optout_tx is not anymore in the mempool.
746+        assert optout_tx['txid'] not in self.nodes[0].getrawmempool()

w0xlt commented at 5:39 pm on July 7, 2022:

nit: this tests whether the conflicting_tx is in the mempool.

0        assert optout_tx['txid'] not in self.nodes[0].getrawmempool()
1        assert conflicting_tx['txid'] in self.nodes[0].getrawmempool()

w0xlt approved

w0xlt commented at 5:40 pm on July 7, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25353/commits/4c9666bd73645b94ae81be1faf7a0b8237dd6e99

Rspigler commented at 8:38 pm on July 7, 2022: contributor

Concept ACK. Full RBF is needed to solve a number of security concerns with second layer protocols.

I don’t see how the complaint about this affecting 0-conf transactions is valid, since it was never secure to accept 0-conf txs, and doing this makes LN more secure, which is what you should be doing for those use cases anyway.

ariard commented at 1:59 am on July 8, 2022: member

Since there are already a number of ACKs on this PR tip, from this point I’m likely not going to re-push to address non-blocking comments and as such I hope to save review time. However, I’m aiming to address the nits in a follow-up PR. If you have more blocking comments, let me know to answer or address them.

MarcoFalke merged this on Jul 8, 2022

MarcoFalke closed this on Jul 8, 2022

MarcoFalke commented at 3:34 pm on July 8, 2022: member

For reference, this was my test nit (applied and passed locally for me):

 0diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
 1index 8e5cbba01a..85b41d0a89 100755
 2--- a/test/functional/feature_rbf.py
 3+++ b/test/functional/feature_rbf.py
 4@@ -702,10 +702,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
 5         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
 6 
 7     def test_fullrbf(self):
 8-        txid = self.wallet.send_self_transfer(from_node=self.nodes[0])['txid']
 9-        self.generate(self.nodes[0], 1)
10-        confirmed_utxo = self.wallet.get_utxo(txid=txid)
11-
12+        confirmed_utxo = self.make_utxo(self.nodes[0], int(2 * COIN))
13         self.restart_node(0, extra_args=["-mempoolfullrbf=1"])
14 
15         # Create an explicitly opt-out transaction

sidhujag referenced this in commit a3e94ca62e on Jul 8, 2022

ariard referenced this in commit 3139f90a87 on Jul 8, 2022

ariard referenced this in commit 6a547cbf8a on Jul 8, 2022

ariard commented at 11:53 pm on July 8, 2022: member

Thanks @glozow @w0xlt @MarcoFalke for the reviews, your remaining comments here should have been addressed in #25575, though let me know if I missed or misunderstood any of them.

in doc/policy/mempool-replacements.md:80 in 4c9666bd73

75@@ -74,3 +76,6 @@ This set of rules is similar but distinct from BIP125.
76 
77 * RBF enabled by default in the wallet GUI as of **v0.18.1** ([PR
78   #11605](https://github.com/bitcoin/bitcoin/pull/11605)).
79+
80+* Full replace-by-fee enabled as a configurable mempool policy as of **v24.0** ([PR

Xekyo commented at 4:18 pm on July 11, 2022:

I think it would be better to refer to this as “general full RBF” in comparison to “opt-in full RBF”. The “full” in both cases stands for how much of the transaction can be replaced: we allow full replacement of the transaction instead of requiring that the replacement transaction recreating all outputs the original transaction had (which was a competing approach back then). And in the default behavior, we allow full replacement of transactions that opted-in, while now nodes may also permit full replacement generally.

0* General full replace-by-fee enabled as a configurable mempool policy as of **v24.0** ([PR

glozow commented at 8:34 am on July 12, 2022:

Maybe @petertodd would be able to help us clarify terminology? How do you think of the term “full,” and do you remember what you meant by “full-RBF opt-in” in #6871?

I interpret “full” RBF policy to mean “no signaling required,” i.e. the opposite of “opt-in.”

~Seems @Xekyo is referring to “full” replacement by a wallet meaning “using all the inputs from the original transaction."~ edit: I misunderstood, and now see that he’s describing the opposite of first-seen-safe.

sipa commented at 10:35 am on July 12, 2022:

I believe the existing “full (opt-in) RBF” was named so because of its contrast to an earlier proposal of “first-seen-safe RBF”.

glozow commented at 11:19 am on July 12, 2022:

Oh I see about the FFS vs full definition. Is there any chance there are two meanings https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-January/013471.html ?

Xekyo commented at 3:21 pm on July 12, 2022:

After more discussion here and the PR for the upcoming Optech newsletter, I retract my suggestion. I changed my opinion because I realized that FSS RBF is no longer on top of people’s mind and from today’s perspective “opt-in RBF” and “full RBF” are sufficiently established that it’s clear what is meant.

glozow commented at 3:36 pm on July 12, 2022:

Ok sounds good! Marking as resolved for now, thanks

ariard referenced this in commit 1056bbdfcd on Jul 11, 2022

glozow referenced this in commit 39d111aee7 on Jul 12, 2022

sidhujag referenced this in commit df20d867a8 on Jul 12, 2022

MarcoFalke locked this on Jul 24, 2022

Add a -mempoolfullrbf node setting #25353

Conflicts

Add a `-mempoolfullrbf` node setting #25353