From facdb8b331da17c10b122deb28f6d71d5797f063 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 4 May 2023 12:19:35 +0200 Subject: [PATCH 1/3] Add BlockManagerOpts::chainparams reference and use it in blockstorage.cpp --- src/bitcoin-chainstate.cpp | 5 ++++- src/init.cpp | 8 ++++++-- src/kernel/blockmanager_opts.h | 3 +++ src/node/blockstorage.cpp | 6 +++--- src/node/blockstorage.h | 3 +++ src/test/blockmanager_tests.cpp | 5 ++++- src/test/util/setup_common.cpp | 5 ++++- src/test/validation_chainstatemanager_tests.cpp | 5 ++++- 8 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 04f6aae4f7..2b139cf908 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -86,7 +86,10 @@ int main(int argc, char* argv[]) .datadir = gArgs.GetDataDirNet(), .adjusted_time_callback = NodeClock::now, }; - ChainstateManager chainman{chainman_opts, {}}; + const node::BlockManager::Options blockman_opts{ + .chainparams = chainman_opts.chainparams, + }; + ChainstateManager chainman{chainman_opts, blockman_opts}; node::CacheSizes cache_sizes; cache_sizes.block_tree_db = 2 << 20; diff --git a/src/init.cpp b/src/init.cpp index 525648b812..ddee3bddc2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1036,7 +1036,9 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) { return InitError(*error); } - node::BlockManager::Options blockman_opts_dummy{}; + node::BlockManager::Options blockman_opts_dummy{ + .chainparams = chainman_opts_dummy.chainparams, + }; if (const auto error{ApplyArgsManOptions(args, blockman_opts_dummy)}) { return InitError(*error); } @@ -1439,7 +1441,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) }; Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction - node::BlockManager::Options blockman_opts{}; + node::BlockManager::Options blockman_opts{ + .chainparams = chainman_opts.chainparams, + }; Assert(!ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction // cache size calculations diff --git a/src/kernel/blockmanager_opts.h b/src/kernel/blockmanager_opts.h index 9dc93b6dd2..5584a66e02 100644 --- a/src/kernel/blockmanager_opts.h +++ b/src/kernel/blockmanager_opts.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H +class CChainParams; + namespace kernel { /** @@ -12,6 +14,7 @@ namespace kernel { * `BlockManager::Options` due to the using-declaration in `BlockManager`. */ struct BlockManagerOpts { + const CChainParams& chainparams; uint64_t prune_target{0}; }; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index abd71ca50b..d20c8391e8 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -255,7 +255,7 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) { - if (!m_block_tree_db->LoadBlockIndexGuts(consensus_params, [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) { + if (!m_block_tree_db->LoadBlockIndexGuts(GetConsensus(), [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) { return false; } @@ -729,7 +729,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid if (!FindUndoPos(state, pindex->nFile, _pos, ::GetSerializeSize(blockundo, CLIENT_VERSION) + 40)) { return error("ConnectBlock(): FindUndoPos failed"); } - if (!UndoWriteToDisk(blockundo, _pos, pindex->pprev->GetBlockHash(), chainparams.MessageStart())) { + if (!UndoWriteToDisk(blockundo, _pos, pindex->pprev->GetBlockHash(), GetParams().MessageStart())) { return AbortNode(state, "Failed to write undo data"); } // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order) @@ -847,7 +847,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha return FlatFilePos(); } if (!position_known) { - if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) { + if (!WriteBlockToDisk(block, blockPos, GetParams().MessageStart())) { AbortNode("Failed to write block"); return FlatFilePos(); } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 3eb27cc72d..59ad58c07f 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -81,6 +82,8 @@ class BlockManager friend ChainstateManager; private: + const CChainParams& GetParams() const { return m_opts.chainparams; } + const Consensus::Params& GetConsensus() const { return m_opts.chainparams.GetConsensus(); } /** * Load the blocktree off disk and into memory. Populate certain metadata * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 2118f476cd..3139f587fa 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -21,7 +21,10 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) { const auto params {CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN)}; - BlockManager blockman{{}}; + node::BlockManager::Options blockman_opts{ + .chainparams = *params, + }; + BlockManager blockman{blockman_opts}; CChain chain {}; // simulate adding a genesis block normally BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, *params, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index ddc1e7ab37..070575ecd2 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -185,7 +185,10 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve .adjusted_time_callback = GetAdjustedTime, .check_block_index = true, }; - m_node.chainman = std::make_unique(chainman_opts, node::BlockManager::Options{}); + node::BlockManager::Options blockman_opts{ + .chainparams = chainman_opts.chainparams, + }; + m_node.chainman = std::make_unique(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), diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index edc7e4b70a..85fe065a38 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -381,10 +381,13 @@ struct SnapshotTestSetup : TestChain100Setup { .datadir = m_args.GetDataDirNet(), .adjusted_time_callback = GetAdjustedTime, }; + node::BlockManager::Options blockman_opts{ + .chainparams = chainman_opts.chainparams, + }; // For robustness, ensure the old manager is destroyed before creating a // new one. m_node.chainman.reset(); - m_node.chainman = std::make_unique(chainman_opts, node::BlockManager::Options{}); + m_node.chainman = std::make_unique(chainman_opts, blockman_opts); } return *Assert(m_node.chainman); } From fa3f74a40efa0db3af31438e9250c41bc7406749 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 4 May 2023 12:30:34 +0200 Subject: [PATCH 2/3] Replace pindex pointer with block reference pindex can not be nullptr, so document that, and clear it up in the next commit. --- src/node/blockstorage.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index d20c8391e8..aa7c805f92 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -722,14 +722,15 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams) { + auto& block{*pindex}; AssertLockHeld(::cs_main); // Write undo information to disk - if (pindex->GetUndoPos().IsNull()) { + if (block.GetUndoPos().IsNull()) { FlatFilePos _pos; - if (!FindUndoPos(state, pindex->nFile, _pos, ::GetSerializeSize(blockundo, CLIENT_VERSION) + 40)) { + if (!FindUndoPos(state, block.nFile, _pos, ::GetSerializeSize(blockundo, CLIENT_VERSION) + 40)) { return error("ConnectBlock(): FindUndoPos failed"); } - if (!UndoWriteToDisk(blockundo, _pos, pindex->pprev->GetBlockHash(), GetParams().MessageStart())) { + if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash(), GetParams().MessageStart())) { return AbortNode(state, "Failed to write undo data"); } // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order) @@ -737,14 +738,14 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid // in the block file info as below; note that this does not catch the case where the undo writes are keeping up // with the block writes (usually when a synced up node is getting newly mined blocks) -- this case is caught in // the FindBlockPos function - if (_pos.nFile < m_last_blockfile && static_cast(pindex->nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) { + if (_pos.nFile < m_last_blockfile && static_cast(block.nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) { FlushUndoFile(_pos.nFile, true); } // update nUndoPos in block index - pindex->nUndoPos = _pos.nPos; - pindex->nStatus |= BLOCK_HAVE_UNDO; - m_dirty_blockindex.insert(pindex); + block.nUndoPos = _pos.nPos; + block.nStatus |= BLOCK_HAVE_UNDO; + m_dirty_blockindex.insert(&block); } return true; From fa5d7c39eb992467e43b12db213b27913374fb83 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 4 May 2023 12:40:21 +0200 Subject: [PATCH 3/3] Remove unused chainparams from BlockManager methods Also, replace pointer with reference while touching the signature. --- src/node/blockstorage.cpp | 11 +++++------ src/node/blockstorage.h | 8 ++++---- src/test/blockmanager_tests.cpp | 6 +++--- src/validation.cpp | 8 ++++---- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index aa7c805f92..bafee4e237 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -253,7 +253,7 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) return pindex; } -bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) +bool BlockManager::LoadBlockIndex() { if (!m_block_tree_db->LoadBlockIndexGuts(GetConsensus(), [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) { return false; @@ -318,9 +318,9 @@ bool BlockManager::WriteBlockIndexDB() return true; } -bool BlockManager::LoadBlockIndexDB(const Consensus::Params& consensus_params) +bool BlockManager::LoadBlockIndexDB() { - if (!LoadBlockIndex(consensus_params)) { + if (!LoadBlockIndex()) { return false; } @@ -720,9 +720,8 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa return true; } -bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams) +bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex& block) { - auto& block{*pindex}; AssertLockHeld(::cs_main); // Write undo information to disk if (block.GetUndoPos().IsNull()) { @@ -830,7 +829,7 @@ bool ReadRawBlockFromDisk(std::vector& block, const FlatFilePos& pos, c return true; } -FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp) +FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const FlatFilePos* dbp) { unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION); FlatFilePos blockPos; diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 59ad58c07f..8f699f8e68 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -89,7 +89,7 @@ private: * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral * collections like m_dirty_blockindex. */ - bool LoadBlockIndex(const Consensus::Params& consensus_params) + bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main); void FlushBlockFile(bool fFinalize = false, bool finalize_undo = false); void FlushUndoFile(int block_file, bool finalize = false); @@ -165,7 +165,7 @@ public: std::unique_ptr m_block_tree_db GUARDED_BY(::cs_main); bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool LoadBlockIndexDB(const Consensus::Params& consensus_params) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** * Remove any pruned block & undo files that are still on disk. @@ -187,11 +187,11 @@ public: /** Get block file info entry for one block file */ CBlockFileInfo* GetBlockFileInfo(size_t n); - bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams) + bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Store block on disk. If dbp is not nullptr, then it provides the known position of the block within a block file on disk. */ - FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp); + FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const FlatFilePos* dbp); /** Whether running in -prune mode. */ [[nodiscard]] bool IsPruneMode() const { return m_prune_mode; } diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 3139f587fa..321c86ebfa 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -27,20 +27,20 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) BlockManager blockman{blockman_opts}; CChain chain {}; // simulate adding a genesis block normally - BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, *params, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); + BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); // simulate what happens during reindex // simulate a well-formed genesis block being found at offset 8 in the blk00000.dat file // the block is found at offset 8 because there is an 8 byte serialization header // consisting of 4 magic bytes + 4 length bytes before each block in a well-formed blk file. FlatFilePos pos{0, BLOCK_SERIALIZATION_HEADER_SIZE}; - BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, *params, &pos).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); + BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, &pos).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); // now simulate what happens after reindex for the first new block processed // the actual block contents don't matter, just that it's a block. // verify that the write position is at offset 0x12d. // this is a check to make sure that https://github.com/bitcoin/bitcoin/issues/21379 does not recur // 8 bytes (for serialization header) + 285 (for serialized genesis block) = 293 // add another 8 bytes for the second block's serialization header and we get 293 + 8 = 301 - FlatFilePos actual{blockman.SaveBlockToDisk(params->GenesisBlock(), 1, chain, *params, nullptr)}; + FlatFilePos actual{blockman.SaveBlockToDisk(params->GenesisBlock(), 1, chain, nullptr)}; BOOST_CHECK_EQUAL(actual.nPos, BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(params->GenesisBlock(), CLIENT_VERSION) + BLOCK_SERIALIZATION_HEADER_SIZE); } diff --git a/src/validation.cpp b/src/validation.cpp index 0b46854902..a24211909d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2374,7 +2374,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, if (fJustCheck) return true; - if (!m_blockman.WriteUndoDataForBlock(blockundo, state, pindex, params)) { + if (!m_blockman.WriteUndoDataForBlock(blockundo, state, *pindex)) { return false; } @@ -4000,7 +4000,7 @@ bool Chainstate::AcceptBlock(const std::shared_ptr& pblock, BlockV // Write block to history file if (fNewBlock) *fNewBlock = true; try { - FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, pindex->nHeight, m_chain, params, dbp)}; + FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, pindex->nHeight, m_chain, dbp)}; if (blockPos.IsNull()) { state.Error(strprintf("%s: Failed to find position to write new block to disk", __func__)); return false; @@ -4418,7 +4418,7 @@ bool ChainstateManager::LoadBlockIndex() // Load block index from databases bool needs_init = fReindex; if (!fReindex) { - bool ret = m_blockman.LoadBlockIndexDB(GetConsensus()); + bool ret{m_blockman.LoadBlockIndexDB()}; if (!ret) return false; m_blockman.ScanAndUnlinkAlreadyPrunedFiles(); @@ -4522,7 +4522,7 @@ bool Chainstate::LoadGenesisBlock() try { const CBlock& block = params.GenesisBlock(); - FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, 0, m_chain, params, nullptr)}; + FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, 0, m_chain, nullptr)}; if (blockPos.IsNull()) { return error("%s: writing genesis block to disk failed", __func__); }