Skip to content

policy: Enable full-rbf by default #28132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

petertodd
Copy link
Contributor

@petertodd petertodd commented Jul 23, 2023

The following pools are have enabled full-rbf:

...and others. The above list is 90%+ of total hash power.

Frankly, I can't think of another time in Bitcoin's history where a supermajority of hash power had chosen to change Bitcoin Core's default mempool policy in the same way.

Enabling full-RBF by default is well overdue. Miners have chosen to mine it on a large scale, it simplifies mempool logic, and it makes certain types of DoS attacks on multiparty protocols significantly more expensive so there's no reason to continue to try to prevent full-RBF replacements from getting to miners.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 23, 2023

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK levantah
Concept NACK BitcoinErrorLog, ChrisMartl, viresinnumeris-1
Concept ACK jaonoctus, luke-jr, hsjoberg, ghost, ekzyis, Symphonic3, russeree, Kruwed, cryptoquick, nvk, michaelfolkson, brandonblack, pox, kilrau, MaxHillebrand, knocte
Stale ACK ariard
Ignored review fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28676 ([WIP] Cluster mempool implementation by sdaftuar)

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.

@petertodd
Copy link
Contributor Author

FYI mempool.space has enabled full-rbf by default, among many others.

@emc99
Copy link

emc99 commented Jul 23, 2023

When you say "by default", do you mean that full-rbf would come by default as part of IBD or when you update Bitcoin Core? When would full-rbf be "by default"?

@petertodd
Copy link
Contributor Author

When you say "by default", do you mean that full-rbf would come by default as part of IBD or when you update Bitcoin Core? When would full-rbf be "by default"?

This pull-req has nothing to do with Initial Block Download (IBD).

It simply changes the default for the -mempoolfullrbf option to true/enabled. Previously the default was false/disabled. Users who update Bitcoin Core to a version containing this change would by default propagate and mine full-rbf replacements unless they had chosen to disable full-rbf.

A significant fraction of the P2P network has already chosen to enable full-rbf, so full-rbf replacements propagate reasonably well. Almost half of pools by hash power has enabled full-rbf, so full-rbf replacements that reach those miners are readily mined, as seen on https://mempool.space/rbf#fullrbf and https://fullrbf.mempool.observer/

@ariard
Copy link

ariard commented Jul 23, 2023

On the first line of arguments, I think zero-conf business acceptance have the option to deploy additional full-nodes with good transaction-relay peering to obtain a reasonable view of network mempools, and therefore increase their odds of seeing a double-spend of a confirmation of interest. In practice, zero-conf applications have risk threshold, once those thresholds are reached they will deactivate zero-conf acceptance.

On the second line or arguments, mempool consistency with miners is becoming a practical concern in my opinion with the appereance of so-called “transaction accelerators”, as now LSPs can use those out-of-band transaction-relay service to fee-bump their stuck multi-party transactions, and therefore provoke a divergence with the rest of the network. I raised this risk of “mining income asymmetries due to unequal access in to transaction flows bypassing policies” in Suhas proposal to remove the mempoolfullrbf option months ago.

On the third line of arguments about the enhancement of multi-party funded transactions flows, the original motivation is explained in this post on the lightning dev mailing list. RBF opt-out allows a counterparty contributing an input to block the confirmation of a multi-party transaction at minimal cost, therefore enabling a liquidity griefing at low-cost (size of the pinning tx * mempool min fee).

Since then, early Lightning Service Providers deploying dual-funding have raised again the concern on the mailing list during May of this year, and how sustained RBF signaling flag opt-out can be an annoying vector of pinning attack. While from my understanding the issue might be solved on the Lightning-side by upcoming deployment of nversion=3 (where full-rbf is implied by the policy regime), this won’t solve the pinning issue for coinjoin multi-party transactions which are concerned too (and this is not sure that we’re deploying nversion=3 for the dual-funding/splicing flows due to package limits size). With that context in mind, full-rbf by default is a welcome deployment for second-layers and multi-party applications.

On the deployment schedule, we’re still mid-26.0 schedule, so I think it’s reasonable to land it now (feature freeze scheduled for 2023-10-01 as of today July 24th). This still gives a period of roughly 5 months to zero-conf business and other software that would need to tweak their software.

37% of the hash power sounds a reasonable demonstration of the Bitcoin mining ecosystem selecting the transaction-relay policy favoring maximization of their mining income strategy.

So I’m Concept ACK on this change, I still think this change deserves announcement on the mailing list and usual technical communication channels to warn ecosystem stakeholders impacted by the proposed change.

@recursive-rat4
Copy link

I queried the explorer like this

~ $ curl -s https://mempool.space/api/v1/block/0000000000000000000153159d7b95debfb0dadcd1040aaf9dbeb0025a1ddeac/audit-summary | jq .fullrbfTxs
[
  "53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643",
  "5bc64344c56f847e2d992fab241567075473eec9423776afadc187299352bce1",
  "64ec51dedd1404a775d590f530a01bbdc4239c8eb6d33ce4b9217ec2f0b8ddae"
]

Only 4 of 6 mentioned transactions seem to be full-rbf, or was it a wrong request?

@DrahtBot DrahtBot changed the title Enable full-rbf by default policy: Enable full-rbf by default Jul 24, 2023
@petertodd
Copy link
Contributor Author

petertodd commented Jul 24, 2023 via email

Copy link

@jaonoctus jaonoctus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept nACK

That's 37% of total hash power regularly mining full-rbf transactions.

I think this decision is premature and that is not enough for the current value to be changed, honestly. Still prefer it as opt-in instead of enabled by default ATM

Let's get this merged and this silly debate over with. Miners and node operators who choose to disable full-rbf still can with a simple configuration change.

Let's get this not merged and this silly debate over with. Miners and node operators who choose to enable full-rbf still can with a simple configuration change.

@luke-jr
Copy link
Member

luke-jr commented Jul 25, 2023

Concept ACK, but the description has some issues:

Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.

It was always invalid.

Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners.

This is backward. Miners are incentivised to match nodes. Nodes shouldn't try to match miners.

@petertodd
Copy link
Contributor Author

Concept ACK, but the description has some issues:

Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.

It was always invalid.

Reworded it to say "even more clearly invalid"

Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners.

This is backward. Miners are incentivised to match nodes. Nodes shouldn't try to match miners.

That's probably true for consensus changes. But this isn't a consensus change. For full-rbf 1) miners can run their own nodes, 2) sufficient full-rbf nodes are running that miners can fairly reliably receive full-rbf replacements.

The status quo is already that full-rbf replacements are regularly mined and can be propagated with a little bit of work. This change merely accepts that fact, and (marginally) improves the propagation situation for small miners, and makes full-rbf more convenient to use for everyone else.

@hsjoberg
Copy link
Contributor

hsjoberg commented Jul 29, 2023

Concept ACK.
This is helpful for second layer protocols, including Lightning.

@sandakersmann
Copy link
Contributor

What happened to: "It's just opt-in"?

@petertodd
Copy link
Contributor Author

petertodd commented Jul 29, 2023 via email

@sandakersmann
Copy link
Contributor

@petertodd Why didn't any hash power opt-in to RBF back in 2015?

@petertodd
Copy link
Contributor Author

petertodd commented Jul 29, 2023 via email

@sandakersmann
Copy link
Contributor

@petertodd Why didn't any hash power opt-in to RBF before you made that patch?

@ghost
Copy link

ghost commented Jul 30, 2023

Concept ACK

There are no answers for questions that do not make sense. Also there are no arguments left against full RBF as it's used regularly.

@YellowRoseCx
Copy link

Replace by fee makes double spending easier and harm's Bitcoin's ability to be used as a currency in my opinion

@ekzyis
Copy link

ekzyis commented Jul 30, 2023

Concept ACK

Enabling full-RBF by default removes remaining false sense of security

@petertodd
Copy link
Contributor Author

@sandakersmann

@petertodd Why didn't any hash power opt-in to RBF before you made that patch?

Someone had to actually write the code of course. AFAIK I was the first (though as far as I know, rbf was first suggested by Satoshi).

@SparK-Cruz
Copy link

The fact RBF is possible already undermines the security of the mempool, which was already low, so having it on by default or not, the fact it exists already killed zero-conf a long time ago.

@petertodd
Copy link
Contributor Author

@ariard

So I’m Concept ACK on this change, I still think this change deserves announcement on the mailing list and usual technical communication channels to warn ecosystem stakeholders impacted by the proposed change.

I've posted a notice on the bitcoin-dev mailing list: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-July/021823.html

Thanks for the Concept ACK!

@sandakersmann
Copy link
Contributor

First-seen-safe is good enough for grocery shopping. But don't let me disturb you while you strip away the utility of BTC.

@TheBlueMatt
Copy link
Contributor

This is helpful for second layer protocols, including Lightning.

I'm not aware of any second layer protocols that are improved by having full-rbf more broadly available.

@ariard
Copy link

ariard commented Jun 20, 2024

Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.

This is good information to have. I’ll recall the original motivation which was behind introducing mempoolfullrbf was removing a zero-cost transaction-relay jamming affecting multi-party transaction broadcast (cf. “On Mempool Funny Games against Multi-Party Funded Transactions”).

I’ll observe that since then collaborative transaction construction, the v2 open channel flow has been merged in lightning specification (bolt PR 851, so more and more multi-party application are at risk of being DoSed, if the associated full-node has a low-topology of mempoolfullrbf=true nodes.


Reviewed code change at 512278e.

@petertodd
Copy link
Contributor Author

Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.

This is good information to have. I’ll recall the original motivation which was behind introducing mempoolfullrbf was removing a zero-cost transaction-relay jamming affecting multi-party transaction broadcast (cf. “On Mempool Funny Games against Multi-Party Funded Transactions”).

Yes, my Libre Relay implementation of replace-by-fee-rate always enables full-rbf as it's the other relevant transaction pinning vector. And while Core intends to ship V3 transactions, it's a very narrow transaction pinning mitigation. For example, it does nothing to help SIGHASH_ANYONECANPAY transactions or coinjoins. Enabling full-RBF in Core would at least be a start to mitigating these issues.

I’ll observe that since then collaborative transaction construction, the v2 open channel flow has been merged in lightning specification (bolt PR 851, so more and more multi-party application are at risk of being DoSed, if the associated full-node has a low-topology of mempoolfullrbf=true nodes.

Agreed.

Reviewed code change at 512278e.

Thanks! You might want to formally Tested ACK it so the bot picks it up.

@levantah
Copy link

I would just like to note that @fjahr is counted as NACK by mistake (read fully his comment which is just mentioning that the other he is replying to can "give a NACK").

Tested ACK as I run fullrbf nodes aver since the configuration option appeared and this change just chenges the default.

@levantah
Copy link

levantah commented Jun 21, 2024

I saw also my previous comment was counted wrong. But this fixes it:

Tested ACK

@fjahr
Copy link
Contributor

fjahr commented Jun 21, 2024

I would just like to note that @fjahr is counted as NACK by mistake (read fully his comment which is just mentioning that the other he is replying to can "give a NACK").

I didn't notice, I gave @DrahtBot a 👎 so it should be removed.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK 512278e

Manual mutation testing with following diff:

diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index 4e1e24a11d..0850b2e60e 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -22,7 +22,7 @@ static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5};
 /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
 static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336};
 /** Default for -mempoolfullrbf, if the transaction replaceability signaling is ignored */
-static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{true};
+static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{false};
 /** Whether to fall back to legacy V1 serialization when writing mempool.dat */
 static constexpr bool DEFAULT_PERSIST_V1_DAT{false};
 /** Default for -acceptnonstdtxn */
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 5e722724b9..6f11683a9c 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -162,7 +162,6 @@ class MempoolAcceptV3(BitcoinTestFramework):
         self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]])
 
 
-    @cleanup(extra_args=["-mempoolfullrbf=0"])
     def test_v3_bip125(self):
         node = self.nodes[0]
         self.log.info("Test v3 transactions that don't signal BIP125 are replaceable")

On analyzing tangential mempool behaviors hypothetically affected by setting mempoolfullrbf=1 by default:

(a) Wtxid: nsequence signaling is encompassed both in txid and wtxid. After this change and assuming reboot a local mempool can re-accept a bip125 opt-out transaction previously rejected as identical non-witness data might not have change, yet the wtxid being different.

(b) Mempool.dat / persistence: the dump of mempool transaction. Dump of transactions are expected to be altered with mempoolfulrbf=1 as bip125 opt-out transactions are expected to be accepted with this change.

(c) Datacarrier: one op_return data carrying output. The data carriage is implemented in the scriptpubkey while the signaling rbf mechanism is implemented in one of the input. There is no effect expected on data carriage mempool acceptance, unless very exotic usage of sighash malleability, where the txid can be altered yet the hash of the op_return scriptpubkey stays identical.

(d) Dust: the dust satoshi threshold. The dust check is implemented on the output amounts while the signaling rbf mechanism is implemeted in one of the input. There is no effect expected on data carriage mempool acceptance, unless very exotic usage of sighash malleability, where the txid can be altered yet the hash of one of the output amount under the dust threshold stays identical.

(e) Mempool expiry: by default after 14 days transactions are sorted out. Transactions accepted under mempoolfullrbf=0 and still in the mempool until reaching expiration could be replaced by an opt-out transactions. I think this a transient risk of double-spend during the deployment of mempoolfullrbf=1 as a default.

(f) Mempool limits / RBF carve-out. When mempool limits are reached out (25 descendants), there is an exemption to get one more child to be connected on the parent. As bip125 inheritance is not implemented in core, the one more opt-out child could be re-accepted in the mempool. I think this is a transient risk of double-spend during the deployment of mempoolfullrbf=1 as a default.

(g) Package initial acceptance / RBF. In-mempool packages are submitted to descendants and ancestors limits. As bip125 inheritance is not implemented in core, a subset of child could be re-accepted in the mempool. I think this is a transient risk of double-spend during the deployment of mempoolfullrbf=1 as a default.

(h) Mempool Reorg. Signaling mechanism is implemented on the nSequence field. In case of chain reorgs, the pool of disconnected transactions can be re-added in mempool. There is no effect expected on disconnected transactions re-acceptance.

(i) Mempool sigops limits. Script sig ops limits are evaluated on the scriptSig / witness spend. This mempool limit check only happens once a transaction is good for acceptance and scripts have to be validated. An opt-out transaction can be accepted for the same sig ops budget, I think this a transient genuine risk of opportunistic full-node ressource consumption during the deployment of mempoolfullrbf=1 as a default.

(j). Mempool unbroadcast. Transaction delivery is ensured by re-broadcasting transactions until a GETDATA is received. After this change and assuming reboot a local mempool can re-accept a bip125 opt-out transaction previously rejected
and keep re-announcing this transaction to non-upgraded peers, provoking a free transaction relay bandwidth. I think this a transient minor risk of bandwidth ressource denial-of-service during the deployment of mempoolfullrbf=1 as a default.

@petertodd petertodd force-pushed the 2023-07-enable-full-rbf branch from 512278e to fbdc61e Compare July 5, 2024 07:32
Copy link

@levantah levantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fbdc61e

@DrahtBot DrahtBot requested a review from ariard July 5, 2024 15:56
@willcl-ark
Copy link
Member

Thank everyone for your participation in this discussion and review of this pull request so far.

However the comments section here has become difficult to follow due to numerous off-topic comments, a few personal disagreements, and repetition of arguments.

In the interest of having a more productive and focused technical and philosophical discussion we are going to close and lock this PR.


@petertodd: We strongly encourage you to open a new pull request for this proposal, as the intention of closing this is not to close down the change request itself, but rather make the commentary easier to follow, and more on-topic, for fellow reviewers and contributors.

In the new PR, please do link to this current PR (#28132) for historical context, this will ensure we do not lose the link between useful review and discussion which has already taken place here, in the new PR.


Contributors: When the new PR is opened, please keep the discussion focused on technical aspects, avoid personal attacks, and refrain from repeating arguments without adding new information. This will help maintain a more constructive and efficient review process.

This action is in line with our moderation guidelines, which aim to keep discussions on-topic, respectful, and constructive.

@willcl-ark willcl-ark closed this Jul 12, 2024
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.