fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless.
This is extracted from #13233 /cc TheBlueMatt.
Recommend reviewing with git show --ignore-all-space, i.e.:
https://github.com/bitcoin/bitcoin/pull/13868/files?w=1
1391@@ -1392,7 +1392,7 @@ void InitScriptExecutionCache() {
1392 *
1393 * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp
1394 */
1395-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks)
1396+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks)
1397 {
1398 if (!tx.IsCoinBase())
1399 {
if (tx.IsCoinBase()) return true; in the first line.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
1422- CSHA256().Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
1423- AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
1424- if (scriptExecutionCache.contains(hashCacheEntry, !cacheFullScriptStore)) {
1425- return true;
1426- }
1427+ if (tx.IsCoinBase()) {
utACK 1d7cc70.
Recommend reviewing with git show –ignore-all-space
Also view with&w=1.
Concept ACK, but this removes the quite useful comment about how assume valid works:
0 // Skip script verification when connecting blocks under the
1 // assumevalid block. Assuming the assumevalid block is valid this
2 // is safe because block merkle hashes are still computed and checked,
3 // Of course, if an assumed valid block is invalid due to false scriptSigs
4 // this optimization would allow an invalid chain to be accepted.
Perhaps you can incorporate that information after the line:
Rebased and I split the coinbase change into another commit.
Once again: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1
1378+
1379+ if (pvChecks) {
1380+ pvChecks->reserve(tx.vin.size());
1381+ }
1382+
1383+ // The first loop above does all the inexpensive checks.
fScriptChecks = false just short-circuits the entire function, so
passing it in is entirely useless.
AcceptToMemoryPoolWorker.
ACK 9b92538adebd30353ffc1a427bc20ab0c3fc75f1 - Notes / testing below.
CheckInputs() is called from:
CheckInputsFromMempoolAndCache() - fScriptChecks was true
AcceptToMemoryPoolWorker() - 3 times and fScriptChecks was always true
CChainState::ConnectBlock() - fScriptChecks is true unless
Changes to CheckInputs():
fScriptChecks parameter.IsCoinBase() check is now clearer. i.e0if (!tx.IsCoinBase()) {
1 // do input checking
2}
3return true
is now:
0if (tx.IsCoinBase()) return true;
1// do input checking
2return true
pvChecks check.AcceptToMemoryPoolWorker.ConnectBlock.Added some logging and tested:
-assumevalid=garbage hash fScriptChecks was true (early IBD).-assumevalid=garbage - checked that we drop into the ConnectBlock error.Other thought. It looks like everywhere we call CheckInputs() we are already checking prior that the tx is not a coinbase. Although I cannot see a downside to a belt-and-suspenders check inside CheckInputs().
@jnewbery can you re-review here?
ACK 9b92538adebd30353ffc1a427bc20ab0c3fc75f1
Verified that function does essentially nothing if the flag is false.
assert(!tx.IsCoinBase());.
Could change the coinbase early exit to assert(!tx.IsCoinBase());
I think that can be discussed / done in a follow up. We’ve got ACKs, so I’m merging this.
Thanks to those of you that I harassed for review for following up. Post-merge ACKs still welcome.
772@@ -773,15 +773,17 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
773 constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS;
774
775 // Check against previous transactions
776- // This is done last to help prevent CPU exhaustion denial-of-service attacks.
777+ // The first loop above does all the inexpensive checks.
This is a really confusing comment here. “The first loop” refers to the checks inside CheckTxInputs() call on L566, which has been moved around since the comment was first written.
I’ll fix this comment in a follow-up PR.