When a transaction fails mempool validation, we categorize the failure as one of a few
types. Most notably, we distinguish between consensus rule violations and local policy-based
rejections so that we can inform the P2P layer about peer misbehaviors (see
In a similar vein, this PR distinguishes between PCKG_BAD and PCKG_POLICY.
Miners seek to maximize the total transaction fees while ensuring that their blocks are within
consensus-enforced weight and sigops limits. To simplify this 2-dimensional
in the mempool, virtual size of a transaction is
as the maximum between its BIP141 serialized
its “sigop weight”.
What does IsChildWithParents() check? How does it do this?
What criteria must a package meet in order to be considered a child-with-unconfirmed-parents
package? Is it possible to verify without looking at the current chain? Is it possible to verify
without looking at our mempool?
How does this PR implement checking that a package is child-with-unconfirmed-parents? (Hint: code
Why do we add the child’s
inputs to coins_to_uncache beforehand?
Why do we distinguish between PCKG_BAD and PCKG_POLICY? Within this PR, do we do anything
differently based on the result type?
In what scenarios could the virtual sizes obtained from GetVirtualTransactionSize()here and
the MempoolAcceptResult be different? (Hint: is it possible for the tx to be different? Is it
possible for PreChecks to calculate the virtual size differently?)
Quiz: given a multi-parent-1-child package of Tx1, Tx2, and Tx3 (where Tx3 is the child and there
are no dependencies between the parents), which of the following groups of transactions may be in
the mempool at the end of ProcessNewPackage()?
(B) Tx1 only
(C) Tx3 only
(D) Tx1 and Tx2
(E) Tx1 and Tx3
(F) Tx1, Tx2 and Tx3
Under what circumstances is the “mempool full” error returned as the validation result for an individual
transaction? (Hint: the code is
prevents the LimitMempoolSize() from being called after each transaction is submitted. What could
happen if we didn’t do this?
This commit adds a descendant limit for each transaction Workspace and changes the MemPoolAccept
limit to const. Given that our mempool policy does not change during a validation instance, how is
it possible for different transactions to have different descendant limits?
<glozow> Kaizen_Kintsugi: partially correct, yes - it defines a package (group of dependent transactions) and implements package acceptance for packages of a specific topology. And it's in preparation for fee-bumping within packages in the future
<davidbak> one thing unclear to me is whether this is the _first_ change to B.C. dealing with packages or if packages already exist in some way. The gist starts by saying this is for "packages consisting of multiple parents and 1 child" which leaves me wondering if there was a previous one, accepted, dealing with, say, 1 parent and 1 child.
<stickies-v> it verifies that the submitted package has 1 child and that all inputs of that child are transactions that are either submitted in the package (parents) if they are unconfirmed, or transactions that are already confirmed and thus in the UTXO set
<glozow> stickies-v's second answer is correct - we're making sure that all of the transactions correspond to an input of the child (last transaction). though not all parents need to be present, as larryruane says
<stickies-v> I think it's not possible to verify without looking at the current chain, but it is possible to verify without looking at the mempool since all confirmed parent transactions need to be in the package
<stickies-v> all transactions in the package except for the last one need to be used as inputs for the last one (i.e. the child), and all the unconfirmed inputs of the child need to be provided in the package
<glozow> A child-with-unconfirmed-parents package is a topologically sorted package that consists of exactly one child and all of its unconfirmed parents (no other transactions may be present). The last transaction in the package is the child; each of its inputs must refer to a UTXO in the current chain tip or some preceding transaction in the package.
<larryruane> By recursive I meant, like D has 3 inputs, which refer to outputs of A,B,C (all 4 in the mempool) ... if A has an input that refers to an X output, (X also in mempool), then can (should) X be in the package?
<stickies-v> glozow we check that each input tx is either in parent_txids or in the UTXO set with m_view.HaveCoin. We keep the coins_to_uncache vector to remove newly added outputs from the cache if the package gets rejected
<jnewbery> larryruane: disconnecting/banning is dangerous. If one of those damn attackers is able to make you send a transaction to your peer that would cause them to disconnect you, then they could use that to isolate you, or do it across the network to cause a network split
<larryruane> (for the newer people here, if a peer misbehaves in some possibly-innocuous way, we bump up its "ban score" which means we don't ban it yet, but if the ban score gets too high, then we do ban it (disconnect, i think?))
<jnewbery> if we gave a peer misbehavior points for causing us to fetch UTXOs from disk for a transaction that turned out to fail because of policy, then that could open up those eclipse/netsplit vectors.
<stickies-v> glozow you want to differentiate between two categories of package validation failure, one is network consensus failure (PCKG_BAD) and should not be accepted by anyone - so maybe bad faith? The other is local policy failure (PCKG_POLICY), i.e. the rules each node can tweak, which don't necessarily indicate malicious intent?
<jnewbery> I think in this PR, we don't do anything differently based on whether the package fails for `PCKG_BAD` or `PCKG_POLICY`, but we could do in future when we update p2p to be able to relay packages
<davidbak> there was a way in a previous PR to "dry run" packages (at that time) - could that have been done in this case? or ... is it or is it not a B.C. dev policy to modify RPC handling to account for new-but-incompletely exposed features?
<davidbak> so in the implementation of a feature over multiple PRs (what the Agile people would call stories in an epic) it is ok (from the POV of the maintainers) to have individual PRs that do not include their own testing as long as you get it in a subsequent PR? (trying to learn the dev philosophy here as well as this individual PR)
<jnewbery> There's a distinction between unit tests, which are testing the logic in individual units of code (in this case the mempool submission/acceptance logic), and functional tests, which test end-to-end user functionality.
<jnewbery> in general, I think this project relies too much on the python functional tests, because they're easy to write and our code is not structured in a way that makes it easy to isolate and test individual components
<glozow> that's one possibility. right now we don't have witness replacement ;) so the RPC will just return the transaction that's already in the mempool. if they have different witness sizes then this would be inaccurate!
<davidbak> jnewbery: i appreciate that comment, thanks; a different POV, by the way, is that if this PR already has PCKG_BAD then that _is_ an end-to-end user functionality; won't be _exposed_ until the P2P stuff is in later, but if not tested here it adds to the test burden _then_. But it's just an argument, I accept your conclusion that unit tests are appropriate here.
<larryruane> glozow: hope this isn't a sidetrack (ignore if you'd like) but is sigop weight a little like gas in ethereum, so that there's some consideration of the CPU time needed to evaluate scripts? I'm really not familar with sigop weight
<glozow> Miners seek to maximize the total transaction fees while ensuring that their blocks are within consensus-enforced weight and sigops limits. To simplify this 2-dimensional knapsack problem, in the mempool, virtual size of a transaction is calculated as the maximum between its BIP141 serialized size and its “sigop weight”.
<davidbak> not just extensive prep for this meeting, but extensive prep for the PR: the additional written documentation _with excellent drawings_ is a great example of a way to get a PR accepted (to my mind anyway, correct me if I'm wrong)