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/test | |
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/test')
-rw-r--r-- | src/test/util/chainstate.h | 17 | ||||
-rw-r--r-- | src/test/validation_chainstatemanager_tests.cpp | 21 |
2 files changed, 16 insertions, 22 deletions
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index e2a88eacdd..ff95e64b7e 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 4bbab1cdcd..4bf66a55eb 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -276,9 +276,6 @@ struct SnapshotTestSetup : TestChain100Setup { BOOST_CHECK_EQUAL( *node::ReadSnapshotBaseBlockhash(found), *chainman.SnapshotBlockhash()); - - // Ensure that the genesis block was not marked assumed-valid. - BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid()); } const auto& au_data = ::Params().AssumeutxoForHeight(snapshot_height); @@ -410,7 +407,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, SnapshotTestSetup) //! - First, verify that setBlockIndexCandidates is as expected when using a single, //! fully-validating chainstate. //! -//! - Then mark a region of the chain BLOCK_ASSUMED_VALID and introduce a second chainstate +//! - Then mark a region of the chain as missing data and introduce a second chainstate //! that will tolerate assumed-valid blocks. Run LoadBlockIndex() and ensure that the first //! chainstate only contains fully validated blocks and the other chainstate contains all blocks, //! except those marked assume-valid, because those entries don't HAVE_DATA. @@ -421,7 +418,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) Chainstate& cs1 = chainman.ActiveChainstate(); int num_indexes{0}; - int num_assumed_valid{0}; // Blocks in range [assumed_valid_start_idx, last_assumed_valid_idx) will be // marked as assumed-valid and not having data. const int expected_assumed_valid{20}; @@ -456,35 +452,30 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) reload_all_block_indexes(); BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), 1); - // Mark some region of the chain assumed-valid, and remove the HAVE_DATA flag. + // Reset some region of the chain's nStatus, removing the HAVE_DATA flag. for (int i = 0; i <= cs1.m_chain.Height(); ++i) { LOCK(::cs_main); auto index = cs1.m_chain[i]; - // Blocks with heights in range [91, 110] are marked ASSUMED_VALID + // Blocks with heights in range [91, 110] are marked as missing data. if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) { - index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID; + index->nStatus = BlockStatus::BLOCK_VALID_TREE; + index->nTx = 0; + index->nChainTx = 0; } ++num_indexes; - if (index->IsAssumedValid()) ++num_assumed_valid; // Note the last fully-validated block as the expected validated tip. if (i == (assumed_valid_start_idx - 1)) { validated_tip = index; - BOOST_CHECK(!index->IsAssumedValid()); } // Note the last assumed valid block as the snapshot base if (i == last_assumed_valid_idx - 1) { assumed_base = index; - BOOST_CHECK(index->IsAssumedValid()); - } else if (i == last_assumed_valid_idx) { - BOOST_CHECK(!index->IsAssumedValid()); } } - BOOST_CHECK_EQUAL(expected_assumed_valid, num_assumed_valid); - // Note: cs2's tip is not set when ActivateExistingSnapshot is called. Chainstate& cs2 = WITH_LOCK(::cs_main, return chainman.ActivateExistingSnapshot(*assumed_base->phashBlock)); |