Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status

mjdietzx commented at 2:41 pm on August 13, 2021: contributor

This PR is on top of #22867 which adds test coverage relevant to this PR, and motivates it by highlighting some of the behavior this improves upon.

It aims to resolve issue #22209 as well as CVE-2021-31876

Bitcoin Core 0.12.0 through 0.21.1 does not properly implement the replacement policy specified in BIP125, which makes it easier for attackers to trigger a loss of funds, or a denial of service attack against downstream projects such as Lightning network nodes.

As a followup to this PR I think some fuzz testing would be useful.
Also an optimization followup can be done, putting some stricter bounds on IsRBFOptIn when it invokes CalculateMemPoolAncestors. However, I don’t do that in this PR to keep it as simple as possible, and I don’t think it’s necessary. a) default mempool limits are much stricter than BIP125 Rule 5’s MAX_BIP125_REPLACEMENT_CANDIDATES{100}, because DEFAULT_DESCENDANT_LIMIT == 25. b) I don’t see a major performance tradeoff (please double check me). I think https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_updatefromblock.py is a good worst-case scenario stress test for additional runtime required to check for inherited signaling, and this test seems to run in approx the same amount of time after this PR. Which seems acceptable to me, and not a risk for a DOS vector. But again, please be critical of this in the review. I also noticed #23621 when testing, and would like to see where that lands before I try any optimization.

mjdietzx force-pushed on Aug 13, 2021

luke-jr commented at 3:05 pm on August 13, 2021: member

This is not a security issue in Bitcoin Core, and there is nothing Bitcoin Core can do to fix it.

Fixing this bug is fine, but please don’t misrepresent it.

mjdietzx commented at 3:11 pm on August 13, 2021: contributor

@luke-jr

This is not a security issue in Bitcoin Core, and there is nothing Bitcoin Core can do to fix it. Fixing this bug is fine, but please don’t misrepresent it.

I don’t think its a security issue in Bitcoin Core. Did I imply that somehow? I basically view this as minor bug fixes, and making bitcoin core adhere to BIP 125 plus getting rid of some inconsistencies

luke-jr commented at 3:13 pm on August 13, 2021: member

Specifically, “It fixes bug CVE-2021-31876” is incorrect. The CVE does not affect Bitcoin Core, and cannot be fixed by changing Core’s default policies.

mjdietzx commented at 3:15 pm on August 13, 2021: contributor

Specifically, “It fixes bug CVE-2021-31876” is incorrect. The CVE does not affect Bitcoin Core, and cannot be fixed by changing Core’s default policies.

Any suggestions on how I should better word this? I’ll edit

luke-jr commented at 3:24 pm on August 13, 2021: member

Just don’t mention the CVE at all?

ghost commented at 3:24 pm on August 13, 2021: none

Any suggestions on how I should better word this? I’ll edit

0-It fixes bug CVE-2021-31876
1+re: CVE-2021-31876

0-Fix CVE-2021-31876 RBF inherited signaling and fixes getmempoolentry returned bip125-replaceable status 
1+Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status

Also Bitcoin Core’s RBF implementation related docs were updated in #21946

mjdietzx renamed this:
~~Fix CVE-2021-31876 RBF inherited signaling and fixes getmempoolentry returned bip125-replaceable status~~
Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status
on Aug 13, 2021

DrahtBot added the label TX fees and policy on Aug 13, 2021

DrahtBot added the label Utils/log/libs on Aug 13, 2021

DrahtBot added the label Validation on Aug 13, 2021

mjdietzx force-pushed on Aug 13, 2021

mjdietzx marked this as a draft on Aug 13, 2021

mjdietzx commented at 6:41 pm on August 13, 2021: contributor

I made this PR a draft for now. Seeing a performance degradation in functional test mempool_updatefromblock.py: Create an acyclic tournament (a type of directed graph) of transactions and use it for testing.

~~This is along the lines of what @ariard said in #21946~~

IMO, main motivation to not fix it is avoiding increasing the DoS surface of the mempool

~~I’m gonna dig into this and see if there’s anything that can be done here. Also open to ideas~~

luke-jr commented at 7:16 pm on August 13, 2021: member

+re: CVE-2021-31876

But it isn’t related to the CVE at all. The security issue is that some software assumes the network has a consistent and firm policy as if it were a consensus rule. That’s an issue regardless of what policy Bitcoin Core implements.

DrahtBot commented at 0:15 am on August 14, 2021: 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:

#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)
#23121 ([policy] check ancestor feerate in RBF, remove BIP125 Rule2 by glozow)
#22689 (rpc: deprecate top-level fee fields in getmempool RPCs by josibake)

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.

mjdietzx force-pushed on Aug 14, 2021

mjdietzx force-pushed on Aug 15, 2021

mjdietzx marked this as ready for review on Aug 15, 2021

mjdietzx commented at 4:24 pm on August 15, 2021: contributor

Marked this as ready for review. I think I just got too fancy with my initial implementation of isRBFOptIn - turns out I didn’t have to change the original src code very much at all, and once I minimized my changes the performance degradation I was seeing (test/functional/mempool_updatefromblock.py’s runtime) resolved itself. Runtime of this test seems to be identical to what it was before this PR - I also should note this test turns out to be a really good benchmark for this PR as it is a pretty good worst-case scenario and stress test for doing tons of rbf signaling checks

mjdietzx commented at 6:12 am on August 16, 2021: contributor

Take it w/ a grain of salt, but as a sanity check I ran some mempool related benchmarks before/after this PR. Not very scientific in how I ran these (on my laptop, similar conditions for both runs but I have other stuff running in the background etc.. there was also a lot of variance between multiple runs to the point where I’m not sure we can tell much from these results), but providing the results as-is (one run each)

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------

Before PR:

0|      284,078,677.00 |                3.52 |    1.3% |      3.14 | `ComplexMemPool`
1|           35,208.00 |           28,402.64 |   26.0% |      0.00 | :wavy_dash: `MempoolEviction` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10)
2|       17,224,542.00 |               58.06 |    4.1% |      0.19 | `RpcMempool`

After PR:

0|      365,566,686.00 |                2.74 |    2.6% |      4.04 | `ComplexMemPool`
1|           50,333.00 |           19,867.68 |   50.2% |      0.00 | :wavy_dash: `MempoolEviction` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10)
2|       19,457,056.00 |               51.40 |    3.6% |      0.22 | `RpcMempool`

mjdietzx force-pushed on Aug 16, 2021

ariard commented at 11:40 pm on August 16, 2021: member

Concept ACK,

Even if I’m still planning to propose opt-in removal in 0.24, this change might reveal as too contentious and it might never lands. Considering this scenario, we’re better off fixing the defect and the code matching the spec.

I don’t have yet look on implementation details, though we’ll look on it soon + #22707 to check we don’t downgrade PreChecks() perf and as such increase DoS surface.

ariard commented at 0:17 am on August 17, 2021: member

@luke-jr

But it isn’t related to the CVE at all. The security issue is that some software assumes the network has a consistent and firm policy as if it were a consensus rule. That’s an issue regardless of what policy Bitcoin Core implements.

I think the security issue is qualified in the consequences for downstream Bitcoin users of the code defect compared to the standard. Such standard committed by Core in doc/bips.md.

In a decentralized system such as Bitcoin, I don’t think we can admit an authoritative enumeration of the valid assumptions an application can draw on the p2p network. It’s more constructive to argue in the way “If we assume X, is Y safer, cheaper, easier-to-design ?”.

If we assume BIP152 to be widely deployed, block propagation is faster and overall network security better. If we assume BIP330 to be widely deployed, transaction bandwidth consumption is better and cost-to-run a full-node lower. If we assume BIP125 to be widely deployed, LN security is better as LN nodes can adapt to mempools congestion.

Following this reasoning, as protocol developers, I think we can rationally discuss about the benefits of assumptions for the wider ecosystem and even how to make them more “binding”.

mjdietzx commented at 10:04 am on August 17, 2021: contributor

Thanks @ariard !

Even if I’m still planning to propose opt-in removal in 0.24, this change might reveal as too contentious and it might never lands. Considering this scenario, we’re better off fixing the defect and the code matching the spec.

I came to the same conclusion (had been following the issues/PRs around this topic), and I thought this was a good next step regardless so through my hat in the ring w this PR

I don’t have yet look on implementation details, though we’ll look on it soon + #22707 to check we don’t downgrade PreChecks() perf and as such increase DoS surface.

Sounds good, looking forward to what you find. I did end up adding this optimization 840cf74878164e0de9fc2bc958dd60448a8e4d45 because:

it establishes a hard upper-bound on worst-case computation (so we don’t only rely on the node’s configured limitancestorcount, limitdescendantcount, etc..). I’m hoping this also makes it easier to reason about PreChecks() performance, and any potential for increased DoS surface.
improve performance, I believe getrawmempool (verbose) and getmempoolentry will run faster in all-cases (and possibly significantly faster in worst-case scenarios) than before this PR due to this optimization. But I think PreChecks() performance is probably more important, and more work is done in PreChecks() to check for inherited signaling after this PR – so we need to ensure that is acceptable.

MarcoFalke referenced this in commit 4fc15d1566 on Aug 20, 2021

michaelfolkson commented at 10:50 am on August 30, 2021: contributor

This PR sort of opposes #22665 - I think they are two different approaches to solving some of the confusing behavior we currently have with BIP 125 / RBF. And they should both be considered, although I think it’s one or the other.

I disagree that it is one or the other (at least assuming this PR can be rebased on top of a future merged #22665 reversing some changes). I think #22665 addresses confusion over not following the BIP wording in the short term (quick fix) and then this PR actually implements that BIP wording in the longer term (not a quick fix). I need to think more about whether the BIP wording should be implemented in the longer term. It seems dependent on speculating on when/if we are able to implement full RBF within the next couple of major versions of Core or not.

mjdietzx commented at 2:17 pm on August 30, 2021: contributor

Good point @michaelfolkson, I’ll be sure to rebase this PR when #22665 is merged so that this PR can be considered 👍

DrahtBot added the label Needs rebase on Aug 31, 2021

mjdietzx marked this as a draft on Sep 2, 2021

mjdietzx commented at 4:09 pm on September 2, 2021: contributor

I am converting this to a draft for now. There are a few related PRs that need to be merged before I can rebase this PR. And I’m also expecting some non-negliglbe changes based on these.

In the meantime I am planning on splitting out the test coverage added in this PR into its own PR. Once everything is in a steady state I will rebase this PR and re-open for review

glozow commented at 6:04 pm on November 20, 2021: member

Are you still working on this?

mjdietzx commented at 6:38 pm on November 20, 2021: contributor

Yeah, I am planning on rebasing this and re-opening it. Do you think now is a good time to do that?

When I marked this as a draft I realized there was a lot of ongoing mempool policy work and that this would need to be re-worked after those changes were merged. At that time I also broke out the tests from this PR into #22867 demonstrating some of the inconsistencies that this PR will resolve, and sort of motivating this PR. So when I do rebase and re-open this it will be on top of #22867

Do you have any thoughts or suggestions about next steps here @glozow ? Are you a concept ack on this in general? I’m interested in your thoughts on how to proceed here

glozow commented at 7:27 pm on November 22, 2021: member

Yeah, I am planning on rebasing this and re-opening it. Do you think now is a good time to do that?

Yes, I think so. Thank you for your consideration :)

Do you have any thoughts or suggestions about next steps here @glozow ? Are you a concept ack on this in general?

Yes, Concept ACK. I was hesitant before because I think adherence to this interpretation of BIP125#1 (which was written after the code) is insufficient rationale; it is better to focus on implementing what’s appropriate for the node. I realized it’s better to implement inherited signaling for now (independent of any full RBF plans), as it’s a simpler interface for wallets. Right now, descendants of replaceable transactions are replaceable despite not “inheriting” signaling (simply replace its signaling ancestor) but wallets devs shoudn’t need to work around that.

Thanks for helping to document and test the RBF logic!

mjdietzx force-pushed on Nov 28, 2021

mjdietzx marked this as ready for review on Nov 29, 2021

mjdietzx commented at 0:56 am on November 29, 2021: contributor

I rebased and simplified this PR. It is now ready for review. I also re-worked the PR description as things have changed a lot (this is just simpler than before)

DrahtBot removed the label Needs rebase on Nov 29, 2021

in src/policy/rbf.cpp:70 in 67a3df7c7d outdated

60-            return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
61-                             txid.ToString(),
62-                             nConflictingCount,
63-                             MAX_BIP125_REPLACEMENT_CANDIDATES);
64-        }
65-    }

glozow commented at 4:44 pm on December 1, 2021:

commit “validation: MemPoolAccept::PreChecks count evictions accurately” and “test: replacement tx rejected bc conflicts may be double counted” are interesting but out of the scope of this PR. I’d suggest removing them.

in doc/release-notes.md:118 in a672b3151b outdated

113+  inherited signaling). Some other discrepancies b/w how `getmempoolentry`
114+  signals, and what the mempool accepts were also made consistent. As well as
115+  more closely adhering to the rules outlined in BIP125 (specifically Rule #5).
116+  The result is increased confidence that the bip125-replaceable status returned
117+  by `getmempoolentry` is accurate. (#22698)
118+

glozow commented at 4:46 pm on December 1, 2021:

in 2ac1d33d19f070fff976d544db1df01b0c88c74d:

I don’t think increased confidence needs to be an RPC release note, especially since there is no change to the returned result.

in test/functional/feature_rbf.py:679 in 3fc1f8b612 outdated

704@@ -705,7 +705,7 @@ def test_prechecks_overestimates_replacements(self):
705         self.wallet.get_utxo(txid=tx['txid'])
706         self.wallet.get_utxo(txid=replacement_tx['txid'])
707 
708-    def test_reorged_inherited_signaling(self):
709+    def test_reorged_inherited_signaling_and_descendant_limit(self):

glozow commented at 4:59 pm on December 1, 2021:

for commit “test: incorrect rbf status when max replacement limit exceeded”

While it’s true that bip125-replaceable is only checking for Rule#1, and actual replacement is restricted by other requirements as well, I don’t think it’s necessary to add a test specifically to illustrate this.

Instead, you could probably clarify the result in the RPC help string.

mjdietzx force-pushed on Dec 1, 2021

test: clarify no inherited replaceability assertions f01954d74a

test: replacement tx rejected bc conflicts may be double counted 5e72716a45

test: dynamic rbf inherited signaling in the case of a reorg

In the event of a reorg, the assumption that a newly added tx has no
in-mempool children is false. If the children opted-out of rbf they
would show \`RBFTransactionState::FINAL\` prior to the reorg.
Upon their rbf opt-in parent being accepted back into the mempool, they
would show \`RBFTransactionState::REPLACEABLE_BIP125\`.

25f1e8aed1

test: incorrect rbf status when max replacement limit exceeded

Highlight that transactions may signal they are replaceable even
though they are not due to BIP125 RBF (Rule #5).

64c20402b2

policy: bip125-replaceable status can't be determined w/o mempool

The number of original transactions to be replaced and their
descendant transactions must not exceed `MAX_BIP125_REPLACEMENT_CANDIDATES`.
This is according to BIP125 RBF (Rule 5). Without an entry in the mempool
(whether it's because we don't have a local mempool or we don't have `tx` in
our mempool) this check can't be done, and therefore the bip125-replaceable
status will be unknown regardless of how `tx` itself signals.

11ce42733f

policy: `getmempoolentry` return "correct" bip125-replaceable status

Now RBF signaling adheres to BIP125 RBF (Rule 5) which states that the number
of original transactions to be replaced and their descendant transactions
must not exceed `MAX_BIP125_REPLACEMENT_CANDIDATES`. Prior to this commit,
the bip125-replaceable status of a given transaction indicated whether the
transaction or _any_ of its unconfirmed ancestors opted-in to RBF, with no
consideration of this limitation.

Note: the bip125-replaceable status that `getmempoolentry` returns is still
inconsistent with what the mempool will treat as replaceable, as the mempool
does not (yet) allow inherited signaling.

831ecb0cc0

validation: `MemPoolAccept::PreChecks` allow rbf inherited signaling

Resolves issue #22209.

The bip125-replaceable status that `getmempoolentry` returns is now
consistent with what the mempool will treat as replaceable. Transactions signal
"correctly" w.r.t. inherited signaling, and the mempool will accept a
replacement of any transaction signaling replaceability via RBF.

1afd090baa

validation: `MemPoolAccept::PreChecks` count evictions accurately

Previously when enforcing BIP125 Rule 5, the number of actual descendants of a set
of conflicts will be counted multiple times in some cases (ie if multiple conflicts
share a descendant). This was done to be conservative and avoid doing too much work.
However, it seems preferable to be accurate as there is no major cost or performance
bottleneck/tradeoff to do this exactly, and the code is simplified.

658a0f39f1

doc: add release notes for rbf improvements 2ac1d33d19

mjdietzx force-pushed on Dec 1, 2021

mjdietzx commented at 6:11 pm on December 1, 2021: contributor

Rebased on top of #22867 after #23437 and #22677 were merged

ghost commented at 9:48 am on December 2, 2021: none

I also re-worked the PR description as things have changed a lot (this is just simpler than before)

Can you share the things that have changed a lot? Sorry it’s difficult to keep a track of everything related to mempool, rbf etc.

Also is there any pull request open in LN implementation repos to fix this CVE which affects LN implementations?

in doc/release-notes.md:61 in 2ac1d33d19

53@@ -54,6 +54,12 @@ unsupported systems.
54 Notable changes
55 ===============
56 
57+Mempool policy changed such that now RBF inherited signaling is supported (transactions
58+that don't explicitly signal replaceability are replaceable for as long as any one
59+of their ancestors signals replaceability and remains unconfirmed). Previously
60+the mempool only accepted replacements for transactions that themselves opted-in
61+to RBF. See `getmempoolentry` in the "Updated RPCs" section below for more details. (#22698)

glozow commented at 11:01 am on December 2, 2021:

0Mempool transactions now inherit RBF signaling from ancestors, addressing CVE-2021-31876. (#22698)

in test/functional/feature_rbf.py:40 in 2ac1d33d19

36@@ -36,7 +37,7 @@ def set_test_params(self):
37             [
38                 "-acceptnonstdtxn=1",
39                 "-maxorphantx=1000",
40-                "-limitancestorcount=50",
41+                f"-limitancestorcount={MAX_REPLACEMENT_LIMIT + 3}", # enough room to test BIP125 Rule #5

glozow commented at 11:05 am on December 2, 2021:

Side note: It’s not very clean to change a policy in order to test another, tightly related policy. If the test requires more than 100 replaced transactions, you should create multiple conflicting transactions rather than increasing the ancestor limit.

glozow commented at 11:10 am on December 2, 2021: member

Thanks for updating! First pass through - I appreciate that you’ve used functional tests to dig into this code and they’re really good, but a lot are out of scope for this PR. I’d recommend adding unit tests for SignalsOptInRBF() instead.

mjdietzx commented at 4:18 pm on December 2, 2021: contributor

@glozow thanks for the review! To clarify, this PR is on top of #22867 which should be reviewed independently. I agree that not all the functional tests are relevant, but I think once you consider the PRs separately it will make more sense.

I will look at adding some unit tests, I agree it’s necessary. I will go through your comments in more detail shortly

mjdietzx commented at 4:28 pm on December 2, 2021: contributor

Can you share the things that have changed a lot? Sorry it’s difficult to keep a track of everything related to mempool, rbf etc. @prayank23 The PR description and the commit messages (which are pretty detailed) should provide a good overview.

I know you looked at this PR way back when I first opened it, when it was very rough. It was marked as a draft for a while, and the main things I changed when I re-opened it were:

broke a lot of the additional functional test coverage into a separate PR #22867
did not do any performance optimizations to IsRBFOptIn (instead I believe that should be a followup, if we think it’s worthwhile)
rebased on top of a lot of related work which allowed me to simplify and have more minimal changes to code (so the code-diff of this PR is much smaller than previously)

Also is there any pull request open in LN implementation repos to fix this CVE which affects LN implementations?

Great idea, I will look into this and follow up. Honestly I have not looked at other implementations much. IIRC mempool policy of other implementations differs from bitcoin core, but I’ll take a look in detail

ghost commented at 5:15 pm on December 2, 2021: none

@mjdietzx Thanks for sharing the details.

Great idea, I will look into this and follow up. Honestly I have not looked at other implementations much. IIRC mempool policy of other implementations differs from bitcoin core, but I’ll take a look in detail.

There are 7 LN implementations based on this list. Some of them might be using BTCD which has inherited signaling. However, 99% of Bitcoin nodes use Bitcoin Core as Bitcoin full node. Please share the link to issue/PR if you create one in any LN implementation (would follow the discussion) because RBF is just a mempool policy and cannot be enforced, so nothing would prevent nodes (or even miners) from using a different policy if that helps in exploiting vulnerable LN projects.

glozow commented at 11:23 am on December 3, 2021: member

To clarify, this PR is on top of #22867 which should be reviewed independently. I agree that not all the functional tests are relevant, but I think once you consider the PRs separately it will make more sense.

Thanks for clarifying. The “validation: MemPoolAccept::PreChecks count evictions accurately” commit is not contained in #22867, and not related to inherited signaling. Note that it increases the number of potential mempool entries visited from 100 to unlimited.

unknown changes_requested

unknown commented at 5:54 pm on December 4, 2021: none

Concept NACK

I don’t think any changes are required in Bitcoin Core’s mempool policy. Documentation could have been improved and it was done in #21946. Documentation can always be improved further.

Even if changed it won’t fix CVE that affects vulnerable LN projects

More context: #22698 (comment)

mjdietzx commented at 7:03 pm on December 4, 2021: contributor

The “validation: MemPoolAccept::PreChecks count evictions accurately” commit is not contained in #22867, and not related to inherited signaling. Note that it increases the number of potential mempool entries visited from 100 to unlimited.

I was on the fence about including 658a0f39f11f023c531a78472c9379033574def1 in this PR. I tend to agree it is not necessary for it to be in this PR, and I’d be open to removing it for now. But my thought process was:

it is related to inherited signaling because there are cases where a transaction should be replaceable due to inherited signaling, but is not due to eviction double counting. And in these cases the bip125-replaceable status that getmempoolentry returns will be different than what the mempool will treat as replaceable. Which is something I wanted to be totally consistent after this PR
Note that it increases the number of potential mempool entries visited from 100 to unlimited. This is a great point, and I want this to be front-and-center in this PR. It is not just the case in MemPoolAccept::PreChecks, but also the case in IsRBFOptIn 831ecb0cc0d2bb66f0aecde01df0ccfe00d6b510. Instead we rely on the local mempool’s policy to put a hard bound on the work we do to determine inherited signaling:

0default mempool limits are much stricter than BIP125 Rule 5's MAX_BIP125_REPLACEMENT_CANDIDATES{100},
1because DEFAULT_DESCENDANT_LIMIT == 25

In my testing/implementation I found this was a safe assumption (I discuss this more in the PR description). But this is the risk of this PR and we need to be sure we don’t introduce a DOS vector here. Which I don’t believe we do, but the reviewers who know much better than me will need to confirm this.

glozow commented at 1:46 pm on December 7, 2021: member

Note that it increases the number of potential mempool entries visited from 100 to unlimited. This is a great point, and I want this to be front-and-center in this PR. It is not just the case in MemPoolAccept::PreChecks, but also the case in IsRBFOptIn 831ecb0.

Right, I hadn’t noticed that it was the case in both. Limiting to 100 direct conflicts and default ancestor limits means up to 2500 entry lookups. But both IsRBFOptIn and GetItersForConflicting are worse because the number of direct conflicts hasn’t been restricted at all. It can contain much more than 100 - the limit is the number of inputs you can cram into a 100KvB transaction. The two functions combined can cause you to search all of the ancestors and all of the descendants, and the inputs don’t even need valid signatures because you haven’t done script checks yet. It can probably cover your entire mempool.

I think as long as you fail faster, you can still get decent bounds (based on the anc/desc limit). Have you also considered an alternative approach for signaling checks - instead of searching ancestor sets of all conflicts every time, caching a signaling bool in mempool entries? It’s an extra bit for every transaction, but perhaps we can pack it somewhere in CTxMemPoolEntry. We would also need to update entries on removal (limited to limitdescendantcount). I haven’t really dug into it - it’s possible that the alternative approach is worse - but maybe worth thinking about?

DrahtBot added the label Needs rebase on Dec 7, 2021

DrahtBot commented at 2:39 pm on December 7, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 1:06 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fanquake commented at 10:50 am on March 22, 2022: member

I’m going to close this for now. It can be re-opened and rebased if/when #22867 is merged.

fanquake closed this on Mar 22, 2022

MarcoFalke referenced this in commit a7f3479ba3 on Jul 8, 2022

DrahtBot locked this on Mar 22, 2023

Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698

Conflicts