From 0ec57cfcba0440585161dc05cc1f2417933fe317 Mon Sep 17 00:00:00 2001 From: p-j01 Date: Fri, 11 Jun 2021 22:58:18 +0000 Subject: [PATCH 1/2] fix: A newly appended block header should not build on an invalid chain --- src/validation.cpp | 74 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c81ab3c20..f05b304d2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -160,6 +160,26 @@ namespace { /** chainwork for the last block that preciousblock has been applied to. */ arith_uint256 nLastPreciousChainwork = 0; + /** In order to efficiently track invalidity of headers, we keep the set of + * blocks which we tried to connect and found to be invalid here (ie which + * were set to BLOCK_FAILED_VALID since the last restart). We can then + * walk this set and check if a new header is a descendant of something in + * this set, preventing us from having to walk mapBlockIndex when we try + * to connect a bad block and fail. + * + * While this is more complicated than marking everything which descends + * from an invalid block as invalid at the time we discover it to be + * invalid, doing so would require walking all of mapBlockIndex to find all + * descendants. Since this case should be very rare, keeping track of all + * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as + * well. + * + * Because we already walk mapBlockIndex in height-order at startup, we go + * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, + * instead of putting things in this set. + */ + std::set gFailedBlocks; + /** Dirty block index entries. */ std::set setDirtyBlockIndex; @@ -1338,6 +1358,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) void static InvalidBlockFound(CBlockIndex *pindex, const CValidationState &state) { if (!state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; + gFailedBlocks.insert(pindex); setDirtyBlockIndex.insert(pindex); setBlockIndexCandidates.erase(pindex); InvalidChainFound(pindex); @@ -2621,16 +2642,17 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C { AssertLockHeld(cs_main); - // Mark the block itself as invalid. - pindex->nStatus |= BLOCK_FAILED_VALID; - setDirtyBlockIndex.insert(pindex); - setBlockIndexCandidates.erase(pindex); + // We first disconnect backwards and then mark the blocks as invalid. + // This prevents a case where pruned nodes may fail to invalidateblock + // and be left unable to start as they have no tip candidates (as there + // are no blocks that meet the "have data and are not invalid per + // nStatus" criteria for inclusion in setBlockIndexCandidates). + + bool pindex_was_in_chain = false; + CBlockIndex *invalid_walk_tip = chainActive.Tip(); while (chainActive.Contains(pindex)) { - CBlockIndex *pindexWalk = chainActive.Tip(); - pindexWalk->nStatus |= BLOCK_FAILED_CHILD; - setDirtyBlockIndex.insert(pindexWalk); - setBlockIndexCandidates.erase(pindexWalk); + pindex_was_in_chain = true; // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(state, chainparams)) { @@ -2639,6 +2661,21 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C } } + // Now mark the blocks we just disconnected as descendants invalid + // (note this may not be all descendants). + while (pindex_was_in_chain && invalid_walk_tip != pindex) { + invalid_walk_tip->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalid_walk_tip); + setBlockIndexCandidates.erase(invalid_walk_tip); + invalid_walk_tip = invalid_walk_tip->pprev; + } + + // Mark the block itself as invalid. + pindex->nStatus |= BLOCK_FAILED_VALID; + setDirtyBlockIndex.insert(pindex); + setBlockIndexCandidates.erase(pindex); + gFailedBlocks.insert(pindex); + LimitMempoolSize(mempool, GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60); // The resulting new best tip may not be in setBlockIndexCandidates anymore, so @@ -2647,6 +2684,7 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C while (it != mapBlockIndex.end()) { if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) { setBlockIndexCandidates.insert(it->second); + gFailedBlocks.erase(it->second); } it++; } @@ -3180,6 +3218,21 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state if (!ContextualCheckBlockHeader(block, state, pindexPrev, GetAdjustedTime())) return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); + + if (!pindexPrev->IsValid(BLOCK_VALID_SCRIPTS)) { + for (const CBlockIndex* failedit : gFailedBlocks) { + if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { + assert(failedit->nStatus & BLOCK_FAILED_VALID); + CBlockIndex* invalid_walk = pindexPrev; + while (invalid_walk != failedit) { + invalid_walk->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalid_walk); + invalid_walk = invalid_walk->pprev; + } + return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + } + } + } } if (pindex == NULL) pindex = AddToBlockIndex(block); @@ -3600,6 +3653,10 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) } else { pindex->nChainTx = pindex->nTx; } + if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { + pindex->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(pindex); + } } if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == NULL)) setBlockIndexCandidates.insert(pindex); @@ -3879,6 +3936,7 @@ void UnloadBlockIndex() nLastBlockFile = 0; nBlockSequenceId = 1; setDirtyBlockIndex.clear(); + gFailedBlocks.clear(); setDirtyFileInfo.clear(); versionbitscache.Clear(); for (int b = 0; b < VERSIONBITS_NUM_BITS; b++) { From 8ee0b0259e8f61ec26ace1e625a7bd1bf0bb3aa9 Mon Sep 17 00:00:00 2001 From: Escanor Liones Date: Mon, 16 Aug 2021 09:29:11 -0400 Subject: [PATCH 2/2] [ #2297 ] Implemented suggestions: camel casing as suggested and printing debug messages for invalidated blocks. --- src/validation.cpp | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index f05b304d2..8fc4ef6c7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2648,11 +2648,11 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C // are no blocks that meet the "have data and are not invalid per // nStatus" criteria for inclusion in setBlockIndexCandidates). - bool pindex_was_in_chain = false; - CBlockIndex *invalid_walk_tip = chainActive.Tip(); + bool findexWasInChain = false; + CBlockIndex *invalidWalkTip = chainActive.Tip(); while (chainActive.Contains(pindex)) { - pindex_was_in_chain = true; + findexWasInChain = true; // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(state, chainparams)) { @@ -2661,13 +2661,19 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C } } + // Debug print the invalid parent's hash, later list the child hashes. + LogPrintf("Invalid Block %s.", pindex->GetBlockHash().ToString() ); + // Now mark the blocks we just disconnected as descendants invalid - // (note this may not be all descendants). - while (pindex_was_in_chain && invalid_walk_tip != pindex) { - invalid_walk_tip->nStatus |= BLOCK_FAILED_CHILD; - setDirtyBlockIndex.insert(invalid_walk_tip); - setBlockIndexCandidates.erase(invalid_walk_tip); - invalid_walk_tip = invalid_walk_tip->pprev; + // Note: this may not be all descendants, e.g. the disconnected block + // may be the root of competing block chains that have not settled into + // one single block chain yet. + while (findexWasInChain && invalidWalkTip != pindex) { + LogPrintf(" Invalid Child Block %s.", invalidWalkTip->GetBlockHash().ToString() ); + invalidWalkTip->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalidWalkTip); + setBlockIndexCandidates.erase(invalidWalkTip); + invalidWalkTip = invalidWalkTip->pprev; } // Mark the block itself as invalid. @@ -3223,12 +3229,14 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state for (const CBlockIndex* failedit : gFailedBlocks) { if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { assert(failedit->nStatus & BLOCK_FAILED_VALID); - CBlockIndex* invalid_walk = pindexPrev; - while (invalid_walk != failedit) { - invalid_walk->nStatus |= BLOCK_FAILED_CHILD; - setDirtyBlockIndex.insert(invalid_walk); - invalid_walk = invalid_walk->pprev; + CBlockIndex* invalidWalk = pindexPrev; + while (invalidWalk != failedit) { + invalidWalk->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalidWalk); + invalidWalk = invalidWalk->pprev; } + + LogPrintf( "AcceptBlockHeader(): Invalid block %s, rejected child hash %s.", invalidWalk->GetBlockHash().ToString(), hash.ToString() ); return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); } } @@ -3654,6 +3662,7 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) pindex->nChainTx = pindex->nTx; } if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { + LogPrintf("Invalid Block %s.", pindex->GetBlockHash().ToString() ); pindex->nStatus |= BLOCK_FAILED_CHILD; setDirtyBlockIndex.insert(pindex); }