From 50b63a5698e533376ef7a20bc0c440d3d6bf7a9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:18:54 -0700 Subject: [PATCH 1/5] refactor: inline constant return value of `CDBWrapper::WriteBatch` `WriteBatch` can only ever return `true` - its errors are handled by throwing a `throw dbwrapper_error` instead. The boolean return value is quite confusing, especially since it's symmetric with `CDBWrapper::Read`, which catches the exceptions and returns a boolean instead. We're removing the constant return value and inlining `true` for its usages. --- src/dbwrapper.cpp | 3 +-- src/dbwrapper.h | 8 +++++--- src/index/base.cpp | 2 +- src/index/blockfilterindex.cpp | 2 +- src/index/coinstatsindex.cpp | 2 +- src/index/txindex.cpp | 3 ++- src/node/blockstorage.cpp | 3 ++- src/test/dbwrapper_tests.cpp | 2 +- src/txdb.cpp | 4 ++-- 9 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index fe5f9cb0893..cb9abee563f 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -274,7 +274,7 @@ CDBWrapper::~CDBWrapper() DBContext().options.env = nullptr; } -bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) +void CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) { const bool log_memory = LogAcceptCategory(BCLog::LEVELDB, BCLog::Level::Debug); double mem_before = 0; @@ -288,7 +288,6 @@ bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) LogDebug(BCLog::LEVELDB, "WriteBatch memory usage: db=%s, before=%.1fMiB, after=%.1fMiB\n", m_name, mem_before, mem_after); } - return true; } size_t CDBWrapper::DynamicMemoryUsage() const diff --git a/src/dbwrapper.h b/src/dbwrapper.h index b9b98bd96ad..94e452db76b 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -234,7 +234,8 @@ public: { CDBBatch batch(*this); batch.Write(key, value); - return WriteBatch(batch, fSync); + WriteBatch(batch, fSync); + return true; } //! @returns filesystem path to the on-disk data. @@ -259,10 +260,11 @@ public: { CDBBatch batch(*this); batch.Erase(key); - return WriteBatch(batch, fSync); + WriteBatch(batch, fSync); + return true; } - bool WriteBatch(CDBBatch& batch, bool fSync = false); + void WriteBatch(CDBBatch& batch, bool fSync = false); // Get an estimate of LevelDB memory usage (in bytes). size_t DynamicMemoryUsage() const; diff --git a/src/index/base.cpp b/src/index/base.cpp index fdd0e0d8af2..0fdb5af3ba1 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -262,7 +262,7 @@ bool BaseIndex::Commit() ok = CustomCommit(batch); if (ok) { GetDB().WriteBestBlock(batch, GetLocator(*m_chain, m_best_block_index.load()->GetBlockHash())); - ok = GetDB().WriteBatch(batch); + GetDB().WriteBatch(batch); } } if (!ok) { diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 2ccae3a221b..c56d787907a 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -338,7 +338,7 @@ bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block) // But since this creates new references to the filter, the position should get updated here // atomically as well in case Commit fails. batch.Write(DB_FILTER_POS, m_next_filter_pos); - if (!m_db->WriteBatch(batch)) return false; + m_db->WriteBatch(batch); // Update cached header to the previous block hash m_last_header = *Assert(ReadFilterHeader(block.height - 1, *Assert(block.prev_hash))); diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 96693f7f48a..8f172d706a7 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -263,7 +263,7 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block) return false; } - if (!m_db->WriteBatch(batch)) return false; + m_db->WriteBatch(batch); if (!ReverseBlock(block)) { return false; // failure cause logged internally diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 11dd856e1b6..16038a3a2b6 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -46,7 +46,8 @@ bool TxIndex::DB::WriteTxs(const std::vector>& v_pos for (const auto& [txid, pos] : v_pos) { batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } - return WriteBatch(batch); + WriteBatch(batch); + return true; } TxIndex::TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 3c01ff2e8d6..f1d4be6b706 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -88,7 +88,8 @@ bool BlockTreeDB::WriteBatchSync(const std::vectorGetBlockHash()), CDiskBlockIndex{bi}); } - return WriteBatch(batch, true); + WriteBatch(batch, true); + return true; } bool BlockTreeDB::WriteFlag(const std::string& name, bool fValue) diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp index cd0f347b66e..7066665751d 100644 --- a/src/test/dbwrapper_tests.cpp +++ b/src/test/dbwrapper_tests.cpp @@ -173,7 +173,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch) // Remove key3 before it's even been written batch.Erase(key3); - BOOST_CHECK(dbw.WriteBatch(batch)); + dbw.WriteBatch(batch); BOOST_CHECK(dbw.Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); diff --git a/src/txdb.cpp b/src/txdb.cpp index bb6ee2eb524..23824f39c06 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -149,9 +149,9 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB batch.Write(DB_BEST_BLOCK, hashBlock); LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0)); - bool ret = m_db->WriteBatch(batch); + m_db->WriteBatch(batch); LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count); - return ret; + return true; } size_t CCoinsViewDB::EstimateSize() const From d1847cf5b5af232ad180f5d302361b72334952b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:23:07 -0700 Subject: [PATCH 2/5] refactor: inline constant return value of `TxIndex::DB::WriteTxs` --- src/index/txindex.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 16038a3a2b6..9554faf1e3a 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -28,7 +28,7 @@ public: bool ReadTxPos(const Txid& txid, CDiskTxPos& pos) const; /// Write a batch of transaction positions to the DB. - [[nodiscard]] bool WriteTxs(const std::vector>& v_pos); + void WriteTxs(const std::vector>& v_pos); }; TxIndex::DB::DB(size_t n_cache_size, bool f_memory, bool f_wipe) : @@ -40,14 +40,13 @@ bool TxIndex::DB::ReadTxPos(const Txid& txid, CDiskTxPos& pos) const return Read(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } -bool TxIndex::DB::WriteTxs(const std::vector>& v_pos) +void TxIndex::DB::WriteTxs(const std::vector>& v_pos) { CDBBatch batch(*this); for (const auto& [txid, pos] : v_pos) { batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } WriteBatch(batch); - return true; } TxIndex::TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) @@ -69,7 +68,8 @@ bool TxIndex::CustomAppend(const interfaces::BlockInfo& block) vPos.emplace_back(tx->GetHash(), pos); pos.nTxOffset += ::GetSerializeSize(TX_WITH_WITNESS(*tx)); } - return m_db->WriteTxs(vPos); + m_db->WriteTxs(vPos); + return true; } BaseIndex::DB& TxIndex::GetDB() const { return *m_db; } From cdab9480e9e35656f490878f92dab5427b36f21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:28:44 -0700 Subject: [PATCH 3/5] refactor: inline constant return value of `CDBWrapper::Write` --- src/dbwrapper.h | 3 +-- src/index/blockfilterindex.cpp | 4 +--- src/index/coinstatsindex.cpp | 3 ++- src/node/blockstorage.cpp | 6 ++++-- src/test/dbwrapper_tests.cpp | 36 +++++++++++++++++----------------- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 94e452db76b..50be26fb99e 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -230,12 +230,11 @@ public: } template - bool Write(const K& key, const V& value, bool fSync = false) + void Write(const K& key, const V& value, bool fSync = false) { CDBBatch batch(*this); batch.Write(key, value); WriteBatch(batch, fSync); - return true; } //! @returns filesystem path to the on-disk data. diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index c56d787907a..77bf931d9d0 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -291,9 +291,7 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c value.second.header = filter_header; value.second.pos = m_next_filter_pos; - if (!m_db->Write(DBHeightKey(block_height), value)) { - return false; - } + m_db->Write(DBHeightKey(block_height), value); m_next_filter_pos.nPos += bytes_written; return true; diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 8f172d706a7..8072167e452 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -226,7 +226,8 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // Intentionally do not update DB_MUHASH here so it stays in sync with // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown. - return m_db->Write(DBHeightKey(block.height), value); + m_db->Write(DBHeightKey(block.height), value); + return true; } [[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch, diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f1d4be6b706..7371111ccbc 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -62,7 +62,8 @@ bool BlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo& info) bool BlockTreeDB::WriteReindexing(bool fReindexing) { if (fReindexing) { - return Write(DB_REINDEX_FLAG, uint8_t{'1'}); + Write(DB_REINDEX_FLAG, uint8_t{'1'}); + return true; } else { return Erase(DB_REINDEX_FLAG); } @@ -94,7 +95,8 @@ bool BlockTreeDB::WriteBatchSync(const std::vector it(const_cast(dbw).NewIterator()); @@ -238,7 +238,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) uint256 in = m_rng.rand256(); uint256 res; - BOOST_CHECK(dbw->Write(key, in)); + dbw->Write(key, in); BOOST_CHECK(dbw->Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); @@ -261,7 +261,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) uint256 res3; // Check that we can write successfully - BOOST_CHECK(odbw.Write(key, in2)); + odbw.Write(key, in2); BOOST_CHECK(odbw.Read(key, res3)); BOOST_CHECK_EQUAL(res3.ToString(), in2.ToString()); } @@ -279,7 +279,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) uint256 in = m_rng.rand256(); uint256 res; - BOOST_CHECK(dbw->Write(key, in)); + dbw->Write(key, in); BOOST_CHECK(dbw->Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); @@ -298,7 +298,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) uint256 res3; // Check that we can write successfully - BOOST_CHECK(odbw.Write(key, in2)); + odbw.Write(key, in2); BOOST_CHECK(odbw.Read(key, res3)); BOOST_CHECK_EQUAL(res3.ToString(), in2.ToString()); } @@ -310,7 +310,7 @@ BOOST_AUTO_TEST_CASE(iterator_ordering) for (int x=0x00; x<256; ++x) { uint8_t key = x; uint32_t value = x*x; - if (!(x & 1)) BOOST_CHECK(dbw.Write(key, value)); + if (!(x & 1)) dbw.Write(key, value); } // Check that creating an iterator creates a snapshot @@ -319,7 +319,7 @@ BOOST_AUTO_TEST_CASE(iterator_ordering) for (unsigned int x=0x00; x<256; ++x) { uint8_t key = x; uint32_t value = x*x; - if (x & 1) BOOST_CHECK(dbw.Write(key, value)); + if (x & 1) dbw.Write(key, value); } for (const int seek_start : {0x00, 0x80}) { @@ -381,7 +381,7 @@ BOOST_AUTO_TEST_CASE(iterator_string_ordering) for (int z = 0; z < y; ++z) key += key; uint32_t value = x*x; - BOOST_CHECK(dbw.Write(StringContentsSerializer{key}, value)); + dbw.Write(StringContentsSerializer{key}, value); } } From e030240e909493549e24aa8bcd5b382cab6e2c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:33:39 -0700 Subject: [PATCH 4/5] refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` Did both in this commit, since the return value of `WriteReindexing` was ignored anyway - which existed only because of the constant `Erase` being called --- src/dbwrapper.h | 3 +-- src/node/blockstorage.cpp | 5 ++--- src/node/blockstorage.h | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 50be26fb99e..c770df8e206 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -255,12 +255,11 @@ public: } template - bool Erase(const K& key, bool fSync = false) + void Erase(const K& key, bool fSync = false) { CDBBatch batch(*this); batch.Erase(key); WriteBatch(batch, fSync); - return true; } void WriteBatch(CDBBatch& batch, bool fSync = false); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 7371111ccbc..5a54baf2b5e 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -59,13 +59,12 @@ bool BlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo& info) return Read(std::make_pair(DB_BLOCK_FILES, nFile), info); } -bool BlockTreeDB::WriteReindexing(bool fReindexing) +void BlockTreeDB::WriteReindexing(bool fReindexing) { if (fReindexing) { Write(DB_REINDEX_FLAG, uint8_t{'1'}); - return true; } else { - return Erase(DB_REINDEX_FLAG); + Erase(DB_REINDEX_FLAG); } } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index cee0eb61ed6..7d6f78f2532 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -55,7 +55,7 @@ public: bool WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info); bool ReadLastBlockFile(int& nFile); - bool WriteReindexing(bool fReindexing); + void WriteReindexing(bool fReindexing); void ReadReindexing(bool& fReindexing); bool WriteFlag(const std::string& name, bool fValue); bool ReadFlag(const std::string& name, bool& fValue); From 743abbcbde9e5a2db489bca461c98df461eff7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:37:40 -0700 Subject: [PATCH 5/5] refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` --- src/node/blockstorage.cpp | 13 ++++--------- src/node/blockstorage.h | 6 +++--- src/test/fuzz/block_index.cpp | 2 +- src/validation.cpp | 4 +--- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 5a54baf2b5e..a72292cb4a9 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -78,7 +78,7 @@ bool BlockTreeDB::ReadLastBlockFile(int& nFile) return Read(DB_LAST_BLOCK, nFile); } -bool BlockTreeDB::WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo) +void BlockTreeDB::WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo) { CDBBatch batch(*this); for (const auto& [file, info] : fileInfo) { @@ -89,13 +89,11 @@ bool BlockTreeDB::WriteBatchSync(const std::vectorGetBlockHash()), CDiskBlockIndex{bi}); } WriteBatch(batch, true); - return true; } -bool BlockTreeDB::WriteFlag(const std::string& name, bool fValue) +void BlockTreeDB::WriteFlag(const std::string& name, bool fValue) { Write(std::make_pair(DB_FLAG, name), fValue ? uint8_t{'1'} : uint8_t{'0'}); - return true; } bool BlockTreeDB::ReadFlag(const std::string& name, bool& fValue) @@ -478,7 +476,7 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha return true; } -bool BlockManager::WriteBlockIndexDB() +void BlockManager::WriteBlockIndexDB() { AssertLockHeld(::cs_main); std::vector> vFiles; @@ -494,10 +492,7 @@ bool BlockManager::WriteBlockIndexDB() m_dirty_blockindex.erase(it++); } int max_blockfile = WITH_LOCK(cs_LastBlockFile, return this->MaxBlockfileNum()); - if (!m_block_tree_db->WriteBatchSync(vFiles, max_blockfile, vBlocks)) { - return false; - } - return true; + m_block_tree_db->WriteBatchSync(vFiles, max_blockfile, vBlocks); } bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_blockhash) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 7d6f78f2532..caf291e9f2d 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -52,12 +52,12 @@ class BlockTreeDB : public CDBWrapper { public: using CDBWrapper::CDBWrapper; - bool WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); + void WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info); bool ReadLastBlockFile(int& nFile); void WriteReindexing(bool fReindexing); void ReadReindexing(bool& fReindexing); - bool WriteFlag(const std::string& name, bool fValue); + void WriteFlag(const std::string& name, bool fValue); bool ReadFlag(const std::string& name, bool& fValue); bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex, const util::SignalInterrupt& interrupt) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); @@ -300,7 +300,7 @@ public: std::unique_ptr m_block_tree_db GUARDED_BY(::cs_main); - bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool LoadBlockIndexDB(const std::optional& snapshot_blockhash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp index eef8c2efc80..5bc6240cabb 100644 --- a/src/test/fuzz/block_index.cpp +++ b/src/test/fuzz/block_index.cpp @@ -89,7 +89,7 @@ FUZZ_TARGET(block_index, .init = init_block_index) } // Store these files and blocks in the block index. It should not fail. - assert(block_index.WriteBatchSync(files_info, files_count - 1, blocks_info)); + block_index.WriteBatchSync(files_info, files_count - 1, blocks_info); // We should be able to read every block file info we stored. Its value should correspond to // what we stored above. diff --git a/src/validation.cpp b/src/validation.cpp index 7b7c8b2d5bd..c322118574b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2851,9 +2851,7 @@ bool Chainstate::FlushStateToDisk( { LOG_TIME_MILLIS_WITH_CATEGORY("write block index to disk", BCLog::BENCH); - if (!m_blockman.WriteBlockIndexDB()) { - return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to block index database.")); - } + m_blockman.WriteBlockIndexDB(); } // Finally remove any pruned files if (fFlushForPrune) {