Major review comments / events by topic:
Note that there is a difference between the results returned from ProcessNewPackage(tx) and ATMP(tx).
The most obvious example is the fact that packages can't do RBF. It seems like the opinions are not very strong, and the decision can be deferred to the future.
There is a lot of discussion on this. I think most people are okay with deferring the decision to later since this is only exposed on RPC at the moment.
The TLDR of why we're disabling RBF in packages is (1) there aren't any use cases where we need both RBF and package relay and (2) making package accepts work with RBF can get really complex.
Expanding on #1: The main use case that I can think of for package accept is CPFPing a transaction (or chain of dependent transactions like in Lightning) whose base feerate is not sufficient to meet the mempool minimum fee and would thus be rejected by the mempool otherwise. If any of the transactions are able to replace mempool transactions, they can be submitted individually and don't need package accept.
Expanding on #2, Here is a concrete example of why RBF + package accept complicates the mempool logic (from @sdaftuar):
"Suppose you have transaction A in the mempool, and you're evaluating a package B -> C for acceptance. Suppose also that B conflicts with A, but that C spends an output of A (!) in addition to an output of B. Obviously no such transaction C can ever be mined, as it must necessarily be invalid; however if the logic you have is just checking that the package (B, C) has unique inputs, and then that B validly RBF's A, then when you evaluate C you will find that both its inputs are available (because A has not yet been removed from the mempool!) and so it can pass validation. So if someone enabled RBF for package acceptance and turned mempool acceptance on for packages coming over the p2p network, this could result in an inconsistent mempool, which would obviously be very bad. In order to prevent this while still permitting packages to RBF, you'd have to more carefully track inputs in the mempool that would become invalid as a result of RBF, so that those coins could not be spent by the package. That would be pretty annoying I think, so absent a use-case for it, I think it's best to just ignore this problem and enforce that RBF is not allowed for the time being."
PR discussion:
This has been deferred to PR#21800. I feel that this is safe to do because ancestor/descendant limits are designed to protect us from heavy computation due to large families in the mempool. This PR may underestimate during the test accept, but the only way to submit these transactions is one-by-one through ATMP (which enforces the actual ancestor/descendant limits). Thus, this code doesn't create a risk of the mempool exceeding our normal limits. The only problem is that testmempoolaccept might incorrectly report success because it underestimates ancestors, so I've noted that in the release notes.
It's also kind of complex and I'm still iterating on the approach. Here's a writeup if you're interested.
There are some comments from when I had the CalculateMemPoolAncestors for packages implementation in #20833, starting around here.
We check the feerate in CheckFeeRate() in PreChecks().
An actual mempool submission will call mempool.TrimToSize() afterward, and it's possible for the tx to be spit right back out because it was above the mempool min fee which was calculated before the tx was added.
A normal testmempoolaccept with 1 tx will also have this inaccuracy.
The PR was first opened with a "if anything fails, we only return 1 result."
- Thread: it's very weird to have a parent fail on "max-fee-exceeded" and report success on its child.
- Issue#21074
My proposal: If a transaction in the package exceeds maxfeerate, the "reject-reason" is "max-fee-exceeded" and all subsequent transactions have blank validation results (i.e. only "txid" and "wtxid" fields). This is the same as what we do if a transaction fails validation and is still compatible with the API on master.
There has been a bit of flip-flopping on this. The PR was first opened with an extension of CCoinsViewMemPool, then had CoinsViewTemporary, then switched back to the CCoinsViewMemPool approach.
- Comment
- When I switched to
CoinsViewTemporary: Comment - When I switched back to
CCoinsViewMemPool: Comment
See thread. Yes, the plan is to add this in real package acceptance.
- IRC logs
- Comment
- Comment
- Comment
- See PR#21413 for the L2 testing use case.
- "Is this supposed to work for package relay?" the short answer is yes. Yes, the code in validation.cpp is supposed to DoS-resistant and fail-fast.
- Why optionals, optional vs variant: thread
- See PR#21062 for more discusssion on this struct
- Why we have
VALIDandINVALIDinstead of just a boolean: I was planning on having anUNFINISHEDstate before (but decided to just have it be absent). Also thought about having aVALID_BYPASSED_SOMETHING: comment - Having private constructors and public static
Success/Failureaccess: comment