Add BIP-125 rule 5 testcase with default mempool (tests)

https://github.com/bitcoin/bitcoin/pull/25228

Host: glozow  -  PR author: jamesob

Notes

  • Mempool validation enforces ancestor and descendant count limits, requiring that no mempool transaction have more than 24 (25 with CPFP carve out) descendants.

  • When a node receives a transaction that conflicts with, or spends the same prevout as, one or more of the transactions in its mempool, it decides which transaction(s) to keep based on a set of rules. Bitcoin Core’s Replace by Fee policy requires that no transaction replace more than 100 mempool transactions (“Rule 5”).

  • Many people conclude that the descendant limit makes Rule 5 redundant; it seems that a transaction cannot replace more than 100 transactions a conflicting mempool transaction cannot have more than 25 descendants. This is a very common misconception. A transaction can spend multiple prevouts, and thus conflict with multiple unrelated transactions.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. What does it mean for a transaction to “conflict with” transactions in the mempool? What is the difference between a “directly” and “indirectly conflicting” transaction?

  3. Based on the default RBF policy, how many “direct” conflicts is a transaction allowed to have? How many transactions is it allowed to replace?

  4. Why should the node limit the number of transactions that can be replaced at a time? Can you think of any potential attacks

  5. How is it possible for a transaction to conflict with 100 transactions if the descendant limit is 25? Can you come up with an example that isn’t the one tested in this PR?

  6. What’s wrong with configuring -acceptnonstdtxn=1, -limitancestorcount, -limitancestorsize, -limitdescendantcount, and -limitdescendantsize, to test mempool policy?

  7. Why is it necessary to pass a different sequence to create_self_transfer_multi?

  8. What does annotating get_utxo with the -> dict return type annotation do?

Bonus Questions

  1. Rule 5 only restricts the number of transactions that can be replaced, not the size. However, an effective maximum exists; what is the effective maximum virtual size of transactions that can be replaced in a default mempool? (Hint: default maximum transaction weight and maximum ancestor/descendant limits (Hint Hint: not all of these numbers are relevant)).

  2. Hypothetically, if we increased the default ancestor/descendant limits to 120, would we also need to change the limit on replaced transactions? (Hint: how can a transaction recipient prevent its replacement?)

  3. In what scenarios will the code for calculating the number of to-be-replaced transactions overestimate? If we call pool.CalculateDescendants() with a set of 99 mempool entries, what is the maximum number of mempool transactions we might traverse before the function returns?

Meeting Log

  110:00 <glozow> #startmeeting
  210:00 <michaelfolkson> hi
  310:00 <BlueMoon> Hello!!
  410:01 <danielabrozzoni> hi
  510:01 <glozow> Welcome to PR Review Club! This meeting is intended for beginners to learn about the Bitcoin Core codebase and how to review PRs.
  610:01 <glozow> Today we're looking at PR #25228, "Add BIP-125 rule 5 testcase with default mempool"
  710:01 <glozow> Notes are here: https://bitcoincore.reviews/25228
  810:02 <effexzi> Hi every1
  910:02 <paul_c> Hey
 1010:02 <glozow> Please feel free to ask questions whenever you want, and don't worry about interrupting - this meeting is for learning!
 1110:03 <BlueMoon> Thanks!!
 1210:03 <glozow> Did anyone get a chance to review the PR and/or look at the notes? How about a y/n from people who are here
 1310:03 <paul_c> y
 1410:03 <danielabrozzoni> y
 1510:03 <michaelfolkson> y
 1610:03 <larryruane> hi
 1710:04 <glozow> Great to see people reviewed it! Could you tell us a little bit about your review process?
 1810:04 <BlueMoon> I couldn't check it, sorry.
 1910:04 <larryruane> I'm trying to run the modified test (feature_rbf.py) in the debugger ... but having unexpected trouble for some reason
 2010:05 <paul_c> read through it, search google/youtube for terms I'm not familiar with
 2110:05 <danielabrozzoni> I read the code, I tried to understand the difference between the new `test_too_many_replacements_with_default_mempool_params` and the old `test_too_many_replacements` to understand why the old one wasn't testing the rule 5 use case
 2210:06 <glozow> Great! could somebody summarize what this PR is doing?
 2310:07 <larryruane> adding test coverage, testing the code-under-test in a more realistic way (how it runs in real life)
 2410:07 <danielabrozzoni> Adding a test with the default mempool parameters to check that the nodes are enforcing RBF rule 5
 2510:08 <glozow> larryruane: danielabrozzoni: Great summaries, thank you! Let's move onto the questions. What does it mean for a transaction to “conflict with” transactions in the mempool? What is the difference between a “directly” and “indirectly conflicting” transaction?
 2610:09 <michaelfolkson> Conflict = they both can't get into the blockchain
 2710:10 <glozow> michaelfolkson: but *why* can't they both get into the blockchain?
 2810:10 <OliverOff> A conflicting transaction is one that spends the same prevout as other transactions already in the mempool
 2910:12 <larryruane> i think indirect would be if the existing transaction has decendants ... those would have to be dropped from the mempool too, if this new tx is accepted
 3010:12 <danielabrozzoni> TxA is directly conflicting with TxB if A double spends B's inputs. TxA is indirectly conflicting with TxB if A is directly conflicting with one of B's ancestors (so if B's ancestor is replaced with A, B has to be evicted as well).
 3110:12 <glozow> OliverOff: right, two transactions "conflict" if they spend the same UTXO. You can tell because they each have an input which refers to the same prevout: https://github.com/bitcoin/bitcoin/blob/b752dade048ced8227a9d205a708f50d58f99312/src/txmempool.cpp#L961
 3210:13 <larryruane> danielabrozzoni: yes, I think you said it more clearly than I did (I think we said the same thing)
 3310:13 <glozow> danielabrozzoni: larryruane: yes! if you evict a transaction, you must also evict its descendants. so we also care if there are "indirect" conflicts, i.e. the transaction conflicts with the ancestor of a mempool tx
 3410:14 <danielabrozzoni> larryruane (IRC): yup, same thing 🙂
 3510:14 <glozow> Based on the default RBF policy, how many direct and indirect conflicts can a transaction have?
 3610:14 <glozow> i.e. what is Rule 5? :P
 3710:15 <OliverOff> 100?
 3810:15 <glozow> OliverOff: yup!
 3910:15 <glozow> Why should the node limit the number of transactions that can be replaced at a time? Can you think of any potential attacks if we don't have a limit?
 4010:15 <larryruane> and just to elaborate slightly, the reason we MUST drop the decendants is because their input references an output by txid (and index), and that txid no longer exists
 4110:16 <larryruane> a useful link https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md
 4210:17 <larryruane> I think you could in effect flush other nodes' mempools almost to empty!
 4310:18 <glozow> larryruane: yes, how would an attacker do that and how much would it cost them?
 4410:19 <larryruane> they could submit (let's just say) 500 independent transactions all with low fee (but enough to make it into the mempool), and then attacker could submit a single transaction that conflicts with all 500
 4510:20 <danielabrozzoni> Why would an attacker do that? Waste cpu time of the nodes?
 4610:20 <larryruane> (they wouldn't have to be independent but the dependent ones can form a set of size more than 25, i believe)
 4710:21 <michaelfolkson> It could crash the node depending on the hardware right?
 4810:22 <larryruane> danielabrozzoni: maybe? but also just flushing other nodes' mempools is a kind of DoS because the flushed tx won't get included in a block (unless they're resubmitted)
 4910:22 <glozow> I don't think there would be any crashes, just inconvenient for everybody
 5010:23 <danielabrozzoni> But the txs you're replacing, they're all yours, so I'm not sure why that would be a DoS... does someone have something to read on this?
 5110:24 <michaelfolkson> Requesting and validating a whole new mempool of transactions sounds resource intensive for a very lightweight node
 5210:24 <michaelfolkson> But maybe it can deal with it
 5310:25 <glozow> this might be useful: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-June/017020.html
 5410:25 <danielabrozzoni> Ah right, that makes sense, it's DoS as everyone has to validate a whole new mempool
 5510:25 <danielabrozzoni> glozow (IRC): Thanks!
 5610:27 <glozow> I'm don't have perfect knowledge on RBF history, but it seems to be the case that Rule 5 predates the ancestry limits
 5710:27 <glozow> let's continue with the questions
 5810:27 <glozow> How is it possible for a transaction to conflict with 100 transactions if the descendant limit is 25? Can you come up with an example that isn’t the one tested in this PR?
 5910:30 <larryruane> let's say there are 100 independent tx in the mempool ... the new tx may try to spend an input from all 100 of those tx
 6010:30 <glozow> larryruane: yep exactly, it's as simple as that!
 6110:30 <larryruane> (is there a limit on the number of inputs a tx can have? or a standardness limit at least?)
 6210:31 <OliverOff> Alternatively, by having a tx that conflicts with 4+ txs, each tx having 25 descendants of its own (as mentioned before)
 6310:31 <sipa> larryruane: No limit beyond the normal standard 400000 weight limit per tx.
 6410:31 <larryruane> OliverOff: sipa: +1
 6510:32 <glozow> larryruane: good question. with just 1 output, I think you can get at least 600something inputs before you reach the 400KWu limit
 6610:33 <sipa> larryruane: Which does imply a strict (standardness) limit of ~2400 inputs, as every input is at least 41 vbytes.
 6710:33 <sipa> Of course, if we're talking about standardness, inputs can also only spend standard outputs, and those have a higher minimum per-input weight.
 6810:33 <glozow> woops i think I used a p2sh input size when i calculated
 6910:34 <sipa> p2tr key path spending inputs are probably the cheapest standard inputs you can construct now
 7010:35 <sipa> With those I think you could get to 1738 inputs in a tx.
 7110:35 <sipa> Paging Murch.
 7210:35 <glozow> is it 58vb for a taproot pubkey spend?
 7310:36 <sipa> 36 vbytes prevout, 4 vbytes nsquence, 1 vbyte scriptsig length, 1 WU for number of witness stack items, 1 WU for the length of the witness stack item, 64 WU for the signature
 7410:36 <larryruane> glozow: i think that's right https://twitter.com/murchandamus/status/1262062602298916865?s=20&t=eFK23X7Xy1y5_32Rxx2aQw
 7510:37 <sipa> so 36 + 4 + 1 + (1 + 1 + 64)/4 = 57.5 vbytes?
 7610:37 <michaelfolkson> https://bitcoinops.org/en/tools/calc-size/
 7710:37 <glozow> say we subtract 100vB for the rest of the transaction, 99900/58 gives us ~1700 inputs
 7810:38 <glozow> Fun! I have a bonus question that's pretty relevant here. Rule 5 only restricts the number of transactions that can be replaced, not the size. However, an effective maximum exists; what is the effective maximum virtual size of transactions that can be replaced in a default mempool?
 7910:40 <larryruane> 1700 * 101k or something like that?
 8010:41 <danielabrozzoni> At a guess, the biggest case is replacing 100 independent txs? So, 100*MAX_STANDARD_TX_WEIGHT?
 8110:41 <glozow> ah no, it won't have anything to do with this 1700 number.
 8210:41 <glozow> danielabrozzoni: yes exactly :)
 8310:42 <glozow> so that's 40,000,000Wu
 8410:43 <glozow> next question. What’s wrong with configuring -acceptnonstdtxn=1, -limitancestorcount, -limitancestorsize, -limitdescendantcount, and -limitdescendantsize, to test mempool policy?
 8510:43 <larryruane> so roughly compare that with the default mempool size, 300mb, that's over 10%?
 8610:44 <larryruane> 13% actually... so a single tx could replace 13% of the mempool?
 8710:45 <glozow> larryruane: not really, because we're comparing virtual bytes of the transaction with memory allocated for the mempool data structure.
 8810:45 <larryruane> oh i see, so the 300mb is memory (including metadata overhead like for the map and stuff)
 8910:45 <glozow> the mempool is about 75% metadata, so 300MB really stores about 75MvB worth of transactions
 9010:46 <michaelfolkson> glozow: Not testing the default mempool policy?
 9110:46 <glozow> larryruane: but yes, your math is correct. 10MvB/75MvB is about 13%
 9210:47 <Murch> sup
 9310:47 <michaelfolkson> Wot. Mempool is 75 percent metadata? Like what metadata?
 9410:47 <larryruane> michaelfolkson: +1 but also it's probably good to test that those args are not ignored
 9510:47 <sipa> michaelfolkson: Not metadata - overhead.
 9610:47 <glozow> here's the declaration for mempool entries: https://github.com/bitcoin/bitcoin/blob/b752dade048ced8227a9d205a708f50d58f99312/src/txmempool.h#L85
 9710:48 <sipa> There is some metadata there too, but most if it is just memory allocation overhead, pointers, indexing, ...
 9810:48 <larryruane> michaelfolkson: if you have a std::map of bool, and it contains 1000 entries, the mem usage will be much more than 1000 bytes
 9910:48 <sipa> And way more than 125 bytes, which ought to be enough to store 1000 bools.
10010:48 <_aj_> glozow: "vB" deweights witness data, if you've got a 1000vB tx, that might be 2000B of serialised data, even without metadata
10110:49 <michaelfolkson> sipa: Still sounds a lot. Overhead is only 1/6 of input size for P2TR
10210:49 <glozow> yes there are a lot of approximations here. my point was we can't really just do simple arithmetic to see how many transactions fit in a 300MB mempool
10310:50 <sipa> michaelfolkson: It helps to realize that transactions are abstract data structures with a lot of complex structure in them (inputs, outputs, witnesses, stack items, ...). The traditional "network protocol raw serialized" view of a transaction is just one way of representing it, and isn't actually used internally except for storing on disk and transmitting over the network.
10410:51 <michaelfolkson> Ok, interesting thanks
10510:51 <glozow> there are other data structures in the mempool as well, like mapNextTx and mapDeltas: https://github.com/bitcoin/bitcoin/blob/b752dade048ced8227a9d205a708f50d58f99312/src/txmempool.h#L560-L561
10610:52 <glozow> ok let's move on
10710:52 <BlueMoon> Thank you all, just from reading you I am learning a lot.
10810:52 <glozow> Why is it necessary to pass a different sequence to create_self_transfer_multi?
10910:52 <sipa> std::move(topic)
11010:52 <glozow> BlueMoon: great to hear!
11110:52 <Murch> The 300 MB are usually reached at around 80-95 blocks depth
11210:52 <Murch> Says past-Murch: https://bitcoin.stackexchange.com/a/96070/5406
11310:52 <BlueMoon> :)
11410:53 <larryruane> yes that indirectmap (for the mapNextTx) is one of the most mindbending things i've encountered! very cool though
11510:54 <_aj_> Murch: that's the ballpark that i was thinking/remembering
11610:54 <danielabrozzoni> The transactions that need to be replaced, or that replace, need to signal for RBF, and the way this is done is by setting the sequence to less than 0xffffffff - 1
11710:54 <glozow> danielabrozzoni: yes exactly
11810:55 <glozow> I guess another approach would have been to make miniwallet transactions signal rbf by default, but maybe that'd break a test somewhere
11910:55 <glozow> What does annotating `get_utxo` with the `-> dict` return type annotation do?
12010:56 <larryruane> danielabrozzoni: and it's not "the" sequence number; each input has its own, so *any* sequence number
12110:56 <danielabrozzoni> Do I recall correctly that even a transaction that replaces others in mempool needs to signal for RBF?
12210:57 <danielabrozzoni> larryruane (IRC): yep right! 🙂
12310:57 <larryruane> danielabrozzoni: i don't think so
12410:57 <michaelfolkson> glozow: More metadata describing the return value
12510:57 <glozow> afaik no the replacement doesn't need to signal anything. just the original
12610:58 <sipa> yeah, the signal represents replaceability, not replacingness (which is implied by the fact it's spending an unconfirmed output).
12710:58 <Murch> No, only the replaced needs to signal
12810:58 <danielabrozzoni> Ah ok, I'm getting confused with something else then, thanks everyone 🙂
12911:00 <glozow> michaelfolkson: ye. see #18410 if anyone's interested
13011:00 <glozow> thanks for coming y'all, that's all the time we have today
13111:00 <glozow> #endmeeting