The PR branch HEAD was ebca80d at the time of this review club meeting.
A package is a group of transactions that have some dependency
relationship, i.e., some transaction spends the outputs of another
transaction in the package.
A test accept of validation refers to an invocation of mempool validation
that validates but does not submit a transaction to the mempool, called from
the testmempoolaccept RPC.
A Coin (CCoin)
is a UTXO. In our context, coins don’t necessarily need to exist on-chain;
they can be outputs from transactions in the mempool or some other valid but
unconfirmed transaction. A Coins View is a data structure that contains
the available coins. The typical usage is to query a Coins View with a
The testmempoolaccept RPC was introduced in
#11742 and allows users to
test raw transactions for validity. While the return object is an array (with
the intention to allow multiple transactions), testmempoolaccept is currently
limited to one transaction at a time, which means users cannot test
transactions that depend on outputs that the mempool has not seen yet. The main
reason for this limitation is that our validation code only uses the coins
available in the current chain state and mempool (and not anything else), to
validate transaction inputs.
#20833 is also intended to be
a first step towards Package Mempool Accept and hopefully, eventually, Package
Relay. We have discussed
Package Relay in a previous review club,
#16401. You can read more about Package
#20833 enables dry runs of
package validation. It does this by having CCoinsViewMemPool keep track of
the Coins created and spent by transactions in the package.
class represents a “validation session” with the current mempool and chain
state; it holds cs_main and pool.cs mutexes throughout validation. Its job
is to figure out whether a transaction (received from peers or clients like
RPC) is a valid candidate for submission to the mempool. To do so, it applies
consensus and policy rules.
Importantly, if a transaction is invalid, MemPoolAccept is designed such that
it fails as quickly as possible to mitigate DoS risks.
Each MemPoolAccept instance creates a Coins View from the current chain tip
and mempool. CCoinsView is a class with a hierarchical structure (see
A CCoinsViewBacked points to another CCoinsView as its “backend.” When
CCoinsViewBacked.GetCoin() cannot find a coin in its own data structures,
it calls backend.GetCoin() to continue searching. Thanks to the magic of
polymorphism, CCoinsViewBacked Coins Views can be “layered” to allow
searching for coins in multiple locations without changing the underlying chain
state. We have discussed the layered UTXO cache in a previous review
Since the UTXO set is quite large and stored on disk, Bitcoin Core uses a
cache to improve validation performance. To prevent cache thrashing caused
by an attacker broadcasting many invalid transactions spending a wide variety
of UTXOs (a cheap DoS attack), we keep track of which Coins are brought in from
disk and uncache them if the transaction turns out to be invalid.
Validation roughly consists of PreChecks, PolicyScriptChecks,
ConsensusScriptChecks, and Finalize. We’ve touched on these functions in
previous review clubs, e.g. #19339. This
about consensus vs policy rules may also be helpful.
In an effort to not make any API-breaking changes, #20833 returns a results
array with one result per transaction in the same order they were passed in
or the first failure result.
What are some use cases for enabling packages through testmempoolaccept?
What are some limitations of this implementation of package validation?
From a code quality standpoint, what is the benefit of moving the
validation state, replaced transactions, and fee to Workspace? Should we
add coins_to_uncache to Workspace and make ATMPArgs a const? Why or why
Why do we run all PreChecks before script checks? Would it still be
correct if we didn’t do this?
(with test_accept=true), both PolicyScriptChecks and
ConsensusScriptChecks are run. However, in
we don’t run ConsensusScriptChecks. Why is that okay? (Followup question:
why do we run ConsensusScriptChecks at all in ATMP?)
When we fetch a coin from m_viewmempool, what 4 places do we look and in
what order? Why do we check cache_package_remove before
cache_package_add in CCoinsViewMemPool::GetCoin?
If a block arrives while we’re running PreChecks, do we have to worry
about our CoinsView becoming inconsistent?
Why do we set our CoinsView backend to dummy
What bug is this trying to prevent?
What are your thoughts on the results array (returning one for each tx if
successful or the first one if failed)? Do you have any suggestions for
improvement, and is it API-breaking?
<glozow> ok let's start with some conceptual questions. What are some use cases for enabling packages through testmempoolaccept? (non-package-relay related please, since that’s very far off from this PR)
<sishir> This allows you to test validity for second-layer application transaction chains (i.e. with multiple spending paths and where you don't want to broadcast yet).it could be helpful to return more information to the client, but don't know what that would look like concretely
<stacie> Another limitation is the atomic validation - the response is only for the first failure, so if you have multiple, you'll need to go back and correct each one before calling testmempoolaccept again
<glozow> The PR starts off with a few refactors. From a code quality standpoint, what is the benefit of moving the validation state, replaced transactions, and fee to `Workspace`? Should we add `coins_to_uncache` to `Workspace `and make `ATMPArgs` a const? Why or why not?
<stacie> ariard - agree, and I think the atomic validation is a good thing, but I wonder if people will use this new feature to try validating a set of unrelated transactions. The only use case I can see is to save on the network overhead of multiple RPC calls, but it does seem strange to need to batch/package up a bunch of unrelated transactions ¯\_(ツ)_/¯
<glozow> sishir: policy includes non-consensus rules that Bitcoin Core nodes enforce on unconfirmed transactions, mostly to protect from DoS. Some of the checks are in `PreChecks`, and some are in `PolicyScriptChecks`
<glozow> Next question: In `AcceptSingleTransaction` (with test_accept=true), both `PolicyScriptChecks` and `ConsensusScriptChecks` are run. However, in `AcceptMultipleTransactions`, we don’t run `ConsensusScriptChecks`. Why is that okay?
<ariard> glozow: I think we don't want to run `ConsensusScriptChecks` because for now those txn aren't added to the mempool and thus there is no matter caching their script results for a potential block validation?
<AnthonyRonning> `cache_package_remove`, `cache_package_add`, mempool, and then backend (disk?). We check removed first because a package tx before could have already spent another package tx’s output.
<glozow> robot-dreams: I think that's a "just in case there is a bug even though we don't think there's a bug" type of thing. policy flags as a superset of consensus flags should guarantee this. (btw we had a whole review club on this topic hehe https://bitcoincore.reviews/19698)
<jnewbery> robot-dreams: a consensus-invalid transaction getting into the mempool is bad news for the miners. They'd construct a block including that transaction and then later find out that the block is invalid.
<jnewbery> glozow: on you question about TestBlockValidity, yes it looks like this is called before getblocktemplate returns a block template. Although I don't know what the advice would be to a miner who saw a "TestBlockValidity() failed" error.
<glozow> last question before we wrap up for today: What are your thoughts on the results array (returning one for each tx if successful or the first one if failed)? Do you have any suggestions for improvement?
<lightlike> glozow: would that be an actual attack that would do something worse than slow us down a bit? I'm not so familiar with the topic, but if we have useless full cache, wouldn't we just load more txes from disk until the cache gets better again?
<robot-dreams> murtyjones: agreed though similar to how compiler writers want to avoid spamming users with errors if they missed a semicolon (and caused the next 500 lines to all have errors), maybe we want some way to show only "uncorrelated" errors
<jnewbery> there's not time to get into this in great detail now, but the UTXO cache has a slightly unusual access pattern compared to many caches that people are familiar. The main thing it's saving us is _writes_, not _reads_
<jnewbery> if a coin gets spent shortly after it gets created, we never have to write it to disk at all, and many coins get spent in the same block or the next few blocks after they get created (see page 14 of https://eprint.iacr.org/2019/611.pdf)
<stacie> ah! I see it in that comment, "A future pull can extend this rpc to take lists of more than one raw transaction and feed them into an ephemeral mempool (e.g. based on the current mempool)." thanks!
<robot-dreams> Following up on something I didn't understand from earlier, how come we do `ConsensusScriptChecks` in the testmempoolaccept -> AcceptSingleTransaction path, but not in the AcceptMultipleTransactions path?
<michaelfolkson> My understanding is that the use case for package relay is where you construct a chain of transactions that you will broadcast onchain at some point in the future (days, weeks, months)
<robot-dreams> Here's my current understanding. If we're adding a transaction to the mempool, we bring the inputs into cache, validate, then remove them (because they're spent). If we're TESTING though, we bring the inputs into cache, validate, then manually uncache them?
<robot-dreams> ariard: Earlier I wasn't referring to fee bumping a justice transaction with CPFP, I was referring to double checking that the chain of 2 transactions would be accepted together even though the commitment transaction hadn't been broadcast yet. Would that use case still make sense in the face of malleable justice transactions?
<ariard> robot-dreams: I think I understand your question better, you mean being sure you can punish correctly a revoked commitment. Yes it makes sense to use testmempoolaccept when you manually assert correctness of your transaction generation code