Add a -mempoolfullrbf node setting

[ariard]

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 : Add a -mempoolfullrbf node setting #25353 (comment)
• p2p discovery and additional outbound connection to full-rbf peers : Add a -mempoolfullrbf node setting #25353 (comment)
• follow-ups: Address comments remaining from #25353 #25575

[maflcko]

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?

[michaelfolkson]

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]

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]

Concept ACK

[naumenkogs]

Concept ACK. I think this change is immediately good.

[instagibbs]

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

[dunxen]

Concept ACK

[benthecarman]

Concept ACK

[ariard]

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]

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

[maflcko]

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]

Concept ACK

[luke-jr]

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

[ghost]

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]

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]

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.

[maflcko]

recr ACK 36870cf

[instagibbs]

ACK 36870cf

[naumenkogs]

Code review ack 36870cf

[darosior]

utACK 36870cf

[mzumsande]

nit: the comment says "if (...) signaling is enforced", but the bool is true when signalling is ignored and false if it's enforced.

[ariard]

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

[ariard]

Updated at 4c9666b.

All last review comments should have been addressed.

[glozow]

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

[instagibbs]

reACK 4c9666b

[w0xlt]

ACK 4c9666b

[Rspigler]

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]

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.

[maflcko]

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

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

[ariard]

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.