diff --git a/src/chain.h b/src/chain.h index fa165a4aa73..7faeb25088c 100644 --- a/src/chain.h +++ b/src/chain.h @@ -98,16 +98,20 @@ enum BlockStatus : uint32_t { /** * Only first tx is coinbase, 2 <= coinbase input script length <= 100, transactions valid, no duplicate txids, - * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS. When all - * parent blocks also have TRANSACTIONS, CBlockIndex::nChainTx will be set. + * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS. + * + * If a block's validity is at least VALID_TRANSACTIONS, CBlockIndex::nTx will be set. If a block and all previous + * blocks back to the genesis block or an assumeutxo snapshot block are at least VALID_TRANSACTIONS, + * CBlockIndex::nChainTx will be set. */ BLOCK_VALID_TRANSACTIONS = 3, //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends, BIP30. - //! Implies all parents are either at least VALID_CHAIN, or are ASSUMED_VALID + //! Implies all previous blocks back to the genesis block or an assumeutxo snapshot block are at least VALID_CHAIN. BLOCK_VALID_CHAIN = 4, - //! Scripts & signatures ok. Implies all parents are either at least VALID_SCRIPTS, or are ASSUMED_VALID. + //! Scripts & signatures ok. Implies all previous blocks back to the genesis block or an assumeutxo snapshot block + //! are at least VALID_SCRIPTS. BLOCK_VALID_SCRIPTS = 5, //! All validity bits. @@ -173,21 +177,16 @@ public: //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block arith_uint256 nChainWork{}; - //! Number of transactions in this block. + //! Number of transactions in this block. This will be nonzero if the block + //! reached the VALID_TRANSACTIONS level, and zero otherwise. //! Note: in a potential headers-first mode, this number cannot be relied upon - //! Note: this value is faked during UTXO snapshot load to ensure that - //! LoadBlockIndex() will load index entries for blocks that we lack data for. - //! @sa ActivateSnapshot unsigned int nTx{0}; //! (memory only) Number of transactions in the chain up to and including this block. - //! This value will be non-zero only if and only if transactions for this block and all its parents are available. + //! This value will be non-zero if this block and all previous blocks back + //! to the genesis block or an assumeutxo snapshot block have reached the + //! VALID_TRANSACTIONS level. //! Change to 64-bit type before 2024 (assuming worst case of 60 byte transactions). - //! - //! Note: this value is faked during use of a UTXO snapshot because we don't - //! have the underlying block data available during snapshot load. - //! @sa AssumeutxoData - //! @sa ActivateSnapshot unsigned int nChainTx{0}; //! Verification status of this block. See enum BlockStatus @@ -262,15 +261,14 @@ public: } /** - * Check whether this block's and all previous blocks' transactions have been - * downloaded (and stored to disk) at some point. + * Check whether this block and all previous blocks back to the genesis block or an assumeutxo snapshot block have + * reached VALID_TRANSACTIONS and had transactions downloaded (and stored to disk) at some point. * * Does not imply the transactions are consensus-valid (ConnectTip might fail) * Does not imply the transactions are still stored on disk. (IsBlockPruned might return true) * - * Note that this will be true for the snapshot base block, if one is loaded (and - * all subsequent assumed-valid blocks) since its nChainTx value will have been set - * manually based on the related AssumeutxoData entry. + * Note that this will be true for the snapshot base block, if one is loaded, since its nChainTx value will have + * been set manually based on the related AssumeutxoData entry. */ bool HaveNumChainTxs() const { return nChainTx != 0; } diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index e2a88eacdda..ff95e64b7ed 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -91,13 +91,16 @@ CreateAndActivateUTXOSnapshot( // these blocks instead CBlockIndex *pindex = orig_tip; while (pindex && pindex != chain.m_chain.Tip()) { - pindex->nStatus &= ~BLOCK_HAVE_DATA; - pindex->nStatus &= ~BLOCK_HAVE_UNDO; - // We have to set the ASSUMED_VALID flag, because otherwise it - // would not be possible to have a block index entry without HAVE_DATA - // and with nTx > 0 (since we aren't setting the pruned flag); - // see CheckBlockIndex(). - pindex->nStatus |= BLOCK_ASSUMED_VALID; + // Remove all data and validity flags by just setting + // BLOCK_VALID_TREE. Also reset transaction counts and sequence + // ids that are set when blocks are received, to make test setup + // more realistic and satisfy consistency checks in + // CheckBlockIndex(). + assert(pindex->IsValid(BlockStatus::BLOCK_VALID_TREE)); + pindex->nStatus = BlockStatus::BLOCK_VALID_TREE; + pindex->nTx = 0; + pindex->nChainTx = 0; + pindex->nSequenceId = 0; pindex = pindex->pprev; } } diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 4bbab1cdcd2..26f9ab59a6a 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -464,6 +464,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) // Blocks with heights in range [91, 110] are marked ASSUMED_VALID if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) { index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID; + index->nTx = 0; + index->nChainTx = 0; } ++num_indexes; diff --git a/src/validation.cpp b/src/validation.cpp index 4cb004c7cc1..f27f47aea03 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3660,7 +3660,18 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd { AssertLockHeld(cs_main); pindexNew->nTx = block.vtx.size(); - pindexNew->nChainTx = 0; + // Typically nChainTx will be 0 at this point, but it can be nonzero if this + // is a pruned block which is being downloaded again, or if this is an + // assumeutxo snapshot block which has a hardcoded nChainTx value from the + // snapshot metadata. If the pindex is not the snapshot block and the + // nChainTx value is not zero, assert that value is actually correct. + auto prev_tx_sum = [](CBlockIndex& block) { return block.nTx + (block.pprev ? block.pprev->nChainTx : 0); }; + if (!Assume(pindexNew->nChainTx == 0 || pindexNew->nChainTx == prev_tx_sum(*pindexNew) || + pindexNew == GetSnapshotBaseBlock())) { + LogWarning("Internal bug detected: block %d has unexpected nChainTx %i that should be %i (%s %s). Please report this issue here: %s\n", + pindexNew->nHeight, pindexNew->nChainTx, prev_tx_sum(*pindexNew), PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT); + pindexNew->nChainTx = 0; + } pindexNew->nFile = pos.nFile; pindexNew->nDataPos = pos.nPos; pindexNew->nUndoPos = 0; @@ -3680,7 +3691,15 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd while (!queue.empty()) { CBlockIndex *pindex = queue.front(); queue.pop_front(); - pindex->nChainTx = (pindex->pprev ? pindex->pprev->nChainTx : 0) + pindex->nTx; + // Before setting nChainTx, assert that it is 0 or already set to + // the correct value. This assert will fail after receiving the + // assumeutxo snapshot block if assumeutxo snapshot metadata has an + // incorrect hardcoded AssumeutxoData::nChainTx value. + if (!Assume(pindex->nChainTx == 0 || pindex->nChainTx == prev_tx_sum(*pindex))) { + LogWarning("Internal bug detected: block %d has unexpected nChainTx %i that should be %i (%s %s). Please report this issue here: %s\n", + pindex->nHeight, pindex->nChainTx, prev_tx_sum(*pindex), PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT); + } + pindex->nChainTx = prev_tx_sum(*pindex); pindex->nSequenceId = nBlockSequenceId++; for (Chainstate *c : GetAll()) { c->TryAddBlockIndexCandidate(pindex); @@ -5023,13 +5042,30 @@ void ChainstateManager::CheckBlockIndex() size_t nNodes = 0; int nHeight = 0; CBlockIndex* pindexFirstInvalid = nullptr; // Oldest ancestor of pindex which is invalid. - CBlockIndex* pindexFirstMissing = nullptr; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA. - CBlockIndex* pindexFirstNeverProcessed = nullptr; // Oldest ancestor of pindex for which nTx == 0. + CBlockIndex* pindexFirstMissing = nullptr; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA, since assumeutxo snapshot if used. + CBlockIndex* pindexFirstNeverProcessed = nullptr; // Oldest ancestor of pindex for which nTx == 0, since assumeutxo snapshot if used. CBlockIndex* pindexFirstNotTreeValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not). - CBlockIndex* pindexFirstNotTransactionsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TRANSACTIONS (regardless of being valid or not). - CBlockIndex* pindexFirstNotChainValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not). - CBlockIndex* pindexFirstNotScriptsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not). + CBlockIndex* pindexFirstNotTransactionsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TRANSACTIONS (regardless of being valid or not), since assumeutxo snapshot if used. + CBlockIndex* pindexFirstNotChainValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not), since assumeutxo snapshot if used. + CBlockIndex* pindexFirstNotScriptsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not), since assumeutxo snapshot if used. CBlockIndex* pindexFirstAssumeValid = nullptr; // Oldest ancestor of pindex which has BLOCK_ASSUMED_VALID + + // After checking an assumeutxo snapshot block, reset pindexFirst pointers + // to earlier blocks that have not been downloaded or validated yet, so + // checks for later blocks can assume the earlier blocks were validated and + // be stricter, testing for more requirements. + const CBlockIndex* snap_base{GetSnapshotBaseBlock()}; + CBlockIndex *snap_first_missing{}, *snap_first_notx{}, *snap_first_notv{}, *snap_first_nocv{}, *snap_first_nosv{}; + auto snap_update_firsts = [&] { + if (pindex == snap_base) { + std::swap(snap_first_missing, pindexFirstMissing); + std::swap(snap_first_notx, pindexFirstNeverProcessed); + std::swap(snap_first_notv, pindexFirstNotTransactionsValid); + std::swap(snap_first_nocv, pindexFirstNotChainValid); + std::swap(snap_first_nosv, pindexFirstNotScriptsValid); + } + }; + while (pindex != nullptr) { nNodes++; if (pindexFirstAssumeValid == nullptr && pindex->nStatus & BLOCK_ASSUMED_VALID) pindexFirstAssumeValid = pindex; @@ -5040,10 +5076,7 @@ void ChainstateManager::CheckBlockIndex() if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex; if (pindex->pprev != nullptr && pindexFirstNotTreeValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TREE) pindexFirstNotTreeValid = pindex; - if (pindex->pprev != nullptr && !pindex->IsAssumedValid()) { - // Skip validity flag checks for BLOCK_ASSUMED_VALID index entries, since these - // *_VALID_MASK flags will not be present for index entries we are temporarily assuming - // valid. + if (pindex->pprev != nullptr) { if (pindexFirstNotTransactionsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS) { pindexFirstNotTransactionsValid = pindex; @@ -5073,36 +5106,26 @@ void ChainstateManager::CheckBlockIndex() if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock) // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred). // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred. - // Unless these indexes are assumed valid and pending block download on a - // background chainstate. - if (!m_blockman.m_have_pruned && !pindex->IsAssumedValid()) { + if (!m_blockman.m_have_pruned) { // If we've never pruned, then HAVE_DATA should be equivalent to nTx > 0 assert(!(pindex->nStatus & BLOCK_HAVE_DATA) == (pindex->nTx == 0)); - if (pindexFirstAssumeValid == nullptr) { - // If we've got some assume valid blocks, then we might have - // missing blocks (not HAVE_DATA) but still treat them as - // having been processed (with a fake nTx value). Otherwise, we - // can assert that these are the same. - assert(pindexFirstMissing == pindexFirstNeverProcessed); - } + assert(pindexFirstMissing == pindexFirstNeverProcessed); } else { // If we have pruned, then we can only say that HAVE_DATA implies nTx > 0 if (pindex->nStatus & BLOCK_HAVE_DATA) assert(pindex->nTx > 0); } if (pindex->nStatus & BLOCK_HAVE_UNDO) assert(pindex->nStatus & BLOCK_HAVE_DATA); if (pindex->IsAssumedValid()) { - // Assumed-valid blocks should have some nTx value. - assert(pindex->nTx > 0); // Assumed-valid blocks should connect to the main chain. assert((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TREE); - } else { - // Otherwise there should only be an nTx value if we have - // actually seen a block's transactions. - assert(((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TRANSACTIONS) == (pindex->nTx > 0)); // This is pruning-independent. } + // There should only be an nTx value if we have + // actually seen a block's transactions. + assert(((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TRANSACTIONS) == (pindex->nTx > 0)); // This is pruning-independent. // All parents having had data (at some point) is equivalent to all parents being VALID_TRANSACTIONS, which is equivalent to HaveNumChainTxs(). - assert((pindexFirstNeverProcessed == nullptr) == pindex->HaveNumChainTxs()); - assert((pindexFirstNotTransactionsValid == nullptr) == pindex->HaveNumChainTxs()); + // HaveNumChainTxs will also be set in the assumeutxo snapshot block from snapshot metadata. + assert((pindexFirstNeverProcessed == nullptr || pindex == snap_base) == pindex->HaveNumChainTxs()); + assert((pindexFirstNotTransactionsValid == nullptr || pindex == snap_base) == pindex->HaveNumChainTxs()); assert(pindex->nHeight == nHeight); // nHeight must be consistent. assert(pindex->pprev == nullptr || pindex->nChainWork >= pindex->pprev->nChainWork); // For every block except the genesis block, the chainwork must be larger than the parent's. assert(nHeight < 2 || (pindex->pskip && (pindex->pskip->nHeight < nHeight))); // The pskip pointer must point back for all but the first 2 blocks. @@ -5115,14 +5138,18 @@ void ChainstateManager::CheckBlockIndex() assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents. } // Make sure nChainTx sum is correctly computed. - unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0; - assert((pindex->nChainTx == pindex->nTx + prev_chain_tx) - // Transaction may be completely unset - happens if only the header was accepted but the block hasn't been processed. - || (pindex->nChainTx == 0 && pindex->nTx == 0) - // nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet) - || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev) - // Transaction counts prior to snapshot are unknown. - || pindex->IsAssumedValid()); + if (!pindex->pprev) { + // If no previous block, nTx and nChainTx must be the same. + assert(pindex->nChainTx == pindex->nTx); + } else if (pindex->pprev->nChainTx > 0 && pindex->nTx > 0) { + // If previous nChainTx is set and number of transactions in block is known, sum must be set. + assert(pindex->nChainTx == pindex->nTx + pindex->pprev->nChainTx); + } else { + // Otherwise nChainTx should only be set if this is a snapshot + // block, and must be set if it is. + assert((pindex->nChainTx != 0) == (pindex == snap_base)); + } + // Chainstate-specific checks on setBlockIndexCandidates for (auto c : GetAll()) { if (c->m_chain.Tip() == nullptr) continue; @@ -5133,16 +5160,19 @@ void ChainstateManager::CheckBlockIndex() // candidate, and this will be asserted below. Otherwise it is a // potential candidate. // - // - If pindex or one of its parent blocks never downloaded - // transactions (pindexFirstNeverProcessed is non-null), it should - // not be a candidate, and this will be asserted below. Otherwise - // it is a potential candidate. - if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && pindexFirstNeverProcessed == nullptr) { + // - If pindex or one of its parent blocks back to the genesis block + // or an assumeutxo snapshot never downloaded transactions + // (pindexFirstNeverProcessed is non-null), it should not be a + // candidate, and this will be asserted below. The only exception + // is if pindex itself is an assumeutxo snapshot block. Then it is + // also a potential candidate. + if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && (pindexFirstNeverProcessed == nullptr || pindex == snap_base)) { // If pindex was detected as invalid (pindexFirstInvalid is // non-null), it is not required to be in // setBlockIndexCandidates. if (pindexFirstInvalid == nullptr) { - // If pindex and all its parents downloaded transactions, + // If pindex and all its parents back to the genesis block + // or an assumeutxo snapshot block downloaded transactions, // and the transactions were not pruned (pindexFirstMissing // is null), it is a potential candidate. The check // excludes pruned blocks, because if any blocks were @@ -5155,13 +5185,17 @@ void ChainstateManager::CheckBlockIndex() // // If pindex is the chain tip, it also is a potential // candidate. - if ((pindexFirstMissing == nullptr || pindex == c->m_chain.Tip())) { + // + // If the chainstate was loaded from a snapshot and pindex + // is the base of the snapshot, pindex is also a potential + // candidate. + if (pindexFirstMissing == nullptr || pindex == c->m_chain.Tip() || pindex == c->SnapshotBase()) { // If this chainstate is the active chainstate, pindex // must be in setBlockIndexCandidates. Otherwise, this // chainstate is a background validation chainstate, and // pindex only needs to be added if it is an ancestor of // the snapshot that is being validated. - if (c == &ActiveChainstate() || GetSnapshotBaseBlock()->GetAncestor(pindex->nHeight) == pindex) { + if (c == &ActiveChainstate() || snap_base->GetAncestor(pindex->nHeight) == pindex) { assert(c->setBlockIndexCandidates.count(pindex)); } } @@ -5192,7 +5226,7 @@ void ChainstateManager::CheckBlockIndex() if (pindexFirstMissing == nullptr) assert(!foundInUnlinked); // We aren't missing data for any parent -- cannot be in m_blocks_unlinked. if (pindex->pprev && (pindex->nStatus & BLOCK_HAVE_DATA) && pindexFirstNeverProcessed == nullptr && pindexFirstMissing != nullptr) { // We HAVE_DATA for this block, have received data for all parents at some point, but we're currently missing data for some parent. - assert(m_blockman.m_have_pruned || pindexFirstAssumeValid != nullptr); // We must have pruned, or else we're using a snapshot (causing us to have faked the received data for some parent(s)). + assert(m_blockman.m_have_pruned); // This block may have entered m_blocks_unlinked if: // - it has a descendant that at some point had more work than the // tip, and @@ -5205,7 +5239,7 @@ void ChainstateManager::CheckBlockIndex() const bool is_active = c == &ActiveChainstate(); if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && c->setBlockIndexCandidates.count(pindex) == 0) { if (pindexFirstInvalid == nullptr) { - if (is_active || GetSnapshotBaseBlock()->GetAncestor(pindex->nHeight) == pindex) { + if (is_active || snap_base->GetAncestor(pindex->nHeight) == pindex) { assert(foundInUnlinked); } } @@ -5216,6 +5250,7 @@ void ChainstateManager::CheckBlockIndex() // End: actual consistency checks. // Try descending into the first subnode. + snap_update_firsts(); std::pair::iterator,std::multimap::iterator> range = forward.equal_range(pindex); if (range.first != range.second) { // A subnode was found. @@ -5227,6 +5262,7 @@ void ChainstateManager::CheckBlockIndex() // Move upwards until we reach a node of which we have not yet visited the last child. while (pindex) { // We are going to either move to a parent or a sibling of pindex. + snap_update_firsts(); // If pindex was the first with a certain property, unset the corresponding variable. if (pindex == pindexFirstInvalid) pindexFirstInvalid = nullptr; if (pindex == pindexFirstMissing) pindexFirstMissing = nullptr; @@ -5730,14 +5766,6 @@ bool ChainstateManager::PopulateAndValidateSnapshot( for (int i = AFTER_GENESIS_START; i <= snapshot_chainstate.m_chain.Height(); ++i) { index = snapshot_chainstate.m_chain[i]; - // Fake nTx so that LoadBlockIndex() loads assumed-valid CBlockIndex - // entries (among other things) - if (!index->nTx) { - index->nTx = 1; - } - // Fake nChainTx so that GuessVerificationProgress reports accurately - index->nChainTx = index->pprev->nChainTx + index->nTx; - // Mark unvalidated block index entries beneath the snapshot base block as assumed-valid. if (!index->IsValid(BLOCK_VALID_SCRIPTS)) { // This flag will be removed once the block is fully validated by a @@ -5760,6 +5788,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( } assert(index); + assert(index == snapshot_start_block); index->nChainTx = au_data.nChainTx; snapshot_chainstate.setBlockIndexCandidates.insert(snapshot_start_block); diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index a29ee8be8b1..27910c39098 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -244,20 +244,19 @@ class AssumeutxoTest(BitcoinTestFramework): tx = n1.getblockheader(block.hash)["nTx"] chain_tx = n1.getchaintxstats(nblocks=1, blockhash=block.hash)["txcount"] - # Intermediate nTx of the starting block should be real, but nTx of - # later blocks should be fake 1 values set by snapshot loading code. + # Intermediate nTx of the starting block should be set, but nTx of + # later blocks should be 0 before they are downloaded. if final or height == START_HEIGHT: assert_equal(tx, block.tx) else: - assert_equal(tx, 1) + assert_equal(tx, 0) # Intermediate nChainTx of the starting block and snapshot block - # should be real, but others will be fake values set by snapshot - # loading code. + # should be set, but others should be 0 until they are downloaded. if final or height in (START_HEIGHT, SNAPSHOT_BASE_HEIGHT): assert_equal(chain_tx, block.chain_tx) else: - assert_equal(chain_tx, height + 1) + assert_equal(chain_tx, 0) check_tx_counts(final=False)