docs: RBF policy and mempool limit exemptions #23711
Conversation
|
Concept ACK. |
|
Nice! Concept ACK |
|
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. |
|
Strong concept ACK. Thank you for adding documentation! |
b27af5c to
abd4c7c
Compare
There was a problem hiding this comment.
ACK abd4c7c -- thanks, that's great
Bikeshedding a few things, but it looks good to me as such.
I also find compelling the point raised by John in the linked issue to have this in the wiki instead, but absolutely not a blocker for me.
| A transaction conflicts with an in-mempool transaction ("directly conflicting transaction") if they | ||
| spend one or more of the same inputs. A transaction may conflict with multiple in-mempool | ||
| transactions. | ||
|
|
||
| A transaction ("replacement transaction") may replace its directly conflicting transactions and | ||
| their in-mempool descendants (together, "original transactions") if, in addition to passing all | ||
| other consensus and policy rules, each of the following conditions are met: |
There was a problem hiding this comment.
Defining "directly conflicting txs", "replacement tx" and "original txs" is neat 👍
There was a problem hiding this comment.
ACK abd4c7c (though maybe few typos/minors comments worthy to fix)
Given mempool policy is directly consumed by downstream projects for a number of safety-critical operations, I believe its documentation correctness matters. I don't think we have, at least not yet (?), a consistent review process for the wiki.
Further, I think it's good to minimize reliance on GH and to have essential documentation canonically replicated in each one's git tree.
|
|
||
| 1. The candidate transaction has exactly 1 directly conflicting transaction. | ||
|
|
||
| 2. The candidate transaction does not spend any unconfirmed inputs that are not also spent by the |
There was a problem hiding this comment.
In fact this is checked by the rule 2 requirement in ReplacementChecks not in the exemption single-conflict RBF in PreChecks, maybe confusing to mention it here ?
There was a problem hiding this comment.
It is explicitly a requirement for this carve-out though. As commented here:
Lines 821 to 825 in 8c0bd87
There was a problem hiding this comment.
Okay, I got your point there are logically united if you want a working carve-out, though spread out in the code.
| significant portions of the node's mempool using replacements with multiple directly conflicting | ||
| transactions, each with large descendant sets. | ||
|
|
||
| This set of rules is similar but distinct from BIP125. |
There was a problem hiding this comment.
I'm not sure I understand what you mean here by "similar but distinct" ? Maybe say we're partial implementation of BIP125 as we have at least one known divergence (though maybe fixed by #22698)
There was a problem hiding this comment.
I think there's a lot of differences and it may change over time, so it's better to just say they're distinct than try to convey a measurement of how different they are. I also don't like the framing of "partial implementation", since the goal was never to implement BIP125. If anything, BIP125 is a partial description of our code.
More differences:
- We use the node's incrementalRelayFee, not minrelayfeerate now, so if the user sets something different for those two values, we're diverging from BIP125
- We use the sum of directly conflicting transactions' descendant counts, not the exact number of original transactions.
There was a problem hiding this comment.
since the goal was never to implement BIP125
Yes BIP125 was written from Core's implementation. Thanks for the explanation, I agree :)
It shouldn't be. It makes no guarantees, and nodes should vary their mempool policy. Relying on it for anything safety-critical is a security vulnerability. Just don't do it. |
|
concept ACK! will review more closely. It may make sense to try to write these up with 2 levels and one level refining the other. The higher level might be nice as to describing what we want the policy to accomplish and then the concrete policy can be described separately. not to make more work for you, but this would be helpful in the future when we're discussing what the policy should be so we can separate from the higher-order-goals and the things that happen to be true as an artifact of our specific engineering choices. edit: a lot of this already in the rationale, but if you collected the main points from the rationales, you could then link the rationales to the higher order doc |
abd4c7c to
1c75f18
Compare
It can be argued that BIP125#1 signaling is honored because descendants of signaling transactions are replaceable by RBF. Regardless, since there are multiple details in our RBF policy that are not captured in BIP125, point to our doc instead.
1c75f18 to
82858ba
Compare
|
@glozow Do you want this merged or wait a bit more for nits/reviews? |
I think we should give @ariard some time to see #23711 (comment) and #23711 (comment) since he had questions about correctness. Then, I'd feel confident that people agree this documentation is accurate, and any nits can be addressed in a followup. Thanks for asking! |
|
Regarding placement of docs and high-level vs low-level:
I like this framing: that we should (1) communicate our overall design philosophy in a resource for developers and (2) provide concrete/precise documentation of the code we're releasing to other devs/users.
I agree with this. The high-level ideas fit well in the dev wiki (which we can all collaborate freely on) and the concrete documentation of our policy interface should be reviewed + packaged with the code. |
|
ACK 82858ba Had a read through and the language seems approachable and actually clears some things up for me. Thanks for this! |
| A mempool entry's *descendant size* is the aggregated virtual size of in-mempool (unconfirmed) | ||
| transactions in its descendant set, including itself. | ||
|
|
||
| Transactions submitted to the mempool must not exceed the ancestor and descendant limits (aka |
There was a problem hiding this comment.
nit: change 'submitted to' to 'present in'
it's fine to submit them, nothing bad happens.
There was a problem hiding this comment.
ACK 82858ba
Thanks for that!
I like this framing: that we should (1) communicate our overall design philosophy in a resource for developers and (2) provide concrete/precise documentation of the code we're releasing to other devs/users.
Agree to document the overall design philosophy and I think too it might be better suited in the wiki as we might have heterogeneous, evolving viewpoints among contributors.
An exception, one piece of document which could fit in the repo would be a formalized process for policy rules tightening with known implications towards Bitcoin applications/L2 (cf. discussion on potential class of policy changes, see Jeremy's https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-September/019400.html)
82858ba [doc] CPFP carve out and single-conflict RBF exemption (glozow) 1fd49eb [doc] clarify RBF difference from BIP125 (glozow) 919ae8b [doc] current rbf policy (glozow) Pull request description: Since RBF was first implemented and BIP125 was written, our code has changed, people have highlighted implementation differences, and some people have proposed further changes to it. Many people seem to support the idea of documenting our _current_ RBF policy as it stands today. As the ancestor/descendant limit carve-out exemptions are very related to RBF, it seemed appropriate to group them with this PR. Related to bitcoin#22806 - it seems that these policies are the most confusing for people, or at least the most documentation-requested. ACKs for top commit: dunxen: ACK 82858ba t-bast: ACK bitcoin@82858ba, thanks @glozow! darosior: re-ACK 82858ba ariard: ACK 82858ba Tree-SHA512: 5d296537cce3488c18179c0aa76c739ca02fdc424e5aa17129b4cdd0d057358f86bcc1e92a9857bd2c60495f834fe9d9406d1a9f8ac5cfc8f0f4f4c27ec4f8e1
Since RBF was first implemented and BIP125 was written, our code has changed, people have highlighted implementation differences, and some people have proposed further changes to it. Many people seem to support the idea of documenting our current RBF policy as it stands today.
As the ancestor/descendant limit carve-out exemptions are very related to RBF, it seemed appropriate to group them with this PR.
Related to #22806 - it seems that these policies are the most confusing for people, or at least the most documentation-requested.