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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30592.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/28374307572</sub>
<details><summary>Hints</summary>
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.
</details>
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.
Agree with @petertodd’s assessment: I don’t think this should be included in v28.0. It might be nice to issue a deprecation warning in v28 on the startup parameter, though.
@murchandamus feel free to open the deprecation PR :+1: I'm leaving as draft as noted in OP until post-28
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. :)
deprecation PR here: https://github.com/bitcoin/bitcoin/pull/30594
@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.
done
left a RPC doc nit, feel free to ignore
Concept ACK
rebased for cmake and un-drafted since we have split off 28.X branch
ACK 4f65c8e98957a6d8fe48ef56383d9d5776d5acd4
This could probably be done in a separate pull-req. But we can also remove the 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.
rebased due to conflict in master
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30886829600</sub>
<details><summary>Hints</summary>
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.
</details>
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.
@maflcko done, thanks
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());
Nice to see these checks going away!
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
This comment is outdated
will fix if I touchup
(note: this did not get fixed - it's a trivial re-ack, would be ideal to get this in this PR - it's very confusing when reading the code)
oops, fixed and removed more vestigial signaling code and comments in test
Concept ACK
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,
I’m a bit surprised, why would we have been signaling replaceability for TRUC transactions in the first place? The only nodes that accept V3 transactions would be upgraded to understand TRUC at the same time, so shouldn’t we create TRUC transactions without signaling replaceability?
Seems erroneous, indeed.
There is a test for both cases v3 + BIP125 and for v3 + non-BIP125 iirc
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)
It seems to me that we would still want to test that TRUC transactions that don’t signal replaceability can be replaced. Obviously the assert_equal here that checks that the node does not use fullrbf would fail, but the rest of the test seems like it could be kept.
~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.
ACK f863f5a4f991ccfdf865d39c73269b196eeb4902
Concept ACK
Also needs a release note.
@fanquake pushed
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
nit: would just drop this line (+newline entirely), already covered in self.log.info('A transaction that replaces a mempool transaction')
done
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. learning a bit from the ecosystem controversies raised when 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.
Starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of this policy, users no longer
benefit from disabling it, so the option has been removed, making full
replace-by-fee the standard behavior. (#30592)
sure, than verbatim
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.
Keeping this section here feels unnecessarily confusing to me. Since it's no longer relevant, I'd just remove the entire paragraph, and potentially add a line in the ## History section?
This may be me being too attached to the BIP125 rule numbers. Moved things around a bit, let me know if it's better.
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.
@stickies-v re 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
1. (Removed)
taken
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).
Move to bottom to retain chronological ordering?
done
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" ?
@glozow tried a version of your suggestion which should be more future proof
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:
<details> <summary>git diff on 806ecd6958</summary>
diff --git a/doc/policy/mempool-replacements.md b/doc/policy/mempool-replacements.md
index eb370672e4..3cddc5f037 100644
--- a/doc/policy/mempool-replacements.md
+++ b/doc/policy/mempool-replacements.md
@@ -54,7 +54,8 @@ other consensus and policy rules, each of the following conditions are met:
preferable for block-inclusion, compared to what would be removed from the mempool. This rule
predates ancestor feerate-based transaction selection.
-This set of rules is similar but distinct from BIP125.
+This set of rules is similar but distinct from [`BIP
+125`](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki).
## History
</details>
I'll let the others weigh in a bit before touching docs more
You can mimic what we did for BIP61 in the same document (which just lists the history of when it was introduced, and when it was removed).
Guess I'd prefer to remove it, since it's been a slippery slope of noncompliance over time
removed the line and updated commit message
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](/bitcoin-bitcoin/6579/) and PR [#6641](/bitcoin-bitcoin/6641/)).BIP 112: The CHECKSEQUENCEVERIFY opcode has been implemented since v0.12.1 (PR [#7524](/bitcoin-bitcoin/7524/)), and has been buried since v0.19.0 (PR [#16060](/bitcoin-bitcoin/16060/)).BIP 113: Median time past lock-time calculations have been implemented since v0.12.1 (PR [#6566](/bitcoin-bitcoin/6566/)), and has been buried since v0.19.0 (PR [#16060](/bitcoin-bitcoin/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:
After the policy was broadly adopted by miners, starting with v28.0, the `mempoolfullrbf` startup option was set to
default 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)
will take language if I touch PR again
ACK c189eec848e3c31f438151d4d3422718a29df3a3
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.
2024-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)"},
After the PR is merged: Thinking out loud, should the 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.
That is changing the API somewhat unnecessarily for this PR. We can properly remove it with deprecation flag in follow-up PR, or in a following release.
reACK c189eec848e3c31f438151d4d3422718a29df3a3
ACK c189eec848e3c31f438151d4d3422718a29df3a3
Milestone
29.0