Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698
Conversation
mjdietzx
force-pushed
the
fix_bip125_inherited_signaling
branch
2 times, most recently
from
e09c0b7 to
1d95210
Compare
|
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 |
|
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 |
|
Just don't mention the CVE at all? |
-It fixes bug CVE-2021-31876
+re: CVE-2021-31876-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 Also Bitcoin Core's RBF implementation related docs were updated in #21946 |
mjdietzx
changed the title
mjdietzx
force-pushed
the
fix_bip125_inherited_signaling
branch
from
1d95210 to
0d48c3b
Compare
|
|
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
the
fix_bip125_inherited_signaling
branch
3 times, most recently
from
d2479d0 to
b7f9983
Compare
mjdietzx
force-pushed
the
fix_bip125_inherited_signaling
branch
5 times, most recently
from
4cdec25 to
832696a
Compare
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\`.
Highlight that transactions may signal they are replaceable even though they are not due to BIP125 RBF (Rule bitcoin#5).
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.
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.
Resolves issue bitcoin#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.
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.
mjdietzx
force-pushed
the
fix_bip125_inherited_signaling
branch
from
42cd4ab to
2ac1d33
Compare
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? |
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
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.
src/policy/rbf.cpp
Outdated
There was a problem hiding this comment.
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.
doc/release-notes.md
Outdated
There was a problem hiding this comment.
in 2ac1d33:
I don't think increased confidence needs to be an RPC release note, especially since there is no change to the returned result.
test/functional/feature_rbf.py
Outdated
There was a problem hiding this comment.
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.
| Mempool policy changed such that now RBF inherited signaling is supported (transactions | ||
| that don't explicitly signal replaceability are replaceable for as long as any one | ||
| of their ancestors signals replaceability and remains unconfirmed). Previously | ||
| the mempool only accepted replacements for transactions that themselves opted-in | ||
| to RBF. See `getmempoolentry` in the "Updated RPCs" section below for more details. (#22698) |
There was a problem hiding this comment.
| Mempool policy changed such that now RBF inherited signaling is supported (transactions | |
| that don't explicitly signal replaceability are replaceable for as long as any one | |
| of their ancestors signals replaceability and remains unconfirmed). Previously | |
| the mempool only accepted replacements for transactions that themselves opted-in | |
| to RBF. See `getmempoolentry` in the "Updated RPCs" section below for more details. (#22698) | |
| Mempool transactions now inherit RBF signaling from ancestors, addressing CVE-2021-31876. (#22698) |
| "-acceptnonstdtxn=1", | ||
| "-maxorphantx=1000", | ||
| "-limitancestorcount=50", | ||
| f"-limitancestorcount={MAX_REPLACEMENT_LIMIT + 3}", # enough room to test BIP125 Rule #5 |
There was a problem hiding this comment.
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 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 |
@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:
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 |
|
@mjdietzx Thanks for sharing the details.
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. |
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. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
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)
I was on the fence about including 658a0f3 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:
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. |
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 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 |
|
🐙 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". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
I'm going to close this for now. It can be re-opened and rebased if/when #22867 is merged. |
4c9666b Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard) aae66ab Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard) 3e27e31 Introduce `mempoolfullrbf` node setting. (Antoine Riard) Pull request description: This is ready for review. Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0]. This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior. I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread. [0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html Follow-up suggestions : - soft-enable opt-in RBF in the wallet : #25353 (comment) - p2p discovery and additional outbound connection to full-rbf peers : #25353 (comment) - match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698 ACKs for top commit: instagibbs: reACK 4c9666b glozow: ACK 4c9666b, a few nits which are non-blocking. w0xlt: ACK 4c9666b Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
7 participants
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
IsRBFOptInwhen it invokesCalculateMemPoolAncestors. 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'sMAX_BIP125_REPLACEMENT_CANDIDATES{100}, becauseDEFAULT_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 Minor bug? CalculateAncestorsAndCheckLimits off-by-one error when checking ancestor/descendant count limits #23621 when testing, and would like to see where that lands before I try any optimization.