Given #30493 and the related discussion on network uptake it’s probably not helpful to have an option for a feature that will not be respected by the network in any meaningful way.
Wallet changes can be done in another PR on its own cadence to account for possible fingerprinting, waiting for fullrbf logic to permeate the network, etc.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30592.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, murchandamus, rkrux, achow101 |
| Concept ACK | 1440000bytes |
| Stale ACK | petertodd |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
permitbaremultisig and maxdatacarriersize in getmempoolinfo by kristapsk)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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28374307572
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
I’d suggest waiting until after v28 has been released to do this. There may be a handful of users remaining that are not vulnerable to double spends in general. But run software that is incompatible with full-RBF. For example, the Wasabi coordinator software used to be an example of this, which they noticed when they were running with Knots (long since fixed).
Perfectly reasonable to give those handful of users a bit more time to fix their software.
But other than that, Concept ACK. Might as well rip out all that obsolete code eventually.
I think mempoolfullrbf could be left in core, if some users wishes to have first-seen of received transactions. E.g for lightning type option_zeroconf where double spend can only happen if (a) funding keys leak which is a bigger problem or (b) double-spend by the the zero-conf inbound conf seller, though here the paid invoices can be used as fraud proofs.
Ultimately, it’s a wider conversation to have mempool policy being more configurable in its own module as jtimon tried to implement years years ago, and let the end-user evaluate with more fine-granularity its own security risks, so I’m ~0 if mempoolfullrbf got removed or not.
@ariard First of all, with ~100% of hash power mining full-rbf, mempoolfullrbf=0 clearly provides no security benefit.
Users may have their own reasons to want a record of the first-seen transaction. But there’s no reason why we in Core need to support such a niche use-case. It would make much more sense for users who need that functionality to get it directly, eg with some kind of “incoming transaction feed” hook.
For the use-case of zeroconf channels, I don’t see how mempoolfullrbf=0 is relevant. Either the user’s software has seen the first transaction, and thus has a record of it, or it doesn’t, in which case the channel might as well have never been created. Lightning node software certainly doesn’t need to rely on Core to record and create a proof-of-doublespend in this circumstance.
@murchandamus feel free to open the deprecation PR 👍 I’m leaving as draft as noted in OP until post-28
I am hearing that someone else is already working on that. :)
@petertodd On the lightning side, in a world where as a node you have a scarcity of UTXO to allocate seeing locally a record of transaction-relay policy and consensus transaction, it’s very useful to start to have (unsafe indeed) HTLC flowing or fee-bump the transaction to increase funds velocity.
I’m not insisting on keeping mempoolfullrbf. It’s more very likely that your first-seen log of transaction to monitor double-spend will ressemble an in-memory pool of transactions and as your computing host ressources are limited that pool has to be size bounded, yet a “first-seen” acceptance policy (any ulterior incoming transactions, if you take actions on it like CPFP fee-bumping which locks UTXO it’s consuming ressources).
Anyway, as I was saying it’s a wider conversation if lightning nodes shall have a more custom log of seen transactions or mempool. If we see controversies again on “first-seen” as a transaction-relay policy in the few years, at least I would have warned so now :)
678@@ -679,7 +679,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
679 ret.pushKV("minrelaytxfee", ValueFromAmount(pool.m_opts.min_relay_feerate.GetFeePerK()));
680 ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
681 ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
682- ret.pushKV("fullrbf", pool.m_opts.full_rbf);
683+ ret.pushKV("fullrbf", true);
RPC doc-nit: Could mention this field is “deprecated”, because it returns a constant and only kept for historic reasons to avoid changing the RPC interface now?
There is almost no cost to keeping it, but I think at some point in the future, it can be removed.
Same for the bip125-replaceable fields, which also only have historic value to keep around a bit, but can be removed at some point into the future.
bip125-replaceable code from the wallet at some point. There’s quite a bit of complexity there tracking it; I was just looking at run_rbf_opt_in_test in test/functional/wallet_listtransactions.py myself for Libre Relay.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30886829600
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Have you seen https://github.com/bitcoin/bitcoin/pull/30592/files#r1710942068 ?
I am now thinking this should probably be done in this pull request. Otherwise there will be another follow-up for a trivial doc update.
841- const bool allow_rbf{m_pool.m_opts.full_rbf || SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION};
842- if (!allow_rbf) {
843- return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
844- }
845-
846- ws.m_conflicts.insert(ptxConflicting->GetHash());
tACK https://github.com/bitcoin/bitcoin/commit/f863f5a4f991ccfdf865d39c73269b196eeb4902
Make and functional tests are successful.
28@@ -29,15 +29,13 @@ def set_test_params(self):
29 # both nodes disable full-rbf to test BIP125 signaling
111@@ -115,7 +112,6 @@ def test_truc_replacement(self):
112 from_node=node,
113 fee_rate=DEFAULT_FEE,
114 utxo_to_spend=utxo_v3_bip125,
115- sequence=MAX_BIP125_RBF_SEQUENCE,
158@@ -163,30 +159,6 @@ def test_truc_replacement(self):
159 self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]])
160
161
162- @cleanup(extra_args=["-mempoolfullrbf=0"])
163- def test_truc_bip125(self):
164- node = self.nodes[0]
165- self.log.info("Test TRUC transactions that don't signal BIP125 are replaceable")
166- assert_equal(node.getmempoolinfo()["fullrbf"], False)
~0 on keeping it, we’re reducing loc and don’t really need the coverage anymore imo
Willing to be over-ridden by other reviewers.
162 rawtxs=[raw_tx_0],
163 )
164
165- self.log.info('A transaction that conflicts with an unconfirmed tx')
166- # Send the transaction that replaces the mempool transaction and opts out of replaceability
167+ # Send the transaction that replaces the mempool transaction
self.log.info('A transaction that replaces a mempool transaction')
mempoolfullrbf was actually introduced back in 24.0.1.
0@@ -0,0 +1,4 @@
1+Full Replace-By-Fee
2+===================
3+
4+`mempoolfullrbf` option is removed, with full rbf the fixed behavior.
nit: suggestion to provide a bit more context. Ideally, at least the PR number is added.
0Starting with v28.0, the `mempoolfullrbf` startup option was set to
1default to `1`. With widespread adoption of this policy, users no longer
2benefit from disabling it, so the option has been removed, making full
3replace-by-fee the standard behavior. (#30592)
9@@ -10,7 +10,9 @@ A transaction ("replacement transaction") may replace its directly conflicting t
10 their in-mempool descendants (together, "original transactions") if, in addition to passing all
11 other consensus and policy rules, each of the following conditions are met:
12
13-1. If `-mempoolfullrbf=0` (the value is 1 by default), the directly conflicting transactions all signal replaceability explicitly. A transaction is
14+1. DEPRECATED: BIP125 signaling is no longer required. The below text is kept for historical completeness.
## History section?
Ah, I see. We don’t mention BIP125 until further down the document, so I don’t think (most) readers are expecting that symmetry here, so I still think it’d least awkward to just remove the item entirely (and renumbering the following items).
We’re in bikeshed territory, so I’ll stop commenting on it further.
Edit: sorry, hit send before finishing review. Will finalize later today, so more comments may follow.
Note: doc/bips.md needs updating, “opt-in” is no longer accurate: https://github.com/bitcoin/bitcoin/blob/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001/doc/bips.md?plain=1#L37
ACK d295638890719a009b480148545288e2dbdd1a5a - although I’ll quickly re-ACK if you address my outstanding comments too.
Nice clean-up, both for users and the codebase, I don’t see the point of keeping this option around any longer.
if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares.
I don’t think we typically announce all backwards-incompatible changes on the ML? Since this was marked as deprecated (+ raised a warning upon usage) since v28.0 (https://github.com/bitcoin/bitcoin/pull/30594), I think this is fine to go in as-is given that I think it’s much less controversial now. Ideally, the deprecation would’ve been marked explicitly in the v28 release notes too, but I missed that in my review.
doc/bips.md/ bip125 is still opt-in, we are drifting further and further away from that BIP and will finally basically kill it off with cluster mempool. Not sure what we want to do there.
14- signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1).
15- A transaction also signals replaceability if its version field is set to 3.
16-
17- *Rationale*: See [BIP125
18- explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).
19+1. DEPRECATED: BIP125 signaling is no longer required.
Deprecated means still supported but discouraged
01. (Removed)
57@@ -63,6 +58,8 @@ This set of rules is similar but distinct from BIP125.
58
59 ## History
60
61+* Signaling for replace-by-fee is no longer required as of [PR 30592](https://github.com/bitcoin/bitcoin/pull/30592).
doc/bips.md/bip125 is still opt-in, we are drifting further and further away from that BIP and will finally basically kill it off with cluster mempool. Not sure what we want to do there.
I think usually if we stop implementing a BIP we can just remove it from bips.md. Or you can say “BIP 125 is not supported. See doc/policy/mempool-replacements.md for replacement policy” ?
33@@ -34,7 +34,7 @@ BIPs that are implemented by Bitcoin Core:
34 * [`BIP 111`](https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki): `NODE_BLOOM` service bit added, and enforced for all peer versions as of **v0.13.0** ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579) and [PR #6641](https://github.com/bitcoin/bitcoin/pull/6641)).
35 * [`BIP 112`](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki): The CHECKSEQUENCEVERIFY opcode has been implemented since **v0.12.1** ([PR #7524](https://github.com/bitcoin/bitcoin/pull/7524)), and has been *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)).
36 * [`BIP 113`](https://github.com/bitcoin/bips/blob/master/bip-0113.mediawiki): Median time past lock-time calculations have been implemented since **v0.12.1** ([PR #6566](https://github.com/bitcoin/bitcoin/pull/6566)), and has been *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)).
37-* [`BIP 125`](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki): Opt-in full replace-by-fee partially implemented: signaling is enforced if configured. For other replacement rules, see doc/policy/mempool-replacements.md.
38+* [`BIP 125`](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki): Not supported. For replacement rules, see doc/policy/mempool-replacements.md.
This list enumerates “BIPs that are implemented by Bitcoin Core”. Adding one that isn’t implemented seems confusing. I’d just remove the line entirely, we already reference BIP125 in https://github.com/bitcoin/bitcoin/blob/da10e0bab4a3e98868dd663af02c43b1dc8b7f4a/doc/policy/mempool-replacements.md?plain=1#L62
If you’re worried about discoverability, adding the hyperlink there could be an alternative:
0diff --git a/doc/policy/mempool-replacements.md b/doc/policy/mempool-replacements.md
1index eb370672e4..3cddc5f037 100644
2--- a/doc/policy/mempool-replacements.md
3+++ b/doc/policy/mempool-replacements.md
4@@ -54,7 +54,8 @@ other consensus and policy rules, each of the following conditions are met:
5 preferable for block-inclusion, compared to what would be removed from the mempool. This rule
6 predates ancestor feerate-based transaction selection.
7
8-This set of rules is similar but distinct from BIP125.
9+This set of rules is similar but distinct from [`BIP
10+125`](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki).
11
12 ## History
13
re-ACK 806ecd6958e4c0fe2a51bee04e20e0dca515bd40
Documentation changes to address review comments, and further functional test cleanups.
On Thu, Oct 31, 2024 at 09:14:40AM -0700, Gregory Sanders wrote:
@@ -34,7 +34,7 @@ BIPs that are implemented by Bitcoin Core:
BIP 111:NODE_BLOOMservice bit added, and enforced for all peer versions as of v0.13.0 ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579) and [PR #6641](https://github.com/bitcoin/bitcoin/pull/6641)).BIP 112: The CHECKSEQUENCEVERIFY opcode has been implemented since v0.12.1 ([PR #7524](https://github.com/bitcoin/bitcoin/pull/7524)), and has been buried since v0.19.0 ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)).BIP 113: Median time past lock-time calculations have been implemented since v0.12.1 ([PR #6566](https://github.com/bitcoin/bitcoin/pull/6566)), and has been buried since v0.19.0 ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)). -*BIP 125: Opt-in full replace-by-fee partially implemented: signaling is enforced if configured. For other replacement rules, see doc/policy/mempool-replacements.md. +*BIP 125: Not supported. For replacement rules, see doc/policy/mempool-replacements.md.Guess I’d prefer to remove it, since it’s been a slippery slope of noncompliance over time
Note that we just changed the status of BIP-125 in the BIPS repo to Obsolete.
I’m fine with removing all mentions of BIP-125 from the Bitcoin Core docs.
Also remove stated support of BIP125 from bips file.
re-ACK c189eec848e3c31f438151d4d3422718a29df3a3
Only change is to remove mention of BIP-125 in doc/bips.md.
0@@ -0,0 +1,7 @@
1+Full Replace-By-Fee
2+===================
3+
4+Starting with v28.0, the `mempoolfullrbf` startup option was set to
5+default to `1`. With widespread adoption of this policy, users no longer
6+benefit from disabling it, so the option has been removed, making full
7+replace-by-fee the standard behavior. (#30592)
This is a bit confusing, because defaulting to mempoolfullrbf=1 already made full replace-by-fee the standard behavior, but this reads as if removing the option makes it the standard behavior.
Perhaps something like this:
0After the policy was broadly adopted by miners, starting with v28.0, the `mempoolfullrbf` startup option was set to
1default to `1`. With widespread adoption of full replace-by-fee, it becomes the standard behavior on the network. Users no longer benefit from disabling it, so the option has been removed. (#30592)
632@@ -633,7 +633,6 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
633 "is of this size or less (default: %u)",
634 MAX_OP_RETURN_RELAY),
635 ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
636- argsman.AddArg("-mempoolfullrbf", strprintf("(DEPRECATED) Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
I ran the node with mempoolfullrbf=0 and it correctly ignored the option.
02024-11-05T09:53:52Z Ignoring unknown configuration value regtest.mempoolfullrbf
700@@ -701,7 +701,7 @@ static RPCHelpMan getmempoolinfo()
701 {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
702 {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
703 {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
704- {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection"},
705+ {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection (DEPRECATED)"},
fullrbf option in this RPC output still be shown at all? Would it hold any meaningful significance or would it be just unnecessary information? I’m inclining towards the latter.
Milestone
29.0