locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

glozow · 2021-01-03T07:17:17Z

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.~~

jnewbery

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()).

DrahtBot · 2021-01-03T11:28:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)

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.

michaelfolkson · 2021-01-03T13:51:32Z

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?

maflcko

Approach ACK. The code indeed doesn't seem to do anything.

src/validation.cpp

glozow · 2021-01-03T18:48:57Z

@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.

jnewbery · 2021-01-05T14:00:20Z

This code was added in commit b014668, specifically in response to this review comment: #10192 (comment).

The code was added here: #10192 (comment) before the change to per-txout db and later rebased: #10192 (comment).

I believe the reasoning is as follows:

if there's a bug in CCoinsViewMempool (e.g. returning the wrong coin when looking up by txid), then the script cache could cache an incorrect outcome for script validation. That is then used as an input to validating a block, meaning that CCoinsViewMempool has become part of consensus.
therefore, explicitly check that the hash of scriptPubKey returned by CCoinsViewMempool for this outpoint is correct. Either:
- fetch the entire transaction from the mempool via pool.get() and check that the hash is committing to the scriptPubKey; or
- lookup the coin in the UTXO set (pcoinsTip) and check that the scriptPubKey matches. Despite the name coinFromDisk, 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:

bitcoin/src/validation.cpp

Lines 528 to 572 in b014668

    
           LOCK(pool.cs); 
        
           CCoinsViewMemPool viewMemPool(pcoinsTip, pool); 
        
           view.SetBackend(viewMemPool); 
        
           // do we already have it? 
        
           for (size_t out = 0; out < tx.vout.size(); out++) { 
        
               COutPoint outpoint(hash, out); 
        
               bool had_coin_in_cache = pcoinsTip->HaveCoinInCache(outpoint); 
        
               if (view.HaveCoin(outpoint)) { 
        
                   if (!had_coin_in_cache) { 
        
                       coins_to_uncache.push_back(outpoint); 
        
                   } 
        
                   return state.Invalid(false, REJECT_ALREADY_KNOWN, "txn-already-known"); 
        
               } 
        
           } 
        
           // do all inputs exist? 
        
           BOOST_FOREACH(const CTxIn txin, tx.vin) { 
        
               if (!pcoinsTip->HaveCoinInCache(txin.prevout)) { 
        
                   coins_to_uncache.push_back(txin.prevout); 
        
               } 
        
               if (!view.HaveCoin(txin.prevout)) { 
        
                   if (pfMissingInputs) { 
        
                       *pfMissingInputs = true; 
        
                   } 
        
                   return false; // fMissingInputs and !state.IsInvalid() is used to detect this condition, don't set state.Invalid() 
        
               } 
        
           } 
        
           // Bring the best block into scope 
        
           view.GetBestBlock(); 
        
           nValueIn = view.GetValueIn(tx); 
        
           // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool 
        
           view.SetBackend(dummy); 
        
           // Only accept BIP68 sequence locked transactions that can be mined in the next 
        
           // block; we don't want our mempool filled up with transactions that can't 
        
           // be mined yet. 
        
           // Must keep pool.cs for this unless we change CheckSequenceLocks to take a 
        
           // CoinsViewCache instead of create its own 
        
           if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) 
        
               return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final"); 
        
           }

). 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.

sdaftuar · 2021-01-05T15:31:41Z

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?)

src/validation.cpp

glozow · 2021-01-05T19:33:40Z

@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.

jnewbery · 2021-01-06T12:09:54Z

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.

src/validation.cpp

sdaftuar · 2021-01-06T14:59:04Z

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 (comment)

glozow · 2021-01-06T22:09:43Z

@sdaftuar Thanks for your thorough review - I'm no longer removing CIFMAC. I think the lock annotations are still helpful, though, and in general would like to update the documentation to clarify what this function does.

maflcko · 2021-01-07T07:50:41Z

Concept ACK for updating locks and docs

jnewbery

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.

src/validation.cpp

sdaftuar · 2021-01-07T16:13:44Z

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.

sdaftuar

Concept ACK -- just a couple small questions.

src/validation.cpp

sdaftuar · 2021-01-08T17:50:26Z

src/validation.cpp

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.

src/validation.cpp

We should already have the mempool lock when entering CheckInputsFromMempoolAndCache

sdaftuar · 2021-01-12T17:32:30Z

utACK 2f463f5

DrahtBot · 2021-01-13T11:45:46Z

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

maflcko

cr ACK 2f463f5

src/validation.cpp

maflcko · 2021-01-13T14:46:22Z

src/validation.cpp

+/**
+* 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.


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?

maflcko · 2021-01-13T14:53:21Z

src/validation.cpp


-        // Check equivalence for available inputs.
+        // If the Coin is available, there are 2 possibilities:
+        // 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

Suggested change

// it is available in our current ChainstateActive UTXO set,

// it is available in our current active UTXO set,

jnewbery · 2021-01-13T16:56:27Z

utACK 2f463f5

fanquake

ACK 2f463f5

…lAndCache

Summary: > 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 is a backport of [[bitcoin/bitcoin#20834 | core#20834]] Notes: the `PolicyScriptChecks` and `CheckFeeRate` methods don't exist in our codebase, they are part of `PreChecks`. See D8203. Test Plan: With clang and debug: `ninja all check-all` With clang and TSAN: `ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11767

glozow mentioned this pull request Jan 3, 2021

rpc/validation: enable packages through testmempoolaccept #20833

Merged

fanquake added the Validation label Jan 3, 2021

maflcko added the Refactoring label Jan 3, 2021

jnewbery reviewed Jan 3, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 3, 2021

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750

Merged

maflcko reviewed Jan 3, 2021

View reviewed changes

src/validation.cpp Show resolved Hide resolved

jnewbery reviewed Jan 5, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 5, 2021

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

bitcoin deleted a comment from sheffine Jan 5, 2021

maflcko reviewed Jan 6, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2021-01-validation-cleanup branch from 747ae56 to 89da5ed Compare January 6, 2021 21:56

glozow changed the title ~~cleanup: remove CheckInputsFromMempoolAndCache~~ locks and docs in ATMP and CheckInputsFromMempoolAndCache Jan 6, 2021

jnewbery reviewed Jan 7, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

sdaftuar reviewed Jan 8, 2021

View reviewed changes

glozow force-pushed the 2021-01-validation-cleanup branch from 89da5ed to 479f6f7 Compare January 8, 2021 21:38

sdaftuar suggested changes Jan 11, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2021-01-validation-cleanup branch from 479f6f7 to 8027938 Compare January 12, 2021 10:13

glozow added 2 commits January 12, 2021 02:27

lock annotations for MemPoolAccept functions

85cc6be

We should already have the mempool lock when entering CheckInputsFromMempoolAndCache

[doc] for CheckInputsFromMempoolAndCache

2f463f5

glozow force-pushed the 2021-01-validation-cleanup branch from 8027938 to 2f463f5 Compare January 12, 2021 10:28

maflcko approved these changes Jan 13, 2021

View reviewed changes

fanquake approved these changes Jan 14, 2021

View reviewed changes

fanquake merged commit f91587f into bitcoin:master Jan 14, 2021

glozow deleted the 2021-01-validation-cleanup branch January 14, 2021 18:17

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 15, 2021

Merge bitcoin#20834: locks and docs in ATMP and CheckInputsFromMempoo…

4c89b81

…lAndCache

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	// it is available in our current ChainstateActive UTXO set,
	// it is available in our current active UTXO set,

locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

Uh oh!

Conversation

glozow commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

michaelfolkson commented Jan 3, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Jan 3, 2021

Uh oh!

jnewbery commented Jan 5, 2021

Uh oh!

sdaftuar commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

glozow commented Jan 5, 2021

Uh oh!

jnewbery commented Jan 6, 2021

Uh oh!

Uh oh!

sdaftuar commented Jan 6, 2021

Uh oh!

glozow commented Jan 6, 2021

Uh oh!

maflcko commented Jan 7, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdaftuar commented Jan 7, 2021

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdaftuar Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdaftuar commented Jan 12, 2021

Uh oh!

DrahtBot commented Jan 13, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jan 13, 2021

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

glozow commented Jan 3, 2021 •

edited

Loading

DrahtBot commented Jan 3, 2021 •

edited

Loading

sdaftuar commented Jan 5, 2021 •

edited

Loading

maflcko Jan 13, 2021 •

edited

Loading