This is a very small PR that adds some lock annotations to clarify that, now, the pool.cs lock is held throughout tx validation for mempool. The comments in CheckInputsFromMempoolAndCache were unclear/outdated so I updated those as well.
~This PR is a cleanup. It removes unnecessary code that doesn't do much.~
Concept ACK.
I think you could update the PR description to say that CIFMAC also does:
CheckInputScripts() to cache signature and script validity against current tip consensus rules.in order for script caching to work, and that you're retaining that check and moving it into the calling function (ConsensusScriptChecks()).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
Concept ACK. Great PR description, really informative.
Is there anything in particular you'd recommend a reviewer do to assure themselves that removing this code doesn't cause any problems beyond the usual building, running tests etc?
431 | - assert(txFrom->vout.size() > txin.prevout.n); 432 | - assert(txFrom->vout[txin.prevout.n] == coin.out); 433 | - } else { 434 | - const Coin& coinFromDisk = ::ChainstateActive().CoinsTip().AccessCoin(txin.prevout); 435 | - assert(!coinFromDisk.IsSpent()); 436 | - assert(coinFromDisk.out == coin.out);
I assumed this was protecting against corruption of the utxo set on disk, but that doesn't seem to be the case. coin will be from disk as well.
Is that true? AccessCoin() doesn't necessarily hit the disk. It could be pulling the coin from the cache.
Approach ACK. The code indeed doesn't seem to do anything.
@michaelfolkson good question. You could go line-by-line in the function and verify that what I said in the description is true, i.e. that each one is done earlier in ATMP. And then you can also look at what cs_main and pool.cs guarantee wrt consistency so you can say "yeah, this wouldn't change." Might be fun to trace the coinsview through ATMP with some logs. If you prefer writing code, you can try to break it, e.g. with a functional test with badly formed/similar but conflicting Coins.
This code was added in commit https://github.com/bitcoin/bitcoin/commit/b014668e27b496bd6ad30985294f3d6971311910, specifically in response to this review comment: #10192 (comment).
The code was added here: #10192#event-1049081888 before the change to per-txout db and later rebased: #10192 (comment).
I believe the reasoning is as follows:
pool.get() and check that the hash is committing to the scriptPubKey; orcoinFromDisk, I believe that the coin could be fetched either from disk or the in-memory pcoinsTip cache. pcoinsTip is already part of consensus, so if it's returning bad data, then we're already in trouble.I'm not sure we really need this. We could move these checks to immediately after the coins are fetched from CCoinsViewMempool. At that point we're basically adding duplicate code to ensure that there isn't a bug in CCoinsViewMempool, which itself is only a few lines of code. We may as well just move CCoinsViewMempool into validation.cpp as suggested in the original PR by @sdaftuar:
as per offline discussion, perhaps just move CCoinsViewMemPool into validation.cpp to make it clear it's part of consensus, and sanity check the results from the mempool.
Another interesting thing to note is that at the time this was introduced, the mempool lock was not held for the duration of ATMP (e.g. it'd be released after this block: https://github.com/bitcoin/bitcoin/blob/b014668e27b496bd6ad30985294f3d6971311910/src/validation.cpp#L528-L572). That was added later in #12368. I don't think it actually changes anything for the purposes of ATMP (since cs_main was held for the duration), but it does make expectations clearer.
I'm pretty skeptical of this change. I think the OP downplays the consensus risk if a bug is introduced in the CCoinsViewMempool or the mempool (which CCoinsViewMempool relies on).
I also took a quick look at #20833, and it seems to make that code much higher risk if we would be turning CCoinsViewMempool into something consensus critical and then make a bunch of changes to it. Is there a method for implementing #20833 that does not require this?
(I was going to suggest looking at what I proposed in #16401, which faced the same issue, but now I suspect there may be a consensus bug there as well -- yikes. EDIT: Looked into this a bit more, I believe #16401 had a bug relating to packages of transactions that conflict with themselves, because the duplicate inputs check only occurs on a per-transaction basis, so when processing a package we need to check for duplicate inputs across the whole package. But that seems like an easy fix (and #20833 seems to address this exact issue already), so now I think that the structure/usage of CCoinsViewMempool and CIFMAC implemented there may work after all?)
166 | @@ -167,6 +167,24 @@ namespace { 167 | std::set<int> setDirtyFileInfo; 168 | } // anon namespace 169 | 170 | +CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { } 171 | + 172 | +bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const { 173 | + // If an entry in the mempool exists, always return that one, as it's guaranteed to never 174 | + // conflict with the underlying cache, and it cannot have pruned entries (as it contains full) 175 | + // transactions. First checking the underlying cache risks returning a pruned entry instead.
Everything after ", and it cannot have pruned ..." can be removed. There's no such thing as pruned coins cache entries since per-txout db.
Up to you. I only mention it here because you're touching this code, but if you want to keep this move-only, then that's fine as well.
@jnewbery @sdaftuar Thanks for your input and for digging into things! I agree that a bug here would be awful as we could potentially cache incorrect script checks, affecting consensus. I think these lines from CIFMAC are a nice sanity check:
const CTransactionRef& txFrom = m_pool.get(txin.prevout.hash);
if (txFrom) {
assert(txFrom->GetHash() == txin.prevout.hash);
assert(txFrom->vout.size() > txin.prevout.n);
assert(txFrom->vout[txin.prevout.n] == coin.out);
} else {
const Coin& coinFromDisk = ::ChainstateActive().CoinsTip().AccessCoin(txin.prevout);
assert(!coinFromDisk.IsSpent());
assert(coinFromDisk.out == coin.out);
}
The main issues are with them being so late in ATMP.
as per offline discussion, perhaps just move CCoinsViewMemPool into validation.cpp to make it clear it's part of consensus, and sanity check the results from the mempool.
We could move these checks to immediately after the coins are fetched from CCoinsViewMempool.
I've moved CCoinsViewMemPool to validation and moved the sanity checks to PreChecks (instead of removing them), right after we get the coins. Now that we hold the lock throughout ATMP, I hope it's clear we don't need to do these exact same checks again later.
I believe #16401 had a bug relating to packages of transactions that conflict with themselves, because the duplicate inputs check only occurs on a per-transaction basis, so when processing a package we need to check for duplicate inputs across the whole package. But that seems like an easy fix (and #20833 seems to address this exact issue already)
Yes exactly, this cleanup came up while I was trying to figure out conflicting transactions. I did a light rebase of #20833 on top of this and this condition needs to be added, but otherwise it still works.
I like the way this is going. CCoinsViewMempool should live in validation.cpp and be considered consensus critical since script caching was introduced. Making that clear by reflecting it in the code structure, rather than working around it by double checking the results returned from CCoinsViewCache seems like an improvement.
As far as I'm aware, CTxMemPool is not consensus critical, since any bugs in there would not cause us to incorrectly validate a block. Making the boundary between consensus-critical and non-consensus-critical code clearer is a good goal and fits with the current direction of the project (eg see #20158).
It'd be nice to eventually have ChainstateActive expose a GetCoin() method that returns a coin from the UTXO set or mempool so that callers (rpc, rest, etc) don't need to concern themselves with locking cs_main/mempool.cs and setting the coins view. After that happens, CCoinsViewMempool can exist just in validation.cpp and not be exposed in the header.
638 | @@ -658,6 +639,19 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) 639 | // Otherwise assume this might be an orphan tx for which we just haven't seen parents yet 640 | return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent"); 641 | } 642 | + // Sanity check each input.
nit: Could explain a bit more what this sanity check is doing? Maybe a oneline summary of #20834 (comment) ?
Made big effort to improve the documentation :D
Concept NACK. I think the current structure of using CIFMAC to encapsulate the consensus requirements of the script cache is better than spreading the checks out across validation, where it is less clear why these checks exist and what they protect against. Moreover, I don't believe this change is necessary (or desired) for #20833, as I commented here: #20833 (review)
Concept ACK for updating locks and docs
409 | +// Checks to avoid mempool polluting consensus critical paths since cached 410 | +// signature and script validity results will be reused if we validate this 411 | +// transaction again during block validation. 412 | static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& view, const CTxMemPool& pool, 413 | - unsigned int flags, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { 414 | + unsigned int flags, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) {
While you're touching these lines, feel free to:
-// Checks to avoid mempool polluting consensus critical paths since cached
-// signature and script validity results will be reused if we validate this
-// transaction again during block validation.
-static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& view, const CTxMemPool& pool,
- unsigned int flags, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) {
+/**
+ * Checks to avoid mempool polluting consensus critical paths since cached
+ * signature and script validity results will be reused if we validate this
+ * transaction again during block validation.
+ * */
+static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationState& state,
+ const CCoinsViewCache& view, const CTxMemPool& pool,
+ unsigned int flags, PrecomputedTransactionData& txdata)
+ EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
+{
Done!
utACK 89da5ed578b4839c426151960594bd4a9bf6d4f7
This is a definite improvement to the documentation/naming.
It's not clear to me why we want CheckInputsFromMempoolAndCache() to be a separate function from ConsensusScriptChecks(). It means that the documentation is split out over three places - the ConsensusScriptChecks() declaration ("Re-run the script checks, using consensus flags, and try to cache the result in the scriptcache..."), the ConsensusScriptChecks() function body ("Check again against the current block tip's script verification flags to cache our script execution flags. This is, of course, useless...") and the CheckInputsFromMempoolAndCache() function comment ("Checks to avoid mempool polluting consensus critical paths ..."). It would be much clearer if the code from CheckInputsFromMempoolAndCache() was brought into ConsensusScriptChecks() and fully documented in one place.
Maybe there's a reason that the code is structured this way and the intent is to call CheckInputsFromMempoolAndCache() from more than one place. If not, then I think a future PR could consolidate them. In any case, this PR is an improvement and stands on its own.
It's not clear to me why we want CheckInputsFromMempoolAndCache() to be a separate function from ConsensusScriptChecks(). It means that the documentation is split out over three places - the ConsensusScriptChecks() declaration ("Re-run the script checks, using consensus flags, and try to cache the result in the scriptcache..."), the ConsensusScriptChecks() function body ("Check again against the current block tip's script verification flags to cache our script execution flags. This is, of course, useless...") and the CheckInputsFromMempoolAndCache() function comment ("Checks to avoid mempool polluting consensus critical paths ..."). It would be much clearer if the code from CheckInputsFromMempoolAndCache() was brought into ConsensusScriptChecks() and fully documented in one place.
Note that ConsensusScriptChecks really exists for two reasons: it's a safeguard against policy script checks somehow not being a superset of consensus script checks (a software bug that could lead to invalid blocks being produced), and it conveniently hooks into caching script validation success to serve as a huge optimization for block validation. If this is unclear, then the documentation should be improved.
Maybe there's a reason that the code is structured this way and the intent is to call CheckInputsFromMempoolAndCache() from more than one place. If not, then I think a future PR could consolidate them. In any case, this PR is an improvement and stands on its own.
Somewhere we should draw a line between mempool validation and block validation; our philosophy has long been to not allow mistakes during mempool validation to cause block validation errors, because we have had historical bugs that have cropped up over the years that could have either caused consensus splits or invalid blocks to be mined. (One example that I recall is #6077, which could have broken consensus; I heard stories that prior to my time working on this project, there were prior bugs that allowed invalid transactions in the mempool, which is the motivation for TestBlockValidity to be called before returning block candidates to miners.)
Script caching of course is a huge performance win, justifying a desire to have mempool validation interact with block validation. So an important part of how we use mempool validation to speed up block validation is designing a system that is robust to differences in mempool vs block validation (the script cache design itself tries to address many of these concerns, in comparison with prior attempts to cache validation success). And in particular, finding a way to draw the line between mempool and block validation so that any future bugs introduced in mempool validation would not result in a consensus split is also very important.
Drawing this line at CIFMAC seems to have held up pretty well. At the least, mempool logic was refactored in preparation for package validation back in #16400 (which is when MemPoolAccept::ConsensusScriptChecks was introduced, after CIFMAC's introduction) and is being modified again a bit in #20833, yet CIFMAC doesn't need to be touched to accommodate the feature (and indeed seems mostly untouched since first introduced in 2017). I think that is a positive sign that we've drawn a reasonably good boundary, as it seems to allow for improvements in one side of the code without touching consensus.
So while we could move this consensus critical function into the MemPoolAccept::ConsensusScriptChecks() function and eliminate a function call, I think doing so would blur the line rather than clarify it (for instance, CIFMAC doesn't need access to the MemPoolAccept state), and would make it more likely that we introduce consensus breaking bugs around the script cache in the future when code relating to mempool acceptance is modified again. At the least, it would raise the review burden for any future PR that would touch mempool acceptance / ConsensusScriptChecks and which otherwise wouldn't need to touch CIFMAC, in its current form. I think this would be a bad idea.
At a higher level: I really think that we should have a very high bar for refactoring or otherwise changing consensus-critical code -- which CIFMAC most certainly is (as the interface to the script cache), and this PR didn't seem to acknowledge when first opened. Sometimes we'll have to modify consensus critical code to fix a bug or implement a feature, and we'll take that review burden and effort when necessary, but when unnecessary I think the cost/benefit of changing consensus code tilts strongly towards not making changes.
As for the current version of this PR: @glozow thank you for taking the feedback! At first glance it looks much better to me as just doc changes and lock annotations -- will take a closer look.
425 | - // available, so this shouldn't fail. If the inputs are not available 426 | - // here then return false. 427 | - if (coin.IsSpent()) return false; 428 | + // This coin was checked in PreChecks and MemPoolAccept 429 | + // has been holding cs_main since then. 430 | + assert(!coin.IsSpent());
Now that we have Assume() (#20255), that might be a good choice to use here? It seems to me that we don't need to crash if this assumption is violated (as the old code handled this case and returned false, so we could just continue to do that) -- but adding the Assume() will mean we do get the crash in debug builds, which I think is what we're looking for.
Ah, I wasn't aware of that! Yes I think that's better.
496 | @@ -502,13 +497,13 @@ class MemPoolAccept 497 | 498 | // Run the script checks using our policy flags. As this can be slow, we should 499 | // only invoke this on transactions that have otherwise passed policy checks. 500 | - bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); 501 | + bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
Interestingly, PolicyScriptChecks doesn't itself need the mempool, so on its face this seems slightly strange. I can understand that the semantics may be easiest to reason about if we try to enforce that cs_main and the mempool.cs locks are held throughout mempool acceptance, so no objection to that idea conceptually. But it's not clear to me if there's some better idiom to express this? As it is, if the caller unlocked the mempool in between calls to these internal functions and then relocked before making the next call, the annotations would not catch that, but I think our understanding is that we want to prevent that from happening.
Anyway, if we go down this route, is it worth adding these lock annotations to the CheckFeeRate helper function inside MemPoolTest as well? I think that's the only one that would be left un-annotated, otherwise.
I've added annotations to CheckFeeRate for now.
In my mind, I think of MemPoolAccept class as one "validation session" on top of current Tip and mempool, so would want to assert that it holds cs_main, pool.cs for the entirety of AcceptSingleTransaction. Don't know how to do that though 😅 not familiar
The non-static functions need EXCLUSIVE_LOCKS_REQUIRED in any header files which include them as well - EXCLUSIVE_LOCKS_REQUIRED will be ignored by call-sites if its only set in the definitions and not all declarations.
Concept ACK -- just a couple small questions.
425 | - // available, so this shouldn't fail. If the inputs are not available 426 | - // here then return false. 427 | - if (coin.IsSpent()) return false; 428 | + // This coin was checked in PreChecks and MemPoolAccept 429 | + // has been holding cs_main since then. 430 | + Assume(!coin.IsSpent());
I believe we need to leave in the old line that returned false if this test fails (after this Assume() call); this check is a no-op in non-debug builds, so we'd want to return false if the check were to fail.
oops! fixed
We should already have the mempool lock when entering
CheckInputsFromMempoolAndCache
utACK 2f463f57e3a9797236142a525703359a98fe19ea
424 | - // AcceptToMemoryPoolWorker has already checked that the coins are 425 | - // available, so this shouldn't fail. If the inputs are not available 426 | - // here then return false. 427 | + // This coin was checked in PreChecks and MemPoolAccept 428 | + // has been holding cs_main since then. 429 | + Assume(!coin.IsSpent());
in commit " lock annotations for MemPoolAccept functions ":
The assumption was that even though cs_main was locked the whole time, but the mempool lock wasn't (guaranteed to), so previously this was a "weak check" (return)? If so: Now that the mempool lock is taken (on top of cs_main) the whole time, there is no way this could fail. So it could make sense to change to Assert?
Also, the policy check did the assertion already, so it seems confusing to check the same with a weaker Assume.
I had Assert and changed to Assume based on @sdaftuar's comment 😅
we don't need to crash if this assumption is violated (as the old code handled this case and returned false, so we could just continue to do that) -- but adding the Assume() will mean we do get the crash in debug builds, which I think is what we're looking for.
Just a nit. It seems a bit odd to assume the Assert here could fail and should be an Assume instead. Then, at the same time assume that the other asserts (before and after this one) checking the exact same thing won't hit. Seems more consistent to have all three be Assert, or even remove this one completely and rely on the others.
If we thought the Assume could fail, we would fix the code, not add an Assume. I think it's a question of what do you want the software to do if we are all somehow wrong in our reasoning. In my view, crashing -- which could be a network-wide event -- should be avoided if the software is otherwise able to recover, and since that seems to be the case here, I don't know why we would change the code to crash. It just makes this change needlessly risky, even if it's low-probability.
I think that there are other uses of assert in our codebase that I'd probably disagree with too, which is why I don't find it convincing if the argument is that some other assert would already trigger.
I agree that this is a nit either way and if it had already been written with an assert, I wouldn't be likely to open a PR suggesting that we change it. But I would personally prefer to avoid ACKing a change that is later found to cause a network-wide crash when fed the wrong data over the wire, simply because of an oversight during the review process!
Thanks, makes sense
409 | -static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& view, const CTxMemPool& pool, 410 | - unsigned int flags, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { 411 | +/** 412 | +* Checks to avoid mempool polluting consensus critical paths since cached 413 | +* signature and script validity results will be reused if we validate this 414 | +* transaction again during block validation.
in commit " [doc] for CheckInputsFromMempoolAndCache ":
Could make sense to refer to g_scriptExecutionCache to clarify this has nothing to do with the schnorr-cache and ecdsa-cache?
438 | + Assume(!coin.IsSpent()); 439 | if (coin.IsSpent()) return false; 440 | 441 | - // Check equivalence for available inputs. 442 | + // If the Coin is available, there are 2 possibilities: 443 | + // it is available in our current ChainstateActive UTXO set,
in commit " lock annotations for MemPoolAccept functions ":
ChainstateActive is deprecated, so to make @dongcarl 's life easier it might be best to not add new references to it
// it is available in our current active UTXO set,
cr ACK 2f463f57e3a9797236142a525703359a98fe19ea
utACK 2f463f57e3a9797236142a525703359a98fe19ea
ACK 2f463f57e3a9797236142a525703359a98fe19ea