From 5c2b3cd4b856f1bb536daaf7f576b1b1b42293ca Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sat, 9 Sep 2023 13:30:50 +0200 Subject: [PATCH 1/3] dbwrapper: Use DataStream for batch operations --- src/dbwrapper.cpp | 9 ++++----- src/dbwrapper.h | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index c95937ba75c..775496e21bd 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -4,7 +4,6 @@ #include -#include #include #include #include @@ -156,9 +155,9 @@ struct CDBBatch::WriteBatchImpl { leveldb::WriteBatch batch; }; -CDBBatch::CDBBatch(const CDBWrapper& _parent) : parent(_parent), - m_impl_batch{std::make_unique()}, - ssValue(SER_DISK, CLIENT_VERSION){}; +CDBBatch::CDBBatch(const CDBWrapper& _parent) + : parent{_parent}, + m_impl_batch{std::make_unique()} {}; CDBBatch::~CDBBatch() = default; @@ -168,7 +167,7 @@ void CDBBatch::Clear() size_estimate = 0; } -void CDBBatch::WriteImpl(Span key, CDataStream& ssValue) +void CDBBatch::WriteImpl(Span key, DataStream& ssValue) { leveldb::Slice slKey(CharCast(key.data()), key.size()); ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent)); diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 2f7448e8780..63c2f99d2a8 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -80,11 +80,11 @@ private: const std::unique_ptr m_impl_batch; DataStream ssKey{}; - CDataStream ssValue; + DataStream ssValue{}; size_t size_estimate{0}; - void WriteImpl(Span key, CDataStream& ssValue); + void WriteImpl(Span key, DataStream& ssValue); void EraseImpl(Span key); public: From fa2f2413b87f5fc1e5c92bf510beebdcd0091714 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 11 Sep 2023 17:30:31 +0200 Subject: [PATCH 2/3] Remove unused GetType() from CBufferedFile and CAutoFile GetType() is only called in tests, so it is unused and can be removed. --- src/bench/streams_findbyte.cpp | 2 +- src/index/txindex.cpp | 2 +- src/kernel/mempool_persist.cpp | 4 ++-- src/node/blockstorage.cpp | 4 ++-- src/streams.h | 10 +++------- src/test/fuzz/buffered_file.cpp | 3 +-- src/test/streams_tests.cpp | 11 +++++------ src/validation.cpp | 2 +- 8 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index 77f5940926e..451b5f758dc 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -16,7 +16,7 @@ static void FindByte(benchmark::Bench& bench) data[file_size-1] = 1; fwrite(&data, sizeof(uint8_t), file_size, file); rewind(file); - CBufferedFile bf(file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0, 0); + CBufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; bench.run([&] { bf.SetPos(0); diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 3c9ec84e242..6b38e19d813 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), SER_DISK, CLIENT_VERSION); + CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true), CLIENT_VERSION}; if (file.IsNull()) { return error("%s: OpenBlockFile failed", __func__); } diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp index 6be07da222a..ff655c5ffae 100644 --- a/src/kernel/mempool_persist.cpp +++ b/src/kernel/mempool_persist.cpp @@ -41,7 +41,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active if (load_path.empty()) return false; FILE* filestr{opts.mockable_fopen_function(load_path, "rb")}; - CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); + CAutoFile file{filestr, CLIENT_VERSION}; if (file.IsNull()) { LogPrintf("Failed to open mempool file from disk. Continuing anyway.\n"); return false; @@ -157,7 +157,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock return false; } - CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); + CAutoFile file{filestr, CLIENT_VERSION}; uint64_t version = MEMPOOL_DUMP_VERSION; file << version; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 70f11be5861..f43c5cbec59 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -822,7 +822,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), SER_DISK, CLIENT_VERSION); + CAutoFile fileout{OpenBlockFile(pos), CLIENT_VERSION}; if (fileout.IsNull()) { return error("WriteBlockToDisk: OpenBlockFile failed"); } @@ -878,7 +878,7 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) cons block.SetNull(); // Open history file to read - CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION); + CAutoFile filein{OpenBlockFile(pos, true), CLIENT_VERSION}; if (filein.IsNull()) { return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString()); } diff --git a/src/streams.h b/src/streams.h index 5ff952be76d..eec9c0956ba 100644 --- a/src/streams.h +++ b/src/streams.h @@ -550,12 +550,10 @@ public: class CAutoFile : public AutoFile { private: - const int nType; const int nVersion; public: - explicit CAutoFile(std::FILE* file, int type, int version, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)}, nType{type}, nVersion{version} {} - int GetType() const { return nType; } + explicit CAutoFile(std::FILE* file, int version, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)}, nVersion{version} {} int GetVersion() const { return nVersion; } template @@ -582,7 +580,6 @@ public: class CBufferedFile { private: - const int nType; const int nVersion; FILE *src; //!< source file @@ -632,8 +629,8 @@ private: } public: - CBufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nTypeIn, int nVersionIn) - : nType(nTypeIn), nVersion(nVersionIn), nReadLimit(std::numeric_limits::max()), nRewind(nRewindIn), vchBuf(nBufSize, std::byte{0}) + CBufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn) + : nVersion{nVersionIn}, 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"); @@ -650,7 +647,6 @@ public: CBufferedFile& operator=(const CBufferedFile&) = delete; int GetVersion() const { return nVersion; } - int GetType() const { return nType; } void fclose() { diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index 2f7ce60c7f0..47aa7e16603 100644 --- a/src/test/fuzz/buffered_file.cpp +++ b/src/test/fuzz/buffered_file.cpp @@ -21,7 +21,7 @@ FUZZ_TARGET(buffered_file) std::optional opt_buffered_file; FILE* fuzzed_file = fuzzed_file_provider.open(); try { - opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()); + opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegral()); } catch (const std::ios_base::failure&) { if (fuzzed_file != nullptr) { fclose(fuzzed_file); @@ -62,7 +62,6 @@ FUZZ_TARGET(buffered_file) }); } opt_buffered_file->GetPos(); - opt_buffered_file->GetType(); opt_buffered_file->GetVersion(); } } diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 589a2fd7666..02add89ab9a 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) // The buffer size (second arg) must be greater than the rewind // amount (third arg). try { - CBufferedFile bfbad(file, 25, 25, 222, 333); + CBufferedFile bfbad{file, 25, 25, 333}; BOOST_CHECK(false); } catch (const std::exception& e) { BOOST_CHECK(strstr(e.what(), @@ -268,11 +268,10 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) } // The buffer is 25 bytes, allow rewinding 10 bytes. - CBufferedFile bf(file, 25, 10, 222, 333); + CBufferedFile bf{file, 25, 10, 333}; BOOST_CHECK(!bf.eof()); - // These two members have no functional effect. - BOOST_CHECK_EQUAL(bf.GetType(), 222); + // This member has no functional effect. BOOST_CHECK_EQUAL(bf.GetVersion(), 333); uint8_t i; @@ -392,7 +391,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) rewind(file); // The buffer is 25 bytes, allow rewinding 10 bytes. - CBufferedFile bf(file, 25, 10, 222, 333); + CBufferedFile bf{file, 25, 10, 333}; uint8_t i; // This is like bf >> (7-byte-variable), in that it will cause data @@ -446,7 +445,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) size_t bufSize = InsecureRandRange(300) + 1; size_t rewindSize = InsecureRandRange(bufSize); - CBufferedFile bf(file, bufSize, rewindSize, 222, 333); + CBufferedFile bf{file, bufSize, rewindSize, 333}; size_t currentPos = 0; size_t maxPos = 0; for (int step = 0; step < 100; ++step) { diff --git a/src/validation.cpp b/src/validation.cpp index e3a00e4241e..dce003ff2b5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4520,7 +4520,7 @@ void ChainstateManager::LoadExternalBlockFile( int nLoaded = 0; try { // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor - CBufferedFile blkdat(fileIn, 2*MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE+8, SER_DISK, CLIENT_VERSION); + CBufferedFile blkdat{fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, CLIENT_VERSION}; // nRewind indicates where to resume scanning in case something goes wrong, // such as a block fails to deserialize. uint64_t nRewind = blkdat.GetPos(); From fa19c914f7fe7be127c0fb330b41ff7c091f40aa Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 12 Sep 2023 12:23:49 +0200 Subject: [PATCH 3/3] scripted-diff: Rename CBufferedFile to BufferedFile While touching all constructors in the previous commit, the class name can be adjusted to comply with the style guide. -BEGIN VERIFY SCRIPT- sed -i 's/CBufferedFile/BufferedFile/g' $( git grep -l CBufferedFile ) -END VERIFY SCRIPT- --- src/bench/streams_findbyte.cpp | 2 +- src/streams.h | 14 +++++++------- src/test/fuzz/buffered_file.cpp | 2 +- src/test/streams_tests.cpp | 10 +++++----- src/validation.cpp | 4 ++-- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index 451b5f758dc..22b8f1b356b 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -16,7 +16,7 @@ static void FindByte(benchmark::Bench& bench) data[file_size-1] = 1; fwrite(&data, sizeof(uint8_t), file_size, file); rewind(file); - CBufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; + BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; bench.run([&] { bf.SetPos(0); diff --git a/src/streams.h b/src/streams.h index eec9c0956ba..f9a817c9b64 100644 --- a/src/streams.h +++ b/src/streams.h @@ -577,7 +577,7 @@ public: * Will automatically close the file when it goes out of scope if not null. * If you need to close the file early, use file.fclose() instead of fclose(file). */ -class CBufferedFile +class BufferedFile { private: const int nVersion; @@ -600,7 +600,7 @@ private: return false; size_t nBytes = fread((void*)&vchBuf[pos], 1, readNow, src); if (nBytes == 0) { - throw std::ios_base::failure(feof(src) ? "CBufferedFile::Fill: end of file" : "CBufferedFile::Fill: fread failed"); + throw std::ios_base::failure(feof(src) ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed"); } nSrcPos += nBytes; return true; @@ -629,7 +629,7 @@ private: } public: - CBufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn) + BufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn) : nVersion{nVersionIn}, nReadLimit{std::numeric_limits::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0}) { if (nRewindIn >= nBufSize) @@ -637,14 +637,14 @@ public: src = fileIn; } - ~CBufferedFile() + ~BufferedFile() { fclose(); } // Disallow copies - CBufferedFile(const CBufferedFile&) = delete; - CBufferedFile& operator=(const CBufferedFile&) = delete; + BufferedFile(const BufferedFile&) = delete; + BufferedFile& operator=(const BufferedFile&) = delete; int GetVersion() const { return nVersion; } @@ -711,7 +711,7 @@ public: } template - CBufferedFile& operator>>(T&& obj) { + BufferedFile& operator>>(T&& obj) { ::Unserialize(*this, obj); return (*this); } diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index 47aa7e16603..1116274e3d2 100644 --- a/src/test/fuzz/buffered_file.cpp +++ b/src/test/fuzz/buffered_file.cpp @@ -18,7 +18,7 @@ FUZZ_TARGET(buffered_file) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider); - std::optional opt_buffered_file; + std::optional opt_buffered_file; FILE* fuzzed_file = fuzzed_file_provider.open(); try { opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegral()); diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 02add89ab9a..99740ee7792 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) // The buffer size (second arg) must be greater than the rewind // amount (third arg). try { - CBufferedFile bfbad{file, 25, 25, 333}; + BufferedFile bfbad{file, 25, 25, 333}; BOOST_CHECK(false); } catch (const std::exception& e) { BOOST_CHECK(strstr(e.what(), @@ -268,7 +268,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) } // The buffer is 25 bytes, allow rewinding 10 bytes. - CBufferedFile bf{file, 25, 10, 333}; + BufferedFile bf{file, 25, 10, 333}; BOOST_CHECK(!bf.eof()); // This member has no functional effect. @@ -356,7 +356,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) BOOST_CHECK(false); } catch (const std::exception& e) { BOOST_CHECK(strstr(e.what(), - "CBufferedFile::Fill: end of file") != nullptr); + "BufferedFile::Fill: end of file") != nullptr); } // Attempting to read beyond the end sets the EOF indicator. BOOST_CHECK(bf.eof()); @@ -391,7 +391,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) rewind(file); // The buffer is 25 bytes, allow rewinding 10 bytes. - CBufferedFile bf{file, 25, 10, 333}; + BufferedFile bf{file, 25, 10, 333}; uint8_t i; // This is like bf >> (7-byte-variable), in that it will cause data @@ -445,7 +445,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) size_t bufSize = InsecureRandRange(300) + 1; size_t rewindSize = InsecureRandRange(bufSize); - CBufferedFile bf{file, bufSize, rewindSize, 333}; + BufferedFile bf{file, bufSize, rewindSize, 333}; size_t currentPos = 0; size_t maxPos = 0; for (int step = 0; step < 100; ++step) { diff --git a/src/validation.cpp b/src/validation.cpp index dce003ff2b5..98a668e1b40 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4519,8 +4519,8 @@ void ChainstateManager::LoadExternalBlockFile( int nLoaded = 0; try { - // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor - CBufferedFile blkdat{fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, CLIENT_VERSION}; + // This takes over fileIn and calls fclose() on it in the BufferedFile destructor + BufferedFile blkdat{fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, CLIENT_VERSION}; // nRewind indicates where to resume scanning in case something goes wrong, // such as a block fails to deserialize. uint64_t nRewind = blkdat.GetPos();