zecfoundation
/
zcashd
mirror of https://github.com/ZcashFoundation/zcashd.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Kris Nuttycombe 2020-10-06 17:23:20 -06:00
parent 0b4395d275
commit fd841a1b81
6 changed files with 102 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/checkpoints.cpp
View File

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

1
src/checkpoints.h
View File

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

16
src/gtest/test_checkblock.cpp
View File

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

106
src/main.cpp
View File

@ -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;
}

30
src/main.h
View File

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

2
src/test/checkblock_tests.cpp
View File

@ -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);