From fae8c73d9e4eba4603447bb52b6e3e760fbf15f8 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 13 Aug 2024 10:00:45 +0200 Subject: [PATCH 1/4] test: Disallow fee_estimator construction in ChainTestingSetup It is expensive to construct, and only one test uses it. Fix both issues by disallowing the construction and moving it to the single test that uses it. --- src/test/policyestimator_tests.cpp | 5 +++-- src/test/util/setup_common.cpp | 6 ++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 6cadc3290a..83977c1c89 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -1,8 +1,9 @@ -// Copyright (c) 2011-2022 The Bitcoin Core developers +// Copyright (c) 2011-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -18,7 +19,7 @@ BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, ChainTestingSetup) BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { - CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator); + CBlockPolicyEstimator feeEst{FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES}; CTxMemPool& mpool = *Assert(m_node.mempool); m_node.validation_signals->RegisterValidationInterface(&feeEst); TestMemPoolEntryHelper entry; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 3f48ea4375..2d6bc3c988 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2022 The Bitcoin Core developers +// Copyright (c) 2011-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -31,7 +31,6 @@ #include #include #include -#include #include #include #include @@ -236,7 +235,6 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) m_node.validation_signals = std::make_unique(std::make_unique(*m_node.scheduler)); } - m_node.fee_estimator = std::make_unique(FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES); bilingual_str error{}; m_node.mempool = std::make_unique(MemPoolOptionsForTest(m_node), error); Assert(error.empty()); @@ -276,7 +274,7 @@ ChainTestingSetup::~ChainTestingSetup() m_node.netgroupman.reset(); m_node.args = nullptr; m_node.mempool.reset(); - m_node.fee_estimator.reset(); + Assert(!m_node.fee_estimator); // Each test must create a local object, if they wish to use the fee_estimator m_node.chainman.reset(); m_node.validation_signals.reset(); m_node.scheduler.reset(); From fa645c7a861ffa83a53a459263b6a620defe31f9 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 15 Aug 2024 08:53:56 +0200 Subject: [PATCH 2/4] fuzz: Remove unused DataStream object --- src/test/fuzz/utxo_snapshot.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index d82f166765..7401a613b7 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -52,7 +52,6 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) std::vector metadata{ConsumeRandomLengthByteVector(fuzzed_data_provider)}; outfile << Span{metadata}; } else { - DataStream data_stream{}; auto msg_start = chainman.GetParams().MessageStart(); int base_blockheight{fuzzed_data_provider.ConsumeIntegralInRange(1, 2 * COINBASE_MATURITY)}; uint256 base_blockhash{g_chain->at(base_blockheight - 1)->GetHash()}; From fa386642b4dfd88f74488c288c7886494d69f4ed Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 13 Aug 2024 10:38:20 +0200 Subject: [PATCH 3/4] fuzz: Speed up utxo_snapshot by lazy re-init The re-init is expensive, so skip it if there is no need. Also, add an even faster fuzz target utxo_snapshot_invalid, which does not need any re-init at all. --- src/test/fuzz/utxo_snapshot.cpp | 109 ++++++++++++++++++++++++++------ src/test/util/setup_common.cpp | 40 +++++++----- src/test/util/setup_common.h | 1 + 3 files changed, 114 insertions(+), 36 deletions(-) diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 7401a613b7..0a0896650f 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -1,45 +1,78 @@ -// Copyright (c) 2021-2022 The Bitcoin Core developers +// Copyright (c) 2021-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include +#include +#include #include +#include #include +#include +#include +#include +#include +#include +#include #include #include #include #include #include -#include +#include +#include #include +#include #include -#include + +#include +#include +#include +#include +#include +#include using node::SnapshotMetadata; namespace { const std::vector>* g_chain; +TestingSetup* g_setup; +template void initialize_chain() { const auto params{CreateChainParams(ArgsManager{}, ChainType::REGTEST)}; static const auto chain{CreateBlockChain(2 * COINBASE_MATURITY, *params)}; g_chain = &chain; + static const auto setup{ + MakeNoLogFileContext(ChainType::REGTEST, + TestOpts{ + .setup_net = false, + .setup_validation_interface = false, + }), + }; + if constexpr (INVALID) { + auto& chainman{*setup->m_node.chainman}; + for (const auto& block : chain) { + BlockValidationState dummy; + bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)}; + Assert(processed); + const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))}; + Assert(index); + } + } + g_setup = setup.get(); } -FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) +template +void utxo_snapshot_fuzz(FuzzBufferType buffer) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - std::unique_ptr setup{ - MakeNoLogFileContext( - ChainType::REGTEST, - TestOpts{ - .setup_net = false, - .setup_validation_interface = false, - })}; - const auto& node = setup->m_node; - auto& chainman{*node.chainman}; + auto& setup{*g_setup}; + bool dirty_chainman{false}; // Re-use the global chainman, but reset it when it is dirty + auto& chainman{*setup.m_node.chainman}; const auto snapshot_path = gArgs.GetDataDirNet() / "fuzzed_snapshot.dat"; @@ -74,6 +107,16 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) height++; } } + if constexpr (INVALID) { + // Append an invalid coin to ensure invalidity. This error will be + // detected late in PopulateAndValidateSnapshot, and allows the + // INVALID fuzz target to reach more potential code coverage. + const auto& coinbase{g_chain->back()->vtx.back()}; + outfile << coinbase->GetHash(); + WriteCompactSize(outfile, 1); // number of coins for the hash + WriteCompactSize(outfile, 999); // index of coin + outfile << Coin{coinbase->vout[0], /*nHeightIn=*/999, /*fCoinBaseIn=*/0}; + } } const auto ActivateFuzzedSnapshot{[&] { @@ -89,12 +132,16 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) }}; if (fuzzed_data_provider.ConsumeBool()) { - for (const auto& block : *g_chain) { - BlockValidationState dummy; - bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)}; - Assert(processed); - const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))}; - Assert(index); + // Consume the bool, but skip the code for the INVALID fuzz target + if constexpr (!INVALID) { + for (const auto& block : *g_chain) { + BlockValidationState dummy; + bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)}; + Assert(processed); + const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))}; + Assert(index); + } + dirty_chainman = true; } } @@ -118,11 +165,35 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) } } Assert(g_chain->size() == coinscache.GetCacheSize()); + dirty_chainman = true; } else { Assert(!chainman.SnapshotBlockhash()); Assert(!chainman.ActiveChainstate().m_from_snapshot_blockhash); } // Snapshot should refuse to load a second time regardless of validity Assert(!ActivateFuzzedSnapshot()); + if constexpr (INVALID) { + // Activating the snapshot, or any other action that makes the chainman + // "dirty" can and must not happen for the INVALID fuzz target + Assert(!dirty_chainman); + } + if (dirty_chainman) { + setup.m_node.chainman.reset(); + setup.m_make_chainman(); + setup.LoadVerifyActivateChainstate(); + } } + +// There are two fuzz targets: +// +// The target 'utxo_snapshot', which allows valid snapshots, but is slow, +// because it has to reset the chainstate manager on almost all fuzz inputs. +// Otherwise, a dirty header tree or dirty chainstate could leak from one fuzz +// input execution into the next, which makes execution non-deterministic. +// +// The target 'utxo_snapshot_invalid', which is fast and does not require any +// expensive state to be reset. +FUZZ_TARGET(utxo_snapshot /*valid*/, .init = initialize_chain) { utxo_snapshot_fuzz(buffer); } +FUZZ_TARGET(utxo_snapshot_invalid, .init = initialize_chain) { utxo_snapshot_fuzz(buffer); } + } // namespace diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2d6bc3c988..cf47d16faf 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -244,24 +244,30 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) m_node.notifications = std::make_unique(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); - const ChainstateManager::Options chainman_opts{ - .chainparams = chainparams, - .datadir = m_args.GetDataDirNet(), - .check_block_index = 1, - .notifications = *m_node.notifications, - .signals = m_node.validation_signals.get(), - .worker_threads_num = 2, + m_make_chainman = [this, &chainparams] { + Assert(!m_node.chainman); + const ChainstateManager::Options chainman_opts{ + .chainparams = chainparams, + .datadir = m_args.GetDataDirNet(), + .check_block_index = 1, + .notifications = *m_node.notifications, + .signals = m_node.validation_signals.get(), + .worker_threads_num = 2, + }; + const BlockManager::Options blockman_opts{ + .chainparams = chainman_opts.chainparams, + .blocks_dir = m_args.GetBlocksDirPath(), + .notifications = chainman_opts.notifications, + }; + m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_opts); + LOCK(m_node.chainman->GetMutex()); + m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ + .path = m_args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = static_cast(m_cache_sizes.block_tree_db), + .memory_only = true, + }); }; - const BlockManager::Options blockman_opts{ - .chainparams = chainman_opts.chainparams, - .blocks_dir = m_args.GetBlocksDirPath(), - .notifications = chainman_opts.notifications, - }; - m_node.chainman = std::make_unique(*Assert(m_node.shutdown), chainman_opts, blockman_opts); - m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ - .path = m_args.GetDataDirNet() / "blocks" / "index", - .cache_bytes = static_cast(m_cache_sizes.block_tree_db), - .memory_only = true}); + m_make_chainman(); } ChainTestingSetup::~ChainTestingSetup() diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 9515f0255e..7d69551516 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -81,6 +81,7 @@ struct ChainTestingSetup : public BasicTestingSetup { node::CacheSizes m_cache_sizes{}; bool m_coins_db_in_memory{true}; bool m_block_tree_db_in_memory{true}; + std::function m_make_chainman{}; explicit ChainTestingSetup(const ChainType chainType = ChainType::MAIN, TestOpts = {}); ~ChainTestingSetup(); From fa899fb7aa8a14acecadd8936ad5824fa0f697ff Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 15 Aug 2024 16:43:11 +0200 Subject: [PATCH 4/4] fuzz: Speed up utxo_snapshot fuzz target This speeds up the fuzz target, which allows "valid" inputs. It does not affect the "INVALID" fuzz target. --- src/test/fuzz/utxo_snapshot.cpp | 1 + src/test/util/setup_common.cpp | 8 ++++++-- src/test/util/setup_common.h | 3 ++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 0a0896650f..63784c0621 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -51,6 +51,7 @@ void initialize_chain() TestOpts{ .setup_net = false, .setup_validation_interface = false, + .min_validation_cache = true, }), }; if constexpr (INVALID) { diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index cf47d16faf..62ff61b227 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -244,9 +244,9 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) m_node.notifications = std::make_unique(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); - m_make_chainman = [this, &chainparams] { + m_make_chainman = [this, &chainparams, opts] { Assert(!m_node.chainman); - const ChainstateManager::Options chainman_opts{ + ChainstateManager::Options chainman_opts{ .chainparams = chainparams, .datadir = m_args.GetDataDirNet(), .check_block_index = 1, @@ -254,6 +254,10 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) .signals = m_node.validation_signals.get(), .worker_threads_num = 2, }; + if (opts.min_validation_cache) { + chainman_opts.script_execution_cache_bytes = 0; + chainman_opts.signature_cache_bytes = 0; + } const BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 7d69551516..b73acc1de5 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -1,4 +1,4 @@ -// Copyright (c) 2015-2022 The Bitcoin Core developers +// Copyright (c) 2015-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -55,6 +55,7 @@ struct TestOpts { bool block_tree_db_in_memory{true}; bool setup_net{true}; bool setup_validation_interface{true}; + bool min_validation_cache{false}; // Equivalent of -maxsigcachebytes=0 }; /** Basic testing setup.