diff options
author | MarcoFalke <falke.marco@gmail.com> | 2021-04-07 07:33:18 +0200 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2021-04-07 07:33:27 +0200 |
commit | 41a8d2b96ff5cbc2efa9b0996f30228d0a625fdb (patch) | |
tree | cc14f35416ce54aba331744327af61e6b7eecadb /src | |
parent | 245a5cd5604a869776e5e274d554295ff1890ccb (diff) | |
parent | fa9b74f5ea89624e052934c48391b5076a87ffef (diff) | |
download | bitcoin-41a8d2b96ff5cbc2efa9b0996f30228d0a625fdb.tar.xz |
Merge #21582: Fix assumeutxo crash due to missing base_blockhash
fa9b74f5ea89624e052934c48391b5076a87ffef Fix assumeutxo crash due to missing base_blockhash (MarcoFalke)
fa8fffebe8ac126f31143619843dd6578a2f4e3c refactor: Prefer clean assert over UB in coinstats (MarcoFalke)
Pull request description:
This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:
* Adds an `Assert` to transform the UB into a clean crash, even when sanitizers are disabled
* Adds an early-fail condition to avoid the crash
ACKs for top commit:
jamesob:
ACK fa9b74f5ea89624e052934c48391b5076a87ffef ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))
ryanofsky:
Code review ACK fa9b74f5ea89624e052934c48391b5076a87ffef with no code changes since last review, just splitting up combocommit a little.
Tree-SHA512: dd36808a09f49c647543a9eaa6fdb785b3f1109af48ba4cc983153b22a144da9ca61af22034dcfaa0e192a65b1ee7de744f187555079aff55bec0efa0ce87cd4
Diffstat (limited to 'src')
-rw-r--r-- | src/node/coinstats.cpp | 3 | ||||
-rw-r--r-- | src/test/validation_chainstatemanager_tests.cpp | 5 | ||||
-rw-r--r-- | src/validation.cpp | 34 |
3 files changed, 16 insertions, 26 deletions
diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 268580c6e6..f8f0fff43f 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -94,7 +94,8 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& { LOCK(cs_main); assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); - stats.nHeight = blockman.LookupBlockIndex(stats.hashBlock)->nHeight; + const CBlockIndex* block = blockman.LookupBlockIndex(stats.hashBlock); + stats.nHeight = Assert(block)->nHeight; } PrepareHash(hash_obj, stats); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 94d4277019..35e087c899 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -259,6 +259,11 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Determi // Coins count is smaller than coins in file metadata.m_coins_count -= 1; })); + BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot( + m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) { + // Wrong hash + metadata.m_base_blockhash = uint256::ONE; + })); BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root)); diff --git a/src/validation.cpp b/src/validation.cpp index 19363c0efb..619d3cea98 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5423,6 +5423,15 @@ bool ChainstateManager::PopulateAndValidateSnapshot( assert(coins_cache.GetBestBlock() == base_blockhash); + CBlockIndex* snapshot_start_block = WITH_LOCK(::cs_main, return m_blockman.LookupBlockIndex(base_blockhash)); + + if (!snapshot_start_block) { + // Needed for GetUTXOStats to determine the height + LogPrintf("[snapshot] Did not find snapshot start blockheader %s\n", + base_blockhash.ToString()); + return false; + } + CCoinsStats stats; auto breakpoint_fnc = [] { /* TODO insert breakpoint here? */ }; @@ -5435,31 +5444,6 @@ bool ChainstateManager::PopulateAndValidateSnapshot( return false; } - // Ensure that the base blockhash appears in the known chain of valid headers. We're willing to - // wait a bit here because the snapshot may have been loaded on startup, before we've - // received headers from the network. - - int max_secs_to_wait_for_headers = 60 * 10; - CBlockIndex* snapshot_start_block = nullptr; - - while (max_secs_to_wait_for_headers > 0) { - snapshot_start_block = WITH_LOCK(::cs_main, - return m_blockman.LookupBlockIndex(base_blockhash)); - --max_secs_to_wait_for_headers; - - if (!snapshot_start_block) { - std::this_thread::sleep_for(std::chrono::seconds(1)); - } else { - break; - } - } - - if (snapshot_start_block == nullptr) { - LogPrintf("[snapshot] timed out waiting for snapshot start blockheader %s\n", - base_blockhash.ToString()); - return false; - } - // Assert that the deserialized chainstate contents match the expected assumeutxo value. int base_height = snapshot_start_block->nHeight; |