From 0b4395d27551d1c4abf0095928b8d77399004f10 Mon Sep 17 00:00:00 2001 From: Aditya Kulkarni Date: Thu, 30 May 2019 14:23:33 -0700 Subject: [PATCH 01/10] Add a config option to skip transaction verification in IBD mode --- src/init.cpp | 1 + src/main.cpp | 49 +++++++++++++++++++++++++++++++------------------ 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0b44993ad..65a646c11 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -342,6 +342,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-debuglogfile=", strprintf(_("Specify location of debug log file: this can be an absolute path or a path relative to the data directory (default: %s)"), DEFAULT_DEBUGLOGFILE)); strUsage += HelpMessageOpt("-exportdir=", _("Specify directory to be used when exporting data")); + strUsage += HelpMessageOpt("-ibdskiptxverification", _("Skip Tx verification during initial block download")); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); strUsage += HelpMessageOpt("-par=", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), diff --git a/src/main.cpp b/src/main.cpp index c364927a4..128e860a0 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4059,6 +4059,11 @@ bool CheckBlock(const CBlock& block, CValidationState& state, return state.DoS(100, error("CheckBlock(): more than one coinbase"), REJECT_INVALID, "bad-cb-multiple"); + // If this is initial block download and "ibdskiptxverification" is set, we'll skip verifying the transactions + if (IsInitialBlockDownload(chainparams) && GetBoolArg("-ibdskiptxverification", false)) { + return true; + } + // Check transactions BOOST_FOREACH(const CTransaction& tx, block.vtx) if (!CheckTransaction(tx, state, verifier)) @@ -4149,24 +4154,6 @@ bool ContextualCheckBlock( const int nHeight = pindexPrev == NULL ? 0 : pindexPrev->nHeight + 1; const Consensus::Params& consensusParams = chainparams.GetConsensus(); - // Check that all transactions are finalized - BOOST_FOREACH(const CTransaction& tx, block.vtx) { - - // Check transaction contextually against consensus rules at block height - if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true)) { - return false; // Failure reason has been set in validation state object - } - - int nLockTimeFlags = 0; - int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) - ? pindexPrev->GetMedianTimePast() - : block.GetBlockTime(); - if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) { - return state.DoS(10, error("%s: contains a non-final transaction", __func__), - REJECT_INVALID, "bad-txns-nonfinal"); - } - } - // Enforce BIP 34 rule that the coinbase starts with serialized block height. // In Zcash this has been enforced since launch, except that the genesis // block didn't include the height in the coinbase (see Zcash protocol spec @@ -4210,6 +4197,32 @@ bool ContextualCheckBlock( } } + // If this is initial block download and "ibdskiptxverification" is set, we'll skip verifying the transactions + if (IsInitialBlockDownload(chainparams) && GetBoolArg("-ibdskiptxverification", false)) { + // If checkpoints are enabled, then skip verification only upto the last checkpoint. + if (fCheckpointsEnabled && nHeight < Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints())) { + return true; + } + } + + // Check that all transactions are finalized + BOOST_FOREACH(const CTransaction& tx, block.vtx) { + + // Check transaction contextually against consensus rules at block height + if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true)) { + return false; // Failure reason has been set in validation state object + } + + int nLockTimeFlags = 0; + int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) + ? pindexPrev->GetMedianTimePast() + : block.GetBlockTime(); + if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) { + return state.DoS(10, error("%s: contains a non-final transaction", __func__), + REJECT_INVALID, "bad-txns-nonfinal"); + } + } + return true; } From fd841a1b81bc4abbf6b659e77b2c4e82f35b2998 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 6 Oct 2020 17:23:20 -0600 Subject: [PATCH 02/10] Remove redundant CheckBlock calls. This change unifies handling of the -ibdskiptxverification flag and ensures consistent handling of checkpoint heights when evaluating whether transaction verification may be skipped. This change also removes two redundant CheckBlock calls. This change is consensus-critical and requires 3 reviews. --- src/checkpoints.cpp | 8 +++ src/checkpoints.h | 1 + src/gtest/test_checkblock.cpp | 16 ++--- src/main.cpp | 106 +++++++++++++++++++++------------- src/main.h | 30 ++++++---- src/test/checkblock_tests.cpp | 2 +- 6 files changed, 102 insertions(+), 61 deletions(-) diff --git a/src/checkpoints.cpp b/src/checkpoints.cpp index fbbb862cb..84c9c008b 100644 --- a/src/checkpoints.cpp +++ b/src/checkpoints.cpp @@ -78,4 +78,12 @@ namespace Checkpoints { return NULL; } + bool IsAncestorOfLastCheckpoint(const CCheckpointData& data, const CBlockIndex* pindex) + { + CBlockIndex *pindexLastCheckpoint = GetLastCheckpoint(data); + return pindexLastCheckpoint && pindexLastCheckpoint->GetAncestor(pindex->nHeight) == pindex; + } + + + } // namespace Checkpoints diff --git a/src/checkpoints.h b/src/checkpoints.h index b6501bc9a..b62b43280 100644 --- a/src/checkpoints.h +++ b/src/checkpoints.h @@ -27,6 +27,7 @@ CBlockIndex* GetLastCheckpoint(const CCheckpointData& data); double GuessVerificationProgress(const CCheckpointData& data, CBlockIndex* pindex, bool fSigchecks = true); +bool IsAncestorOfLastCheckpoint(const CCheckpointData& data, const CBlockIndex* pindex); } //namespace Checkpoints #endif // BITCOIN_CHECKPOINTS_H diff --git a/src/gtest/test_checkblock.cpp b/src/gtest/test_checkblock.cpp index bf726145f..b92f9ca76 100644 --- a/src/gtest/test_checkblock.cpp +++ b/src/gtest/test_checkblock.cpp @@ -32,7 +32,7 @@ TEST(CheckBlock, VersionTooLow) { MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "version-too-low", false)).Times(1); - EXPECT_FALSE(CheckBlock(block, state, Params(), verifier, false, false)); + EXPECT_FALSE(CheckBlock(block, state, Params(), verifier, false, false, true)); } @@ -65,7 +65,7 @@ TEST(CheckBlock, BlockSproutRejectsBadVersion) { auto verifier = ProofVerifier::Strict(); EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-version-too-low", false)).Times(1); - EXPECT_FALSE(CheckBlock(block, state, Params(), verifier, false, false)); + EXPECT_FALSE(CheckBlock(block, state, Params(), verifier, false, false, true)); } @@ -119,7 +119,7 @@ protected: // We now expect this to be a valid block. MockCValidationState state; - EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), &indexPrev)); + EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), &indexPrev, true)); } // Expects a height-1 block containing a given transaction to fail @@ -137,7 +137,7 @@ protected: // We now expect this to be an invalid block, for the given reason. MockCValidationState state; EXPECT_CALL(state, DoS(level, false, REJECT_INVALID, reason, false)).Times(1); - EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev)); + EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev, true)); } }; @@ -154,7 +154,7 @@ TEST_F(ContextualCheckBlockTest, BadCoinbaseHeight) { // Treating block as genesis should pass MockCValidationState state; - EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), NULL)); + EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), NULL, true)); // Give the transaction a Founder's Reward vout mtx.vout.push_back(CTxOut( @@ -168,20 +168,20 @@ TEST_F(ContextualCheckBlockTest, BadCoinbaseHeight) { CBlockIndex indexPrev {prev}; indexPrev.nHeight = 0; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-cb-height", false)).Times(1); - EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev)); + EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev, true)); // Setting to an incorrect height should fail mtx.vin[0].scriptSig = CScript() << 2 << OP_0; CTransaction tx3 {mtx}; block.vtx[0] = tx3; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-cb-height", false)).Times(1); - EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev)); + EXPECT_FALSE(ContextualCheckBlock(block, state, Params(), &indexPrev, true)); // After correcting the scriptSig, should pass mtx.vin[0].scriptSig = CScript() << 1 << OP_0; CTransaction tx4 {mtx}; block.vtx[0] = tx4; - EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), &indexPrev)); + EXPECT_TRUE(ContextualCheckBlock(block, state, Params(), &indexPrev, true)); } // TEST PLAN: first, check that each ruleset accepts its own transaction type. diff --git a/src/main.cpp b/src/main.cpp index 128e860a0..951749043 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2696,25 +2696,42 @@ static int64_t nTimeIndex = 0; static int64_t nTimeCallbacks = 0; static int64_t nTimeTotal = 0; +/** + * Determine whether to do transaction checks when validating blocks. + * Returns `false` (allowing transaction checks to be skipped) only if all + * of the following are true: + * - we're currently in initial block download + * - the `-ibdskiptxverification` flag is set + * - the block under inspection is an ancestor of the latest checkpoint. + */ +static bool IBDSkipTxVerification(const CChainParams& chainparams, const CBlockIndex* pindex) { + return IsInitialBlockDownload(chainparams) + && GetBoolArg("-ibdskiptxverification", false) + && fCheckpointsEnabled + && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex); +} + bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck) { AssertLockHeld(cs_main); bool fExpensiveChecks = true; - if (fCheckpointsEnabled) { - CBlockIndex *pindexLastCheckpoint = Checkpoints::GetLastCheckpoint(chainparams.Checkpoints()); - if (pindexLastCheckpoint && pindexLastCheckpoint->GetAncestor(pindex->nHeight) == pindex) { - // This block is an ancestor of a checkpoint: disable script checks - fExpensiveChecks = false; - } + + // If this block is an ancestor of a checkpoint, disable expensive checks + if (fCheckpointsEnabled && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex)) { + fExpensiveChecks = false; } - auto verifier = ProofVerifier::Strict(); - auto disabledVerifier = ProofVerifier::Disabled(); + // proof verification is expensive, disable if possible + auto verifier = fExpensiveChecks ? ProofVerifier::Strict() : ProofVerifier::Disabled(); + + // If in initial block download, and this block is an ancestor of a checkpoint, + // and -ibdskiptxverification is set, disable all transaction checks. + bool skipTxVerification = IBDSkipTxVerification(chainparams, pindex); // Check it again to verify JoinSplit proofs, and in case a previous version let a bad block in - if (!CheckBlock(block, state, chainparams, fExpensiveChecks ? verifier : disabledVerifier, !fJustCheck, !fJustCheck)) + if (!CheckBlock(block, state, chainparams, verifier, !fJustCheck, !fJustCheck, !skipTxVerification)) return false; // verify that the view's current state corresponds to the previous block @@ -4013,10 +4030,14 @@ bool CheckBlockHeader( return true; } -bool CheckBlock(const CBlock& block, CValidationState& state, +bool CheckBlock(const CBlock& block, + CValidationState& state, const CChainParams& chainparams, ProofVerifier& verifier, - bool fCheckPOW, bool fCheckMerkleRoot) + bool fCheckPOW, + bool fCheckMerkleRoot, + bool fCheckTransactions + ) { // These are checks that are independent of context. @@ -4059,10 +4080,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, return state.DoS(100, error("CheckBlock(): more than one coinbase"), REJECT_INVALID, "bad-cb-multiple"); - // If this is initial block download and "ibdskiptxverification" is set, we'll skip verifying the transactions - if (IsInitialBlockDownload(chainparams) && GetBoolArg("-ibdskiptxverification", false)) { - return true; - } + // skip all transaction checks if this flag is not set + if (!fCheckTransactions) return true; // Check transactions BOOST_FOREACH(const CTransaction& tx, block.vtx) @@ -4149,7 +4168,8 @@ bool ContextualCheckBlockHeader( bool ContextualCheckBlock( const CBlock& block, CValidationState& state, - const CChainParams& chainparams, CBlockIndex * const pindexPrev) + const CChainParams& chainparams, CBlockIndex * const pindexPrev, + bool fCheckTransactions) { const int nHeight = pindexPrev == NULL ? 0 : pindexPrev->nHeight + 1; const Consensus::Params& consensusParams = chainparams.GetConsensus(); @@ -4197,13 +4217,8 @@ bool ContextualCheckBlock( } } - // If this is initial block download and "ibdskiptxverification" is set, we'll skip verifying the transactions - if (IsInitialBlockDownload(chainparams) && GetBoolArg("-ibdskiptxverification", false)) { - // If checkpoints are enabled, then skip verification only upto the last checkpoint. - if (fCheckpointsEnabled && nHeight < Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints())) { - return true; - } - } + if (!fCheckTransactions) + return true; // Check that all transactions are finalized BOOST_FOREACH(const CTransaction& tx, block.vtx) { @@ -4306,9 +4321,14 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha if (fTooFarAhead) return true; // Block height is too high } - // See method docstring for why this is always disabled + // See method docstring for why this is always disabled. The only caller of + // AcceptBlock (ProcessNewBlock) will fully check proofs when it eventually + // delegates to `ConnectTip` and thereafter `ConnectBlock`. This + // arrangement is a bit fragile and should be reconsidered. auto verifier = ProofVerifier::Disabled(); - if ((!CheckBlock(block, state, chainparams, verifier)) || !ContextualCheckBlock(block, state, chainparams, pindex->pprev)) { + bool skipTxVerification = IBDSkipTxVerification(chainparams, pindex); + if ((!CheckBlock(block, state, chainparams, verifier, true, true, !skipTxVerification)) || + !ContextualCheckBlock(block, state, chainparams, pindex->pprev, !skipTxVerification)) { if (state.IsInvalid() && !state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); @@ -4359,17 +4379,9 @@ bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, c auto span = TracingSpan("info", "main", "ProcessNewBlock"); auto spanGuard = span.Enter(); - // Preliminary checks - auto verifier = ProofVerifier::Disabled(); - bool checked = CheckBlock(*pblock, state, chainparams, verifier); - { LOCK(cs_main); - bool fRequested = MarkBlockAsReceived(pblock->GetHash()); - fRequested |= fForceProcessing; - if (!checked) { - return error("%s: CheckBlock FAILED", __func__); - } + bool fRequested = MarkBlockAsReceived(pblock->GetHash()) | fForceProcessing; // Store to disk CBlockIndex *pindex = NULL; @@ -4388,6 +4400,9 @@ bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, c return true; } +/** + * This is only invoked by the miner. fCheckPOW is always false. + */ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot) { AssertLockHeld(cs_main); @@ -4397,15 +4412,15 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, CBlockIndex indexDummy(block); indexDummy.pprev = pindexPrev; indexDummy.nHeight = pindexPrev->nHeight + 1; - // JoinSplit proofs are verified in ConnectBlock - auto verifier = ProofVerifier::Disabled(); + auto verifier = ProofVerifier::Disabled(); // NOTE: CheckBlockHeader is called by CheckBlock if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev)) return false; - if (!CheckBlock(block, state, chainparams, verifier, fCheckPOW, fCheckMerkleRoot)) + // The following may be duplicative of the `CheckBlock` call within `ConnectBlock` + if (!CheckBlock(block, state, chainparams, verifier, fCheckPOW, fCheckMerkleRoot, true)) return false; - if (!ContextualCheckBlock(block, state, chainparams, pindexPrev)) + if (!ContextualCheckBlock(block, state, chainparams, pindexPrev, true)) return false; if (!ConnectBlock(block, state, &indexDummy, viewNew, chainparams, true)) return false; @@ -4797,21 +4812,28 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, CBlockIndex* pindexFailure = NULL; int nGoodTransactions = 0; CValidationState state; - // No need to verify JoinSplits twice - auto verifier = ProofVerifier::Disabled(); + + // Flags used to permit skipping checks for efficiency + auto verifier = ProofVerifier::Disabled(); // No need to verify JoinSplits twice + bool skipTxVerification = false; + for (CBlockIndex* pindex = chainActive.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) { boost::this_thread::interruption_point(); uiInterface.ShowProgress(_("Verifying blocks..."), std::max(1, std::min(99, (int)(((double)(chainActive.Height() - pindex->nHeight)) / (double)nCheckDepth * (nCheckLevel >= 4 ? 50 : 100))))); if (pindex->nHeight < chainActive.Height()-nCheckDepth) break; + CBlock block; // check level 0: read from disk if (!ReadBlockFromDisk(block, pindex, chainparams.GetConsensus())) return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + // check level 1: verify block validity - if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams, verifier)) + skipTxVerification = IBDSkipTxVerification(chainparams, pindex); + if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams, verifier, true, true, !skipTxVerification)) return error("VerifyDB(): *** found bad block at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { CBlockUndo undo; @@ -4821,6 +4843,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, return error("VerifyDB(): *** found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); } } + // check level 3: check for inconsistencies during memory-only disconnect of tip blocks if (nCheckLevel >= 3 && pindex == pindexState && (coins.DynamicMemoryUsage() + pcoinsTip->DynamicMemoryUsage()) <= nCoinCacheUsage) { // insightexplorer: do not update indices (false) @@ -4836,6 +4859,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, nGoodTransactions += block.vtx.size(); } } + if (ShutdownRequested()) return true; } diff --git a/src/main.h b/src/main.h index e077dffcb..7ec6608e0 100644 --- a/src/main.h +++ b/src/main.h @@ -210,11 +210,11 @@ void RegisterNodeSignals(CNodeSignals& nodeSignals); /** Unregister a network node */ void UnregisterNodeSignals(CNodeSignals& nodeSignals); -/** +/** * Process an incoming block. This only returns after the best known valid * block is made active. Note that it does not, however, guarantee that the * specific block passed to it has been checked for validity! - * + * * @param[out] state This may be set to an Error state if any error occurred processing it, including during validation/connection/etc of otherwise unrelated blocks during reorganisation; or it may be set to an Invalid state if pblock is itself invalid (but this is not guaranteed even when the block is checked). If you want to *possibly* get feedback on whether pblock is valid, you must also install a CValidationInterface (see validationinterface.h) - this will have its BlockChecked method called whenever *any* block completes validation. * @param[in] pfrom The node which we are receiving the block from; it is added to mapBlockSource and may be penalised if the block is invalid. * @param[in] pblock The block we want to process. @@ -311,7 +311,7 @@ struct CNodeStateStats { CAmount GetMinRelayFee(const CTransaction& tx, unsigned int nBytes, bool fAllowFree); -/** +/** * Count ECDSA signature operations the old-fashioned (pre-0.6) way * @return number of sigops this transaction's outputs will produce when spent * @see CTransaction::FetchInputs @@ -320,7 +320,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx); /** * Count ECDSA signature operations in pay-to-script-hash inputs. - * + * * @param[in] mapInputs Map of previous transactions that have outputs we're spending * @return maximum number of sigops required to validate this transaction's inputs * @see CTransaction::FetchInputs @@ -390,9 +390,9 @@ bool IsExpiringSoonTx(const CTransaction &tx, int nNextBlockHeight); */ bool CheckFinalTx(const CTransaction &tx, int flags = -1); -/** +/** * Closure representing one script verification - * Note that this stores references to the spending transaction + * Note that this stores references to the spending transaction */ class CScriptCheck { @@ -447,13 +447,18 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus /** Functions for validating blocks and updating the block tree */ /** Context-independent validity checks */ + bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, bool fCheckPOW = true); + bool CheckBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, ProofVerifier& verifier, - bool fCheckPOW = true, bool fCheckMerkleRoot = true); + bool fCheckPOW, + bool fCheckMerkleRoot, + bool fCheckTransactions + ); /** Context-dependent validity checks. * By "context", we mean only the previous block headers, but not the UTXO @@ -461,7 +466,10 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindexPrev); bool ContextualCheckBlock(const CBlock& block, CValidationState& state, - const CChainParams& chainparams, CBlockIndex *pindexPrev); + const CChainParams& chainparams, + CBlockIndex *pindexPrev, + bool fCheckTransactions + ); /** Apply the effects of this block (with given index) on the UTXO set represented by coins. * Validity checks that depend on the UTXO set are also done; ConnectBlock() @@ -469,9 +477,9 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins, const CChainParams& chainparams, bool fJustCheck = false); -/** +/** * Check a block is completely valid from start to finish (only works on top - * of our current best block, with cs_main held) + * of our current best block, with cs_main held) */ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true); @@ -521,7 +529,7 @@ int GetSpendHeight(const CCoinsViewCache& inputs); uint64_t CalculateCurrentUsage(); -/** +/** * Return a CMutableTransaction with contextual default values based on set of consensus rules at nHeight. The expiryDelta will * either be based on the command-line argument '-txexpirydelta' or derived from consensusParams. */ diff --git a/src/test/checkblock_tests.cpp b/src/test/checkblock_tests.cpp index 4df9ca027..494ce4255 100644 --- a/src/test/checkblock_tests.cpp +++ b/src/test/checkblock_tests.cpp @@ -59,7 +59,7 @@ BOOST_AUTO_TEST_CASE(May15) // After May 15'th, big blocks are OK: forkingBlock.nTime = tMay15; // Invalidates PoW auto verifier = ProofVerifier::Strict(); - BOOST_CHECK(CheckBlock(forkingBlock, state, Params(), verifier, false, false)); + BOOST_CHECK(CheckBlock(forkingBlock, state, Params(), verifier, false, false, true)); } SetMockTime(0); From 91172401ac49b84fd8e6d1317f9b35d592e51bbb Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 8 Oct 2020 16:34:00 -0600 Subject: [PATCH 03/10] Reduce diff complexity. --- src/main.cpp | 54 +++++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 951749043..c1b0cc992 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2711,6 +2711,11 @@ static bool IBDSkipTxVerification(const CChainParams& chainparams, const CBlockI && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex); } +/** + * - Validate the block with CheckBlock + * - Check the the CCoinsViewCache's best block is the parent of the block. + * - + */ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck) { @@ -4174,6 +4179,26 @@ bool ContextualCheckBlock( const int nHeight = pindexPrev == NULL ? 0 : pindexPrev->nHeight + 1; const Consensus::Params& consensusParams = chainparams.GetConsensus(); + if (fCheckTransactions) { + // Check that all transactions are finalized + BOOST_FOREACH(const CTransaction& tx, block.vtx) { + + // Check transaction contextually against consensus rules at block height + if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true)) { + return false; // Failure reason has been set in validation state object + } + + int nLockTimeFlags = 0; + int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) + ? pindexPrev->GetMedianTimePast() + : block.GetBlockTime(); + if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) { + return state.DoS(10, error("%s: contains a non-final transaction", __func__), + REJECT_INVALID, "bad-txns-nonfinal"); + } + } + } + // Enforce BIP 34 rule that the coinbase starts with serialized block height. // In Zcash this has been enforced since launch, except that the genesis // block didn't include the height in the coinbase (see Zcash protocol spec @@ -4217,27 +4242,6 @@ bool ContextualCheckBlock( } } - if (!fCheckTransactions) - return true; - - // Check that all transactions are finalized - BOOST_FOREACH(const CTransaction& tx, block.vtx) { - - // Check transaction contextually against consensus rules at block height - if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true)) { - return false; // Failure reason has been set in validation state object - } - - int nLockTimeFlags = 0; - int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) - ? pindexPrev->GetMedianTimePast() - : block.GetBlockTime(); - if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) { - return state.DoS(10, error("%s: contains a non-final transaction", __func__), - REJECT_INVALID, "bad-txns-nonfinal"); - } - } - return true; } @@ -4286,9 +4290,11 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state /** * Store block on disk. - * JoinSplit proofs are never verified, because: - * - AcceptBlock doesn't perform script checks either. - * - The only caller of AcceptBlock verifies JoinSplit proofs elsewhere. + * + * JoinSplit proofs are not verified here; the only caller of AcceptBlock + * (ProcessNewBlock) invokes ActivateBestChain, which ultimately calls + * ConnectBlock in a manner that can verify the proofs + * * If dbp is non-NULL, the file is known to already reside on disk */ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, CDiskBlockPos* dbp) From 293af68ebbba9d120af8af17b676994b683254c6 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Oct 2020 08:54:16 -0600 Subject: [PATCH 04/10] Apply style suggestions from code review Co-authored-by: str4d --- src/main.cpp | 3 +-- src/main.h | 6 ++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index c1b0cc992..648537a30 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4041,8 +4041,7 @@ bool CheckBlock(const CBlock& block, ProofVerifier& verifier, bool fCheckPOW, bool fCheckMerkleRoot, - bool fCheckTransactions - ) + bool fCheckTransactions) { // These are checks that are independent of context. diff --git a/src/main.h b/src/main.h index 7ec6608e0..db57e4e4d 100644 --- a/src/main.h +++ b/src/main.h @@ -457,8 +457,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, ProofVerifier& verifier, bool fCheckPOW, bool fCheckMerkleRoot, - bool fCheckTransactions - ); + bool fCheckTransactions); /** Context-dependent validity checks. * By "context", we mean only the previous block headers, but not the UTXO @@ -468,8 +467,7 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta bool ContextualCheckBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindexPrev, - bool fCheckTransactions - ); + bool fCheckTransactions); /** Apply the effects of this block (with given index) on the UTXO set represented by coins. * Validity checks that depend on the UTXO set are also done; ConnectBlock() From f0c24bd9f9b9bb6638a06c8141cebc69c8069f42 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Oct 2020 09:45:31 -0600 Subject: [PATCH 05/10] -ibdskiptxverification must imply -checkpoints --- src/init.cpp | 13 +++++++------ src/main.cpp | 6 ++++-- src/main.h | 2 ++ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 65a646c11..5aa8c045f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -342,7 +342,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-debuglogfile=", strprintf(_("Specify location of debug log file: this can be an absolute path or a path relative to the data directory (default: %s)"), DEFAULT_DEBUGLOGFILE)); strUsage += HelpMessageOpt("-exportdir=", _("Specify directory to be used when exporting data")); - strUsage += HelpMessageOpt("-ibdskiptxverification", _("Skip Tx verification during initial block download")); + strUsage += HelpMessageOpt("-ibdskiptxverification", _("Skip transaction verification during initial block download up to the last checkpoint height. Implies -checkpoints.")); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); strUsage += HelpMessageOpt("-par=", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), @@ -420,13 +420,13 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-nuparams=hexBranchId:activationHeight", "Use given activation height for specified network upgrade (regtest-only)"); strUsage += HelpMessageOpt("-nurejectoldversions", strprintf("Reject peers that don't know about the current epoch (regtest-only) (default: %u)", DEFAULT_NU_REJECT_OLD_VERSIONS)); strUsage += HelpMessageOpt( - "-fundingstream=streamId:startHeight:endHeight:comma_delimited_addresses", + "-fundingstream=streamId:startHeight:endHeight:comma_delimited_addresses", "Use given addresses for block subsidy share paid to the funding stream with id (regtest-only)"); } string debugCategories = "addrman, alert, bench, coindb, db, estimatefee, http, libevent, lock, mempool, net, partitioncheck, pow, proxy, prune, " "rand, reindex, rpc, selectcoins, tor, zmq, zrpc, zrpcunsafe (implies zrpc)"; // Don't translate these strUsage += HelpMessageOpt("-debug=", strprintf(_("Output debugging information (default: %u, supplying is optional)"), 0) + ". " + - _("If is not supplied or if = 1, output all debugging information.") + " " + _(" can be:") + " " + debugCategories + ". " + + _("If is not supplied or if = 1, output all debugging information.") + " " + _(" can be:") + " " + debugCategories + ". " + _("For multiple specific categories use -debug= multiple times.")); strUsage += HelpMessageOpt("-experimentalfeatures", _("Enable use of experimental features")); strUsage += HelpMessageOpt("-help-debug", _("Show all debugging options (usage: --help -help-debug)")); @@ -964,7 +964,8 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) mempool.SetMempoolCostLimit(mempoolTotalCostLimit, mempoolEvictionMemorySeconds); fCheckBlockIndex = GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks()); - fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED); + fIBDSkipTxVerification = GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION); + fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED) || fIBDSkipTxVerification; // -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency nScriptCheckThreads = GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS); @@ -1106,8 +1107,8 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) return InitError("Funding stream parameters malformed, expecting streamId:startHeight:endHeight:comma_delimited_addresses"); } int nFundingStreamId; - if (!ParseInt32(vStreamParams[0], &nFundingStreamId) || - nFundingStreamId < Consensus::FIRST_FUNDING_STREAM || + if (!ParseInt32(vStreamParams[0], &nFundingStreamId) || + nFundingStreamId < Consensus::FIRST_FUNDING_STREAM || nFundingStreamId >= Consensus::MAX_FUNDING_STREAMS) { return InitError(strprintf("Invalid streamId (%s)", vStreamParams[0])); } diff --git a/src/main.cpp b/src/main.cpp index 648537a30..af63768df 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -80,6 +80,7 @@ bool fPruneMode = false; bool fIsBareMultisigStd = DEFAULT_PERMIT_BAREMULTISIG; bool fCheckBlockIndex = false; bool fCheckpointsEnabled = DEFAULT_CHECKPOINTS_ENABLED; +bool fIBDSkipTxVerification = DEFAULT_IBD_SKIP_TX_VERIFICATION; bool fCoinbaseEnforcedShieldingEnabled = true; size_t nCoinCacheUsage = 5000 * 300; uint64_t nPruneTarget = 0; @@ -2706,7 +2707,7 @@ static int64_t nTimeTotal = 0; */ static bool IBDSkipTxVerification(const CChainParams& chainparams, const CBlockIndex* pindex) { return IsInitialBlockDownload(chainparams) - && GetBoolArg("-ibdskiptxverification", false) + && fIBDSkipTxVerification && fCheckpointsEnabled && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex); } @@ -4417,8 +4418,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, CBlockIndex indexDummy(block); indexDummy.pprev = pindexPrev; indexDummy.nHeight = pindexPrev->nHeight + 1; - + // JoinSplit proofs are verified in ConnectBlock auto verifier = ProofVerifier::Disabled(); + // NOTE: CheckBlockHeader is called by CheckBlock if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev)) return false; diff --git a/src/main.h b/src/main.h index db57e4e4d..fe38bf19a 100644 --- a/src/main.h +++ b/src/main.h @@ -113,6 +113,7 @@ static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60; /** Default for -permitbaremultisig */ static const bool DEFAULT_PERMIT_BAREMULTISIG = true; static const bool DEFAULT_CHECKPOINTS_ENABLED = true; +static const bool DEFAULT_IBD_SKIP_TX_VERIFICATION = false; static const bool DEFAULT_TXINDEX = false; static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100; @@ -164,6 +165,7 @@ extern bool fTimestampIndex; extern bool fIsBareMultisigStd; extern bool fCheckBlockIndex; extern bool fCheckpointsEnabled; +extern bool fIBDSkipTxVerification; // TODO: remove this flag by structuring our code such that // it is unneeded for testing extern bool fCoinbaseEnforcedShieldingEnabled; From 2c051acae5c2613341283e358f64a77b3f647483 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Oct 2020 11:44:15 -0600 Subject: [PATCH 06/10] Apply suggestions from code review Co-authored-by: str4d --- src/init.cpp | 4 ++-- src/main.cpp | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 5aa8c045f..b0da47077 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -342,7 +342,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-debuglogfile=", strprintf(_("Specify location of debug log file: this can be an absolute path or a path relative to the data directory (default: %s)"), DEFAULT_DEBUGLOGFILE)); strUsage += HelpMessageOpt("-exportdir=", _("Specify directory to be used when exporting data")); - strUsage += HelpMessageOpt("-ibdskiptxverification", _("Skip transaction verification during initial block download up to the last checkpoint height. Implies -checkpoints.")); + strUsage += HelpMessageOpt("-ibdskiptxverification", strprintf(_("Skip transaction verification during initial block download up to the last checkpoint height. Incompatible with -nocheckpoints. (default = %u)"), DEFAULT_IBD_SKIP_TX_VERIFICATION)); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); strUsage += HelpMessageOpt("-par=", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), @@ -965,7 +965,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) fCheckBlockIndex = GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks()); fIBDSkipTxVerification = GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION); - fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED) || fIBDSkipTxVerification; + fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED); // -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency nScriptCheckThreads = GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS); diff --git a/src/main.cpp b/src/main.cpp index af63768df..3d55c1ba6 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2712,11 +2712,6 @@ static bool IBDSkipTxVerification(const CChainParams& chainparams, const CBlockI && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex); } -/** - * - Validate the block with CheckBlock - * - Check the the CCoinsViewCache's best block is the parent of the block. - * - - */ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck) { From 8a2aaebd7efdd7dd224e37d452c735620e9d3c2d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Oct 2020 11:48:23 -0600 Subject: [PATCH 07/10] Ensure conflicting flags are reported as an error. --- src/init.cpp | 4 ++++ src/main.cpp | 10 +++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index b0da47077..763b274c7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -967,6 +967,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) fIBDSkipTxVerification = GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION); fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED); + if (fIBDSkipTxVerification && !fCheckpointsEnabled) { + return InitError(_("-ibdskiptxverification requires checkpoints to be enabled; it is incompatible with -nocheckpoints")); + } + // -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency nScriptCheckThreads = GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS); if (nScriptCheckThreads <= 0) diff --git a/src/main.cpp b/src/main.cpp index 3d55c1ba6..afa8b7632 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4285,12 +4285,11 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state /** * Store block on disk. + * If dbp is non-NULL, the file is known to already reside on disk. * * JoinSplit proofs are not verified here; the only caller of AcceptBlock - * (ProcessNewBlock) invokes ActivateBestChain, which ultimately calls + * (ProcessNewBlock) later invokes ActivateBestChain, which ultimately calls * ConnectBlock in a manner that can verify the proofs - * - * If dbp is non-NULL, the file is known to already reside on disk */ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, CDiskBlockPos* dbp) { @@ -4322,10 +4321,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha if (fTooFarAhead) return true; // Block height is too high } - // See method docstring for why this is always disabled. The only caller of - // AcceptBlock (ProcessNewBlock) will fully check proofs when it eventually - // delegates to `ConnectTip` and thereafter `ConnectBlock`. This - // arrangement is a bit fragile and should be reconsidered. + // See method docstring for why this is always disabled. auto verifier = ProofVerifier::Disabled(); bool skipTxVerification = IBDSkipTxVerification(chainparams, pindex); if ((!CheckBlock(block, state, chainparams, verifier, true, true, !skipTxVerification)) || From 88a7dd59c0400aeb385a64309ae45b34a031965f Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 9 Oct 2020 11:54:06 -0600 Subject: [PATCH 08/10] Reject incompatible flags in "Step 2" --- src/init.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 763b274c7..9127045b6 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -923,6 +923,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) #endif } + // ensure that the user has not disabled checkpoints when requesting to + // skip transaction verification in initial block download. + if (GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION)) { + if (!GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED)) { + return InitError(_("-ibdskiptxverification requires checkpoints to be enabled; it is incompatible with -nocheckpoints")); + } + } + // ********************************************************* Step 3: parameter-to-internal-flags fDebug = !mapMultiArgs["-debug"].empty(); @@ -967,10 +975,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) fIBDSkipTxVerification = GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION); fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED); - if (fIBDSkipTxVerification && !fCheckpointsEnabled) { - return InitError(_("-ibdskiptxverification requires checkpoints to be enabled; it is incompatible with -nocheckpoints")); - } - // -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency nScriptCheckThreads = GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS); if (nScriptCheckThreads <= 0) From 95e38235c758b2498fa167627e8975a1df4a4eeb Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 12 Oct 2020 15:38:07 -0600 Subject: [PATCH 09/10] Rename IBDSkipTxVerification back to ShouldCheckTransaction Co-authored-by: Daira Hopwood --- src/init.cpp | 2 +- src/main.cpp | 28 ++++++++++++++-------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9127045b6..8287be36e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -927,7 +927,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) // skip transaction verification in initial block download. if (GetBoolArg("-ibdskiptxverification", DEFAULT_IBD_SKIP_TX_VERIFICATION)) { if (!GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED)) { - return InitError(_("-ibdskiptxverification requires checkpoints to be enabled; it is incompatible with -nocheckpoints")); + return InitError(_("-ibdskiptxverification requires checkpoints to be enabled; it is incompatible with flags that disable checkpoints")); } } diff --git a/src/main.cpp b/src/main.cpp index afa8b7632..07d5fc0cf 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2698,18 +2698,18 @@ static int64_t nTimeCallbacks = 0; static int64_t nTimeTotal = 0; /** - * Determine whether to do transaction checks when validating blocks. + * Determine whether to do transaction checks when verifying blocks. * Returns `false` (allowing transaction checks to be skipped) only if all * of the following are true: * - we're currently in initial block download * - the `-ibdskiptxverification` flag is set * - the block under inspection is an ancestor of the latest checkpoint. */ -static bool IBDSkipTxVerification(const CChainParams& chainparams, const CBlockIndex* pindex) { - return IsInitialBlockDownload(chainparams) - && fIBDSkipTxVerification - && fCheckpointsEnabled - && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex); +static bool ShouldCheckTransactions(const CChainParams& chainparams, const CBlockIndex* pindex) { + return !(IsInitialBlockDownload(chainparams) + && fIBDSkipTxVerification + && fCheckpointsEnabled + && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex)); } bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, @@ -2729,10 +2729,10 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // If in initial block download, and this block is an ancestor of a checkpoint, // and -ibdskiptxverification is set, disable all transaction checks. - bool skipTxVerification = IBDSkipTxVerification(chainparams, pindex); + bool fCheckTransactions = ShouldCheckTransactions(chainparams, pindex); // Check it again to verify JoinSplit proofs, and in case a previous version let a bad block in - if (!CheckBlock(block, state, chainparams, verifier, !fJustCheck, !fJustCheck, !skipTxVerification)) + if (!CheckBlock(block, state, chainparams, verifier, !fJustCheck, !fJustCheck, fCheckTransactions)) return false; // verify that the view's current state corresponds to the previous block @@ -4323,9 +4323,9 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha // See method docstring for why this is always disabled. auto verifier = ProofVerifier::Disabled(); - bool skipTxVerification = IBDSkipTxVerification(chainparams, pindex); - if ((!CheckBlock(block, state, chainparams, verifier, true, true, !skipTxVerification)) || - !ContextualCheckBlock(block, state, chainparams, pindex->pprev, !skipTxVerification)) { + bool fCheckTransactions = ShouldCheckTransactions(chainparams, pindex); + if ((!CheckBlock(block, state, chainparams, verifier, true, true, fCheckTransactions)) || + !ContextualCheckBlock(block, state, chainparams, pindex->pprev, fCheckTransactions)) { if (state.IsInvalid() && !state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); @@ -4813,7 +4813,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, // Flags used to permit skipping checks for efficiency auto verifier = ProofVerifier::Disabled(); // No need to verify JoinSplits twice - bool skipTxVerification = false; + bool fCheckTransactions = true; for (CBlockIndex* pindex = chainActive.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) { @@ -4828,8 +4828,8 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); // check level 1: verify block validity - skipTxVerification = IBDSkipTxVerification(chainparams, pindex); - if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams, verifier, true, true, !skipTxVerification)) + fCheckTransactions = ShouldCheckTransactions(chainparams, pindex); + if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams, verifier, true, true, fCheckTransactions)) return error("VerifyDB(): *** found bad block at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); // check level 2: verify undo validity From a75c613ade7a70b352d07fa8363f735a230bcf1d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Oct 2020 07:18:51 -0600 Subject: [PATCH 10/10] Fix command-line help for -ibdskiptxverification --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 8287be36e..3815ba398 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -342,7 +342,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-debuglogfile=", strprintf(_("Specify location of debug log file: this can be an absolute path or a path relative to the data directory (default: %s)"), DEFAULT_DEBUGLOGFILE)); strUsage += HelpMessageOpt("-exportdir=", _("Specify directory to be used when exporting data")); - strUsage += HelpMessageOpt("-ibdskiptxverification", strprintf(_("Skip transaction verification during initial block download up to the last checkpoint height. Incompatible with -nocheckpoints. (default = %u)"), DEFAULT_IBD_SKIP_TX_VERIFICATION)); + strUsage += HelpMessageOpt("-ibdskiptxverification", strprintf(_("Skip transaction verification during initial block download up to the last checkpoint height. Incompatible with flags that disable checkpoints. (default = %u)"), DEFAULT_IBD_SKIP_TX_VERIFICATION)); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); strUsage += HelpMessageOpt("-par=", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"),