From 0d114e3cb20cb9e03fc9ba8daf3d03436b491742 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 20 Mar 2024 15:08:40 -0400 Subject: blockstorage: Add Assume for fKnown / snapshot chainstate fKnown is true during reindex (and only then), which deletes any existing snapshot chainstate. As a result, this function can never be called wth fKnown set and a snapshot chainstate. Add an Assume for this, and make the code initializing a blockfile cursor for the snapshot conditional on !fKnown. This is a preparation for splitting the reindex logic out of FindBlockPos in the following commits. --- src/node/blockstorage.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 576c07a833..9b8467c2f4 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -853,8 +853,16 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne LOCK(cs_LastBlockFile); const BlockfileType chain_type = BlockfileTypeForHeight(nHeight); + // Check that chain type is NORMAL if fKnown is true, because fKnown is only + // true during reindexing, and reindexing deletes snapshot chainstates, so + // chain_type will not be SNAPSHOT. Also check that cursor exists, because + // the normal cursor should never be null. + if (fKnown) { + Assume(chain_type == BlockfileType::NORMAL); + Assume(m_blockfile_cursors[chain_type]); + } - if (!m_blockfile_cursors[chain_type]) { + if (!fKnown && !m_blockfile_cursors[chain_type]) { // If a snapshot is loaded during runtime, we may not have initialized this cursor yet. assert(chain_type == BlockfileType::ASSUMED); const auto new_cursor = BlockfileCursor{this->MaxBlockfileNum() + 1}; -- cgit v1.2.3 From fdae638e83522c28a1222e65c43d1cbca3e34cba Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 10 May 2024 16:46:35 -0400 Subject: doc: Improve doc for functions involved in saving blocks to disk In particular, document the flat file positions expected and returned by functions better. Co-authored-by: Ryan Ofsky --- src/node/blockstorage.h | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ce514cc645..27b7737051 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -155,6 +155,16 @@ private: /** Return false if undo file flushing fails. */ [[nodiscard]] bool FlushUndoFile(int block_file, bool finalize = false); + /** + * Helper function performing various preparations before a block can be saved to disk: + * Returns the correct position for the block to be saved, which may be in the current or a new + * block file depending on nAddSize. May flush the previous blockfile to disk if full, updates + * blockfile info, and checks if there is enough disk space to save the block. + * + * If fKnown is false, the nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of + * separator fields which are written before it by WriteBlockToDisk (BLOCK_SERIALIZATION_HEADER_SIZE). + * If fKnown is true, nAddSize should be just the size of the serialized CBlock. + */ [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown); [[nodiscard]] bool FlushChainstateBlockFile(int tip_height); bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize); @@ -164,6 +174,12 @@ private: AutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; + /** + * Write a block to disk. The pos argument passed to this function is modified by this call. Before this call, it should + * point to an unused file location where separator fields will be written, followed by the serialized CBlock data. + * After this call, it will point to the beginning of the serialized CBlock data, after the separator fields + * (BLOCK_SERIALIZATION_HEADER_SIZE) + */ bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const; bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const; @@ -312,7 +328,16 @@ public: 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. */ + /** Store block on disk and update block file statistics. + * If dbp is non-null, it means the block data is already stored, and dbp contains the file position. + * In this case, the block data will not be written, only the block file statistics will be updated. This case should only happen during reindexing. + * + * @param[in] block the block to be stored + * @param[in] nHeight the height of the block + * + * @returns in case of success, the position to which the block was written to + * in case of an error, an empty FlatFilePos + */ FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp); /** Whether running in -prune mode. */ -- cgit v1.2.3 From 064859bbad6984a6ec85c744064abdf757807c58 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 20 Mar 2024 15:05:08 -0400 Subject: blockstorage: split up FindBlockPos function FindBlockPos does different things depending on whether the block is known or not, as shown by the fact that much of the existing code is conditional on fKnown set or not. If the block position is known (during reindex) the function only updates the block info statistics. It doesn't actually find a block position in this case. This commit removes fKnown and splits up these two code paths by introducing a separate function for the reindex case when the block position is known. It doesn't change behavior. --- src/node/blockstorage.cpp | 163 ++++++++++++++++++++++++---------------------- src/node/blockstorage.h | 14 +++- 2 files changed, 97 insertions(+), 80 deletions(-) (limited to 'src') diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 9b8467c2f4..0fe989d722 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -848,21 +848,13 @@ fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const return BlockFileSeq().FileName(pos); } -bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown) +bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) { LOCK(cs_LastBlockFile); const BlockfileType chain_type = BlockfileTypeForHeight(nHeight); - // Check that chain type is NORMAL if fKnown is true, because fKnown is only - // true during reindexing, and reindexing deletes snapshot chainstates, so - // chain_type will not be SNAPSHOT. Also check that cursor exists, because - // the normal cursor should never be null. - if (fKnown) { - Assume(chain_type == BlockfileType::NORMAL); - Assume(m_blockfile_cursors[chain_type]); - } - if (!fKnown && !m_blockfile_cursors[chain_type]) { + if (!m_blockfile_cursors[chain_type]) { // If a snapshot is loaded during runtime, we may not have initialized this cursor yet. assert(chain_type == BlockfileType::ASSUMED); const auto new_cursor = BlockfileCursor{this->MaxBlockfileNum() + 1}; @@ -871,90 +863,107 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne } const int last_blockfile = m_blockfile_cursors[chain_type]->file_num; - int nFile = fKnown ? pos.nFile : last_blockfile; + int nFile = last_blockfile; if (static_cast(m_blockfile_info.size()) <= nFile) { m_blockfile_info.resize(nFile + 1); } bool finalize_undo = false; - if (!fKnown) { - unsigned int max_blockfile_size{MAX_BLOCKFILE_SIZE}; - // Use smaller blockfiles in test-only -fastprune mode - but avoid - // the possibility of having a block not fit into the block file. - if (m_opts.fast_prune) { - max_blockfile_size = 0x10000; // 64kiB - if (nAddSize >= max_blockfile_size) { - // dynamically adjust the blockfile size to be larger than the added size - max_blockfile_size = nAddSize + 1; - } + unsigned int max_blockfile_size{MAX_BLOCKFILE_SIZE}; + // Use smaller blockfiles in test-only -fastprune mode - but avoid + // the possibility of having a block not fit into the block file. + if (m_opts.fast_prune) { + max_blockfile_size = 0x10000; // 64kiB + if (nAddSize >= max_blockfile_size) { + // dynamically adjust the blockfile size to be larger than the added size + max_blockfile_size = nAddSize + 1; } - assert(nAddSize < max_blockfile_size); - - while (m_blockfile_info[nFile].nSize + nAddSize >= max_blockfile_size) { - // when the undo file is keeping up with the block file, we want to flush it explicitly - // when it is lagging behind (more blocks arrive than are being connected), we let the - // undo block write case handle it - finalize_undo = (static_cast(m_blockfile_info[nFile].nHeightLast) == - Assert(m_blockfile_cursors[chain_type])->undo_height); - - // Try the next unclaimed blockfile number - nFile = this->MaxBlockfileNum() + 1; - // Set to increment MaxBlockfileNum() for next iteration - m_blockfile_cursors[chain_type] = BlockfileCursor{nFile}; - - if (static_cast(m_blockfile_info.size()) <= nFile) { - m_blockfile_info.resize(nFile + 1); - } + } + assert(nAddSize < max_blockfile_size); + + while (m_blockfile_info[nFile].nSize + nAddSize >= max_blockfile_size) { + // when the undo file is keeping up with the block file, we want to flush it explicitly + // when it is lagging behind (more blocks arrive than are being connected), we let the + // undo block write case handle it + finalize_undo = (static_cast(m_blockfile_info[nFile].nHeightLast) == + Assert(m_blockfile_cursors[chain_type])->undo_height); + + // Try the next unclaimed blockfile number + nFile = this->MaxBlockfileNum() + 1; + // Set to increment MaxBlockfileNum() for next iteration + m_blockfile_cursors[chain_type] = BlockfileCursor{nFile}; + + if (static_cast(m_blockfile_info.size()) <= nFile) { + m_blockfile_info.resize(nFile + 1); } - pos.nFile = nFile; - pos.nPos = m_blockfile_info[nFile].nSize; } + pos.nFile = nFile; + pos.nPos = m_blockfile_info[nFile].nSize; if (nFile != last_blockfile) { - if (!fKnown) { - LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s (onto %i) (height %i)\n", - last_blockfile, m_blockfile_info[last_blockfile].ToString(), nFile, nHeight); - - // Do not propagate the return code. The flush concerns a previous block - // and undo file that has already been written to. If a flush fails - // here, and we crash, there is no expected additional block data - // inconsistency arising from the flush failure here. However, the undo - // data may be inconsistent after a crash if the flush is called during - // a reindex. A flush error might also leave some of the data files - // untrimmed. - if (!FlushBlockFile(last_blockfile, !fKnown, finalize_undo)) { - LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, - "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", - last_blockfile, !fKnown, finalize_undo, nFile); - } + LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s (onto %i) (height %i)\n", + last_blockfile, m_blockfile_info[last_blockfile].ToString(), nFile, nHeight); + + // Do not propagate the return code. The flush concerns a previous block + // and undo file that has already been written to. If a flush fails + // here, and we crash, there is no expected additional block data + // inconsistency arising from the flush failure here. However, the undo + // data may be inconsistent after a crash if the flush is called during + // a reindex. A flush error might also leave some of the data files + // untrimmed. + if (!FlushBlockFile(last_blockfile, /*fFinalize=*/true, finalize_undo)) { + LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, + "Failed to flush previous block file %05i (finalize=1, finalize_undo=%i) before opening new block file %05i\n", + last_blockfile, finalize_undo, nFile); } // No undo data yet in the new file, so reset our undo-height tracking. m_blockfile_cursors[chain_type] = BlockfileCursor{nFile}; } m_blockfile_info[nFile].AddBlock(nHeight, nTime); - if (fKnown) { - m_blockfile_info[nFile].nSize = std::max(pos.nPos + nAddSize, m_blockfile_info[nFile].nSize); - } else { - m_blockfile_info[nFile].nSize += nAddSize; - } + m_blockfile_info[nFile].nSize += nAddSize; - if (!fKnown) { - bool out_of_space; - size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space); - if (out_of_space) { - m_opts.notifications.fatalError(_("Disk space is too low!")); - return false; - } - if (bytes_allocated != 0 && IsPruneMode()) { - m_check_for_pruning = true; - } + bool out_of_space; + size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space); + if (out_of_space) { + m_opts.notifications.fatalError(_("Disk space is too low!")); + return false; + } + if (bytes_allocated != 0 && IsPruneMode()) { + m_check_for_pruning = true; } m_dirty_fileinfo.insert(nFile); return true; } +void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos) +{ + LOCK(cs_LastBlockFile); + + const unsigned int added_size = ::GetSerializeSize(TX_WITH_WITNESS(block)); + const BlockfileType chain_type = BlockfileTypeForHeight(nHeight); + // Check that chain type is NORMAL, because this function is only + // called during reindexing, and reindexing deletes snapshot chainstates, so + // chain_type will not be SNAPSHOT. Also check that cursor exists, because + // the normal cursor should never be null. + Assume(chain_type == BlockfileType::NORMAL); + Assume(m_blockfile_cursors[chain_type]); + const int nFile = pos.nFile; + if (static_cast(m_blockfile_info.size()) <= nFile) { + m_blockfile_info.resize(nFile + 1); + } + + const int last_blockfile = m_blockfile_cursors[chain_type]->file_num; + if (nFile != last_blockfile) { + // No undo data yet in the new file, so reset our undo-height tracking. + m_blockfile_cursors[chain_type] = BlockfileCursor{nFile}; + } + m_blockfile_info[nFile].AddBlock(nHeight, block.GetBlockTime()); + m_blockfile_info[nFile].nSize = std::max(pos.nPos + added_size, m_blockfile_info[nFile].nSize); + m_dirty_fileinfo.insert(nFile); +} + bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize) { pos.nFile = nFile; @@ -1145,17 +1154,17 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, cons const auto position_known {dbp != nullptr}; if (position_known) { blockPos = *dbp; + // position_known is set iff performing a reindex. In this case, no blocks need to be written, only the blockfile info database needs to be rebuilt. + UpdateBlockInfo(block, nHeight, *dbp); } else { // when known, blockPos.nPos points at the offset of the block data in the blk file. that already accounts for // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes = BLOCK_SERIALIZATION_HEADER_SIZE). // we add BLOCK_SERIALIZATION_HEADER_SIZE only for new blocks since they will have the serialization header added when written to disk. nBlockSize += static_cast(BLOCK_SERIALIZATION_HEADER_SIZE); - } - if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime(), position_known)) { - LogError("%s: FindBlockPos failed\n", __func__); - return FlatFilePos(); - } - if (!position_known) { + if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime())) { + LogError("%s: FindBlockPos failed\n", __func__); + return FlatFilePos(); + } if (!WriteBlockToDisk(block, blockPos)) { m_opts.notifications.fatalError(_("Failed to write block.")); return FlatFilePos(); diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 27b7737051..ff77a66b3b 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -161,11 +161,10 @@ private: * block file depending on nAddSize. May flush the previous blockfile to disk if full, updates * blockfile info, and checks if there is enough disk space to save the block. * - * If fKnown is false, the nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of + * The nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of * separator fields which are written before it by WriteBlockToDisk (BLOCK_SERIALIZATION_HEADER_SIZE). - * If fKnown is true, nAddSize should be just the size of the serialized CBlock. */ - [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown); + [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime); [[nodiscard]] bool FlushChainstateBlockFile(int tip_height); bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize); @@ -340,6 +339,15 @@ public: */ FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp); + /** Update blockfile info while processing a block during reindex. The block must be available on disk. + * + * @param[in] block the block being processed + * @param[in] nHeight the height of the block + * @param[in] pos the position of the serialized CBlock on disk. This is the position returned + * by WriteBlockToDisk pointing at the CBlock, not the separator fields before it + */ + void UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos); + /** Whether running in -prune mode. */ [[nodiscard]] bool IsPruneMode() const { return m_prune_mode; } -- cgit v1.2.3 From d9e477c4dc39d9623ed66c35c06e28f94ae62ad5 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 26 Apr 2024 15:06:55 -0400 Subject: validation, blockstorage: Separate code paths for reindex and saving new blocks By calling SaveBlockToDisk only when we actually want to save a new block to disk. In the reindex case, we now call UpdateBlockInfo directly from validation. This commit doesn't change behavior. --- src/bench/readblock.cpp | 2 +- src/node/blockstorage.cpp | 30 +++++++++++------------------- src/node/blockstorage.h | 4 +--- src/test/blockmanager_tests.cpp | 24 ++++++++++-------------- src/validation.cpp | 16 +++++++++++----- 5 files changed, 34 insertions(+), 42 deletions(-) (limited to 'src') diff --git a/src/bench/readblock.cpp b/src/bench/readblock.cpp index 0545c6b017..2b2bfe069e 100644 --- a/src/bench/readblock.cpp +++ b/src/bench/readblock.cpp @@ -18,7 +18,7 @@ static FlatFilePos WriteBlockToDisk(ChainstateManager& chainman) CBlock block; stream >> TX_WITH_WITNESS(block); - return chainman.m_blockman.SaveBlockToDisk(block, 0, nullptr); + return chainman.m_blockman.SaveBlockToDisk(block, 0); } static void ReadBlockFromDiskTest(benchmark::Bench& bench) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 0fe989d722..9d8c4701ef 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1147,28 +1147,20 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector& block, const FlatF return true; } -FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp) +FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight) { unsigned int nBlockSize = ::GetSerializeSize(TX_WITH_WITNESS(block)); FlatFilePos blockPos; - const auto position_known {dbp != nullptr}; - if (position_known) { - blockPos = *dbp; - // position_known is set iff performing a reindex. In this case, no blocks need to be written, only the blockfile info database needs to be rebuilt. - UpdateBlockInfo(block, nHeight, *dbp); - } else { - // when known, blockPos.nPos points at the offset of the block data in the blk file. that already accounts for - // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes = BLOCK_SERIALIZATION_HEADER_SIZE). - // we add BLOCK_SERIALIZATION_HEADER_SIZE only for new blocks since they will have the serialization header added when written to disk. - nBlockSize += static_cast(BLOCK_SERIALIZATION_HEADER_SIZE); - if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime())) { - LogError("%s: FindBlockPos failed\n", __func__); - return FlatFilePos(); - } - if (!WriteBlockToDisk(block, blockPos)) { - m_opts.notifications.fatalError(_("Failed to write block.")); - return FlatFilePos(); - } + // Account for the 4 magic message start bytes + the 4 length bytes (8 bytes total, + // defined as BLOCK_SERIALIZATION_HEADER_SIZE) + nBlockSize += static_cast(BLOCK_SERIALIZATION_HEADER_SIZE); + if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime())) { + LogError("%s: FindBlockPos failed\n", __func__); + return FlatFilePos(); + } + if (!WriteBlockToDisk(block, blockPos)) { + m_opts.notifications.fatalError(_("Failed to write block.")); + return FlatFilePos(); } return blockPos; } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ff77a66b3b..ae47478779 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -328,8 +328,6 @@ public: EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Store block on disk and update block file statistics. - * If dbp is non-null, it means the block data is already stored, and dbp contains the file position. - * In this case, the block data will not be written, only the block file statistics will be updated. This case should only happen during reindexing. * * @param[in] block the block to be stored * @param[in] nHeight the height of the block @@ -337,7 +335,7 @@ public: * @returns in case of success, the position to which the block was written to * in case of an error, an empty FlatFilePos */ - FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, const FlatFilePos* dbp); + FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight); /** Update blockfile info while processing a block during reindex. The block must be available on disk. * diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index d7ac0bf823..efe0983698 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -35,20 +35,20 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) }; BlockManager blockman{*Assert(m_node.shutdown), blockman_opts}; // simulate adding a genesis block normally - BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); + BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0).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, &pos).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); + const FlatFilePos pos{0, BLOCK_SERIALIZATION_HEADER_SIZE}; + blockman.UpdateBlockInfo(params->GenesisBlock(), 0, pos); // 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, nullptr)}; + FlatFilePos actual{blockman.SaveBlockToDisk(params->GenesisBlock(), 1)}; BOOST_CHECK_EQUAL(actual.nPos, BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(TX_WITH_WITNESS(params->GenesisBlock())) + BLOCK_SERIALIZATION_HEADER_SIZE); } @@ -156,12 +156,11 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) // Blockstore is empty BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), 0); - // Write the first block; dbp=nullptr means this block doesn't already have a disk - // location, so allocate a free location and write it there. - FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, /*dbp=*/nullptr)}; + // Write the first block to a new location. + FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1)}; // Write second block - FlatFilePos pos2{blockman.SaveBlockToDisk(block2, /*nHeight=*/2, /*dbp=*/nullptr)}; + FlatFilePos pos2{blockman.SaveBlockToDisk(block2, /*nHeight=*/2)}; // Two blocks in the file BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2); @@ -181,22 +180,19 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) BOOST_CHECK_EQUAL(read_block.nVersion, 2); } - // When FlatFilePos* dbp is given, SaveBlockToDisk() will not write or - // overwrite anything to the flat file block storage. It will, however, - // update the blockfile metadata. This is to facilitate reindexing - // when the user has the blocks on disk but the metadata is being rebuilt. + // During reindex, the flat file block storage will not be written to. + // UpdateBlockInfo will, however, update the blockfile metadata. // Verify this behavior by attempting (and failing) to write block 3 data // to block 2 location. CBlockFileInfo* block_data = blockman.GetBlockFileInfo(0); BOOST_CHECK_EQUAL(block_data->nBlocks, 2); - BOOST_CHECK(blockman.SaveBlockToDisk(block3, /*nHeight=*/3, /*dbp=*/&pos2) == pos2); + blockman.UpdateBlockInfo(block3, /*nHeight=*/3, /*pos=*/pos2); // Metadata is updated... BOOST_CHECK_EQUAL(block_data->nBlocks, 3); // ...but there are still only two blocks in the file BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2); // Block 2 was not overwritten: - // SaveBlockToDisk() did not call WriteBlockToDisk() because `FlatFilePos* dbp` was non-null blockman.ReadBlockFromDisk(read_block, pos2); BOOST_CHECK_EQUAL(read_block.nVersion, 2); } diff --git a/src/validation.cpp b/src/validation.cpp index 903f9caf13..2bdffdb1b0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4342,10 +4342,16 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, // Write block to history file if (fNewBlock) *fNewBlock = true; try { - FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, pindex->nHeight, dbp)}; - if (blockPos.IsNull()) { - state.Error(strprintf("%s: Failed to find position to write new block to disk", __func__)); - return false; + FlatFilePos blockPos{}; + if (dbp) { + blockPos = *dbp; + m_blockman.UpdateBlockInfo(block, pindex->nHeight, blockPos); + } else { + blockPos = m_blockman.SaveBlockToDisk(block, pindex->nHeight); + if (blockPos.IsNull()) { + state.Error(strprintf("%s: Failed to find position to write new block to disk", __func__)); + return false; + } } ReceivedBlockTransactions(block, pindex, blockPos); } catch (const std::runtime_error& e) { @@ -4845,7 +4851,7 @@ bool Chainstate::LoadGenesisBlock() try { const CBlock& block = params.GenesisBlock(); - FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, 0, nullptr)}; + FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, 0)}; if (blockPos.IsNull()) { LogError("%s: writing genesis block to disk failed\n", __func__); return false; -- cgit v1.2.3 From 17103637c6fa2dfcf5374ebb0cd715e540dd4ce1 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 10 May 2024 15:08:55 -0400 Subject: blockstorage: Rename FindBlockPos and have it return a FlatFilePos The new name reflects that it is no longer called with existing blocks for which the position is already known. Returning a FlatFilePos directly simplifies the interface. --- src/node/blockstorage.cpp | 15 ++++++++------- src/node/blockstorage.h | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 9d8c4701ef..aab6a9e7fb 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -848,7 +848,7 @@ fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const return BlockFileSeq().FileName(pos); } -bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) +FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime) { LOCK(cs_LastBlockFile); @@ -897,6 +897,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne m_blockfile_info.resize(nFile + 1); } } + FlatFilePos pos; pos.nFile = nFile; pos.nPos = m_blockfile_info[nFile].nSize; @@ -927,14 +928,14 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space); if (out_of_space) { m_opts.notifications.fatalError(_("Disk space is too low!")); - return false; + return {}; } if (bytes_allocated != 0 && IsPruneMode()) { m_check_for_pruning = true; } m_dirty_fileinfo.insert(nFile); - return true; + return pos; } void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, const FlatFilePos& pos) @@ -1031,7 +1032,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid // we want to flush the rev (undo) file once we've written the last block, which is indicated by the last height // 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 + // the FindNextBlockPos function if (_pos.nFile < cursor.file_num && static_cast(block.nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) { // Do not propagate the return code, a failed flush here should not // be an indication for a failed write. If it were propagated here, @@ -1150,12 +1151,12 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector& block, const FlatF FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight) { unsigned int nBlockSize = ::GetSerializeSize(TX_WITH_WITNESS(block)); - FlatFilePos blockPos; // Account for the 4 magic message start bytes + the 4 length bytes (8 bytes total, // defined as BLOCK_SERIALIZATION_HEADER_SIZE) nBlockSize += static_cast(BLOCK_SERIALIZATION_HEADER_SIZE); - if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime())) { - LogError("%s: FindBlockPos failed\n", __func__); + FlatFilePos blockPos{FindNextBlockPos(nBlockSize, nHeight, block.GetBlockTime())}; + if (blockPos.IsNull()) { + LogError("%s: FindNextBlockPos failed\n", __func__); return FlatFilePos(); } if (!WriteBlockToDisk(block, blockPos)) { diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ae47478779..eb2ed14747 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -164,7 +164,7 @@ private: * The nAddSize argument passed to this function should include not just the size of the serialized CBlock, but also the size of * separator fields which are written before it by WriteBlockToDisk (BLOCK_SERIALIZATION_HEADER_SIZE). */ - [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime); + [[nodiscard]] FlatFilePos FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime); [[nodiscard]] bool FlushChainstateBlockFile(int tip_height); bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize); @@ -221,7 +221,7 @@ private: //! effectively. //! //! This data structure maintains separate blockfile number cursors for each - //! BlockfileType. The ASSUMED state is initialized, when necessary, in FindBlockPos(). + //! BlockfileType. The ASSUMED state is initialized, when necessary, in FindNextBlockPos(). //! //! The first element is the NORMAL cursor, second is ASSUMED. std::array, BlockfileType::NUM_TYPES> -- cgit v1.2.3 From e41667b720372dae8438ea86e9819027e62b54e0 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 9 May 2024 18:34:14 -0400 Subject: blockstorage: Don't move cursor backwards in UpdateBlockInfo Previously, it was possible to move the cursor back to an older file during reindex if blocks are enocuntered out of order during reindex. This would mean that MaxBlockfileNum() would be incorrect, and a wrong DB_LAST_BLOCK could be written to disk. This improves the logic by only ever moving the cursor forward (if possible) but not backwards. Co-authored-by: Martin Zumsande --- src/node/blockstorage.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index aab6a9e7fb..2c0cbe7163 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -942,24 +942,19 @@ void BlockManager::UpdateBlockInfo(const CBlock& block, unsigned int nHeight, co { LOCK(cs_LastBlockFile); + // Update the cursor so it points to the last file. + const BlockfileType chain_type{BlockfileTypeForHeight(nHeight)}; + auto& cursor{m_blockfile_cursors[chain_type]}; + if (!cursor || cursor->file_num < pos.nFile) { + m_blockfile_cursors[chain_type] = BlockfileCursor{pos.nFile}; + } + + // Update the file information with the current block. const unsigned int added_size = ::GetSerializeSize(TX_WITH_WITNESS(block)); - const BlockfileType chain_type = BlockfileTypeForHeight(nHeight); - // Check that chain type is NORMAL, because this function is only - // called during reindexing, and reindexing deletes snapshot chainstates, so - // chain_type will not be SNAPSHOT. Also check that cursor exists, because - // the normal cursor should never be null. - Assume(chain_type == BlockfileType::NORMAL); - Assume(m_blockfile_cursors[chain_type]); const int nFile = pos.nFile; if (static_cast(m_blockfile_info.size()) <= nFile) { m_blockfile_info.resize(nFile + 1); } - - const int last_blockfile = m_blockfile_cursors[chain_type]->file_num; - if (nFile != last_blockfile) { - // No undo data yet in the new file, so reset our undo-height tracking. - m_blockfile_cursors[chain_type] = BlockfileCursor{nFile}; - } m_blockfile_info[nFile].AddBlock(nHeight, block.GetBlockTime()); m_blockfile_info[nFile].nSize = std::max(pos.nPos + added_size, m_blockfile_info[nFile].nSize); m_dirty_fileinfo.insert(nFile); -- cgit v1.2.3