diff options
author | Ava Chow <github@achow101.com> | 2024-03-20 12:49:19 -0400 |
---|---|---|
committer | Ava Chow <github@achow101.com> | 2024-03-20 12:56:49 -0400 |
commit | b50554babdddf452acaa51bac757736766c70e81 (patch) | |
tree | b6919c8198465392c12050f22bca5464d130364f /src/validation.cpp | |
parent | 69ddee6f393ecd604f0aed27bba47c59dc656a4a (diff) | |
parent | 9d9a7458a2570f7db56ab626b22010591089c312 (diff) | |
download | bitcoin-b50554babdddf452acaa51bac757736766c70e81.tar.xz |
Merge bitcoin/bitcoin#29370: assumeutxo: Get rid of faked nTx and nChainTx values
9d9a7458a2570f7db56ab626b22010591089c312 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed21c08f38e5d4b537b6decfd1f646db9 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d767b842a7925785a7053400c0e1cb55a test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662309a438121a83f27fd7bdd1779700c assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf980d657a277c85d113c2ae3e870e0ec doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6bef63bb360ccc5c039a3c11676c38e3 validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c21a2f5b47e8eab10fb13c604afed85 ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec94b6ccafb5bc44b7df3daeb473fdea assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)
Pull request description:
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.
Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.
Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
ACKs for top commit:
Sjors:
re-ACK 9d9a7458a2570f7db56ab626b22010591089c312
achow101:
ACK 9d9a7458a2570f7db56ab626b22010591089c312
mzumsande:
Tested ACK 9d9a7458a2570f7db56ab626b22010591089c312
maflcko:
ACK 9d9a7458a2570f7db56ab626b22010591089c312 🎯
Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
Diffstat (limited to 'src/validation.cpp')
-rw-r--r-- | src/validation.cpp | 193 |
1 files changed, 120 insertions, 73 deletions
diff --git a/src/validation.cpp b/src/validation.cpp index dd8457b490..b6d0c38f39 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3687,7 +3687,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; @@ -3707,7 +3718,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); @@ -5050,16 +5069,31 @@ 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* pindexFirstAssumeValid = nullptr; // Oldest ancestor of pindex which has BLOCK_ASSUMED_VALID + 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. + + // 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; if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex; if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) { pindexFirstMissing = pindex; @@ -5067,10 +5101,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; @@ -5100,36 +5131,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); + if (snap_base && snap_base->GetAncestor(pindex->nHeight) == pindex) { // 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. @@ -5142,30 +5163,64 @@ 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; - if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && pindexFirstNeverProcessed == nullptr) { + // Two main factors determine whether pindex is a candidate in + // setBlockIndexCandidates: + // + // - If pindex has less work than the chain tip, it should not be a + // candidate, and this will be asserted below. Otherwise it is a + // potential candidate. + // + // - 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) { - const bool is_active = c == &ActiveChainstate(); - // If this block sorts at least as good as the current tip and - // is valid and we have all data for its parents, it must be in - // setBlockIndexCandidates. m_chain.Tip() must also be there - // even if some data has been pruned. + // 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 + // pruned between pindex the current chain tip, pindex will + // only temporarily be added to setBlockIndexCandidates, + // before being moved to m_blocks_unlinked. This check + // could be improved to verify that if all blocks between + // the chain tip and pindex have data, pindex must be a + // candidate. + // + // If pindex is the chain tip, it also is a potential + // candidate. // - if ((pindexFirstMissing == nullptr || pindex == c->m_chain.Tip())) { - // The active chainstate should always have this block - // as a candidate, but a background chainstate should - // only have it if it is an ancestor of the snapshot base. - if (is_active || GetSnapshotBaseBlock()->GetAncestor(pindex->nHeight) == pindex) { + // 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() || snap_base->GetAncestor(pindex->nHeight) == pindex) { assert(c->setBlockIndexCandidates.count(pindex)); } } @@ -5196,7 +5251,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 @@ -5209,7 +5264,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); } } @@ -5220,6 +5275,7 @@ void ChainstateManager::CheckBlockIndex() // End: actual consistency checks. // Try descending into the first subnode. + snap_update_firsts(); std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> range = forward.equal_range(pindex); if (range.first != range.second) { // A subnode was found. @@ -5231,6 +5287,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; @@ -5239,7 +5296,6 @@ void ChainstateManager::CheckBlockIndex() if (pindex == pindexFirstNotTransactionsValid) pindexFirstNotTransactionsValid = nullptr; if (pindex == pindexFirstNotChainValid) pindexFirstNotChainValid = nullptr; if (pindex == pindexFirstNotScriptsValid) pindexFirstNotScriptsValid = nullptr; - if (pindex == pindexFirstAssumeValid) pindexFirstAssumeValid = nullptr; // Find our parent. CBlockIndex* pindexPar = pindex->pprev; // Find which child we just visited. @@ -5313,6 +5369,12 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin if (pindex == nullptr) return 0.0; + if (!Assume(pindex->nChainTx > 0)) { + LogWarning("Internal bug detected: block %d has unset nChainTx (%s %s). Please report this issue here: %s\n", + pindex->nHeight, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT); + return 0.0; + } + int64_t nNow = time(nullptr); double fTxTotal; @@ -5719,30 +5781,14 @@ bool ChainstateManager::PopulateAndValidateSnapshot( // Fake various pieces of CBlockIndex state: CBlockIndex* index = nullptr; - // Don't make any modifications to the genesis block. - // This is especially important because we don't want to erroneously - // apply BLOCK_ASSUMED_VALID to genesis, which would happen if we didn't skip - // it here (since it apparently isn't BLOCK_VALID_SCRIPTS). + // Don't make any modifications to the genesis block since it shouldn't be + // neccessary, and since the genesis block doesn't have normal flags like + // BLOCK_VALID_SCRIPTS set. constexpr int AFTER_GENESIS_START{1}; 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 - // background chainstate. - index->nStatus |= BLOCK_ASSUMED_VALID; - } - // Fake BLOCK_OPT_WITNESS so that Chainstate::NeedsRedownload() // won't ask to rewind the entire assumed-valid chain on startup. if (DeploymentActiveAt(*index, *this, Consensus::DEPLOYMENT_SEGWIT)) { @@ -5758,6 +5804,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( } assert(index); + assert(index == snapshot_start_block); index->nChainTx = au_data.nChainTx; snapshot_chainstate.setBlockIndexCandidates.insert(snapshot_start_block); |