From e63f64307929ad398a23ecfaabc3664270883155 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 15 Nov 2023 13:07:14 +1000 Subject: [PATCH 1/5] streams: Base BufferedFile on AutoFile instead of CAutoFile --- src/streams.h | 8 +++----- src/test/fuzz/buffered_file.cpp | 4 +--- src/test/streams_tests.cpp | 3 --- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/streams.h b/src/streams.h index a3f8028da78..e7779d5bbdd 100644 --- a/src/streams.h +++ b/src/streams.h @@ -529,7 +529,7 @@ class CAutoFile : public AutoFile } }; -/** Wrapper around a CAutoFile& that implements a ring buffer to +/** Wrapper around an AutoFile& that implements a ring buffer to * deserialize from. It guarantees the ability to rewind a given number of bytes. * * Will automatically close the file when it goes out of scope if not null. @@ -538,7 +538,7 @@ class CAutoFile : public AutoFile class BufferedFile { private: - CAutoFile& m_src; + AutoFile& m_src; uint64_t nSrcPos{0}; //!< how many bytes have been read from source uint64_t m_read_pos{0}; //!< how many bytes have been read from this uint64_t nReadLimit; //!< up to which position we're allowed to read @@ -585,15 +585,13 @@ class BufferedFile } public: - BufferedFile(CAutoFile& file, uint64_t nBufSize, uint64_t nRewindIn) + BufferedFile(AutoFile& file, uint64_t nBufSize, uint64_t nRewindIn) : m_src{file}, nReadLimit{std::numeric_limits::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0}) { if (nRewindIn >= nBufSize) throw std::ios_base::failure("Rewind limit must be less than buffer size"); } - int GetVersion() const { return m_src.GetVersion(); } - //! check whether we're at the end of the source file bool eof() const { return m_read_pos == nSrcPos && m_src.feof(); diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index 813af63738b..e30c19b2659 100644 --- a/src/test/fuzz/buffered_file.cpp +++ b/src/test/fuzz/buffered_file.cpp @@ -20,9 +20,8 @@ FUZZ_TARGET(buffered_file) FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider}; std::optional opt_buffered_file; - CAutoFile fuzzed_file{ + AutoFile fuzzed_file{ fuzzed_file_provider.open(), - 0, ConsumeRandomLengthByteVector(fuzzed_data_provider), }; try { @@ -65,6 +64,5 @@ FUZZ_TARGET(buffered_file) }); } opt_buffered_file->GetPos(); - opt_buffered_file->GetVersion(); } } diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index f03f7c1da24..38fdc7a6e9b 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -271,9 +271,6 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) BufferedFile bf{file, 25, 10}; BOOST_CHECK(!bf.eof()); - // This member has no functional effect. - BOOST_CHECK_EQUAL(bf.GetVersion(), 333); - uint8_t i; bf >> i; BOOST_CHECK_EQUAL(i, 0); From c72ddf04db95a94e91939d46d13ab6a46fefb4be Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 17 Nov 2023 23:58:43 +1000 Subject: [PATCH 2/5] streams: Remove unused CAutoFile::GetVersion --- src/streams.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/streams.h b/src/streams.h index e7779d5bbdd..794a0dd6b77 100644 --- a/src/streams.h +++ b/src/streams.h @@ -507,12 +507,8 @@ class AutoFile class CAutoFile : public AutoFile { -private: - const int nVersion; - public: - explicit CAutoFile(std::FILE* file, int version, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)}, nVersion{version} {} - int GetVersion() const { return nVersion; } + explicit CAutoFile(std::FILE* file, int /*unused*/, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)} {} template CAutoFile& operator<<(const T& obj) From bbd4646a2ef514c31570ca9d1475fc9ddb35bdfd Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 15 Nov 2023 13:07:28 +1000 Subject: [PATCH 3/5] blockstorage: switch from CAutoFile to AutoFile Also bump includes per suggestions from iwyu. --- src/index/txindex.cpp | 2 +- src/node/blockstorage.cpp | 35 ++++++++++++++-------- src/node/blockstorage.h | 16 +++++----- src/test/fuzz/load_external_block_file.cpp | 2 +- src/validation.cpp | 2 +- src/validation.h | 2 +- 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 5d37fd0d8f6..4983926e686 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -79,7 +79,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe return false; } - CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)}; + AutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)}; if (file.IsNull()) { return error("%s: OpenBlockFile failed", __func__); } diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index c066d1c939f..ebc08d75679 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -4,23 +4,32 @@ #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 #include @@ -656,7 +665,7 @@ CBlockFileInfo* BlockManager::GetBlockFileInfo(size_t n) bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const { // Open history file to append - CAutoFile fileout{OpenUndoFile(pos)}; + AutoFile fileout{OpenUndoFile(pos)}; if (fileout.IsNull()) { return error("%s: OpenUndoFile failed", __func__); } @@ -691,7 +700,7 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in } // Open history file to read - CAutoFile filein{OpenUndoFile(pos, true)}; + AutoFile filein{OpenUndoFile(pos, true)}; if (filein.IsNull()) { return error("%s: OpenUndoFile failed", __func__); } @@ -810,15 +819,15 @@ FlatFileSeq BlockManager::UndoFileSeq() const return FlatFileSeq(m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE); } -CAutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const +AutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const { - return CAutoFile{BlockFileSeq().Open(pos, fReadOnly), CLIENT_VERSION}; + return AutoFile{BlockFileSeq().Open(pos, fReadOnly)}; } /** Open an undo file (rev?????.dat) */ -CAutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const +AutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const { - return CAutoFile{UndoFileSeq().Open(pos, fReadOnly), CLIENT_VERSION}; + return AutoFile{UndoFileSeq().Open(pos, fReadOnly)}; } fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const @@ -950,7 +959,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const { // Open history file to append - CAutoFile fileout{OpenBlockFile(pos)}; + AutoFile fileout{OpenBlockFile(pos)}; if (fileout.IsNull()) { return error("WriteBlockToDisk: OpenBlockFile failed"); } @@ -1016,7 +1025,7 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) cons block.SetNull(); // Open history file to read - CAutoFile filein{OpenBlockFile(pos, true)}; + AutoFile filein{OpenBlockFile(pos, true)}; if (filein.IsNull()) { return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString()); } @@ -1059,7 +1068,7 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector& block, const FlatF { FlatFilePos hpos = pos; hpos.nPos -= 8; // Seek back 8 bytes for meta header - CAutoFile filein{OpenBlockFile(hpos, true)}; + AutoFile filein{OpenBlockFile(hpos, true)}; if (filein.IsNull()) { return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString()); } @@ -1151,7 +1160,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) { break; // No block files left to reindex } - CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)}; + AutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)}; if (file.IsNull()) { break; // This error is logged in OpenBlockFile } @@ -1172,7 +1181,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile // -loadblock= for (const fs::path& path : vImportFiles) { - CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION}; + AutoFile file{fsbridge::fopen(path, "rb")}; if (!file.IsNull()) { LogPrintf("Importing blocks file %s...\n", fs::PathToString(path)); chainman.LoadExternalBlockFile(file); diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ba44d31581c..d540406ae5c 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -8,21 +8,26 @@ #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 @@ -30,14 +35,9 @@ #include class BlockValidationState; -class CAutoFile; -class CBlock; class CBlockUndo; -class CChainParams; class Chainstate; class ChainstateManager; -struct CCheckpointData; -struct FlatFilePos; namespace Consensus { struct Params; } @@ -162,7 +162,7 @@ class BlockManager FlatFileSeq BlockFileSeq() const; FlatFileSeq UndoFileSeq() const; - CAutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; + AutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const; bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const; @@ -350,7 +350,7 @@ class BlockManager void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Open a block file (blk?????.dat) */ - CAutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const; + AutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const; /** Translation to a filesystem path */ fs::path GetBlockPosFilename(const FlatFilePos& pos) const; diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index ae4f5d089b6..6460261f0f5 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -28,7 +28,7 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider}; - CAutoFile fuzzed_block_file{fuzzed_file_provider.open(), CLIENT_VERSION}; + AutoFile fuzzed_block_file{fuzzed_file_provider.open()}; if (fuzzed_block_file.IsNull()) { return; } diff --git a/src/validation.cpp b/src/validation.cpp index ed72a7c97a6..728a049f890 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4649,7 +4649,7 @@ bool Chainstate::LoadGenesisBlock() } void ChainstateManager::LoadExternalBlockFile( - CAutoFile& file_in, + AutoFile& file_in, FlatFilePos* dbp, std::multimap* blocks_with_unknown_parent) { diff --git a/src/validation.h b/src/validation.h index e669ec46d5d..7473bcbc3ba 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1137,7 +1137,7 @@ class ChainstateManager * (only used for reindex) * */ void LoadExternalBlockFile( - CAutoFile& file_in, + AutoFile& file_in, FlatFilePos* dbp = nullptr, std::multimap* blocks_with_unknown_parent = nullptr); From cde9a4b137e93f1548dffac9b396ca0b472b6187 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 15 Nov 2023 13:10:13 +1000 Subject: [PATCH 4/5] refactor: switch from CAutoFile to AutoFile --- src/bench/load_external.cpp | 2 +- src/bench/streams_findbyte.cpp | 2 +- src/test/streams_tests.cpp | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 3b100d97b0b..fba12339019 100644 --- a/src/bench/load_external.cpp +++ b/src/bench/load_external.cpp @@ -55,7 +55,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench) bench.run([&] { // "rb" is "binary, O_RDONLY", positioned to the start of the file. // The file will be closed by LoadExternalBlockFile(). - CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION}; + AutoFile file{fsbridge::fopen(blkfile, "rb")}; testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); }); fs::remove(blkfile); diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index 4aaccb2af84..5098262e9af 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -14,7 +14,7 @@ static void FindByte(benchmark::Bench& bench) { // Setup - CAutoFile file{fsbridge::fopen("streams_tmp", "w+b"), 0}; + AutoFile file{fsbridge::fopen("streams_tmp", "w+b")}; const size_t file_size = 200; uint8_t data[file_size] = {0}; data[file_size-1] = 1; diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 38fdc7a6e9b..d8478ecf5f9 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -249,7 +249,7 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor) BOOST_AUTO_TEST_CASE(streams_buffered_file) { fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; - CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; + AutoFile file{fsbridge::fopen(streams_test_filename, "w+b")}; // The value at each offset is the offset. for (uint8_t j = 0; j < 40; ++j) { @@ -380,7 +380,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) { fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; - CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; + AutoFile file{fsbridge::fopen(streams_test_filename, "w+b")}; // The value at each offset is the byte offset (e.g. byte 1 in the file has the value 0x01). for (uint8_t j = 0; j < 40; ++j) { file << j; @@ -433,7 +433,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; for (int rep = 0; rep < 50; ++rep) { - CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; + AutoFile file{fsbridge::fopen(streams_test_filename, "w+b")}; size_t fileSize = InsecureRandRange(256); for (uint8_t i = 0; i < fileSize; ++i) { file << i; From 4eb2a9ea4b6262bec0bc7c20cb3e684ea75caf42 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 17 Nov 2023 23:57:35 +1000 Subject: [PATCH 5/5] streams: Drop unused CAutoFile --- src/streams.h | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/streams.h b/src/streams.h index 794a0dd6b77..068e2e8efd5 100644 --- a/src/streams.h +++ b/src/streams.h @@ -505,26 +505,6 @@ class AutoFile } }; -class CAutoFile : public AutoFile -{ -public: - explicit CAutoFile(std::FILE* file, int /*unused*/, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)} {} - - template - CAutoFile& operator<<(const T& obj) - { - ::Serialize(*this, obj); - return (*this); - } - - template - CAutoFile& operator>>(T&& obj) - { - ::Unserialize(*this, obj); - return (*this); - } -}; - /** Wrapper around an AutoFile& that implements a ring buffer to * deserialize from. It guarantees the ability to rewind a given number of bytes. *