Merge bitcoin/bitcoin#32997: index: Deduplicate HashKey / HeightKey handling

5646e6c index: restrict index helper function to namespace (Martin Zumsande)
032f350 index, refactor: deduplicate LookUpOne (Martin Zumsande)
a67d3eb index: deduplicate Hash / Height handling (Martin Zumsande)

Pull request description:

  The logic for `DBHashKey` / `DBHeightKey` handling and lookup of entries is shared by `coinstatsindex` and `blockfilterindex`, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/db_key.h` (using templates for the index-specific `DBVal`).

ACKs for top commit:
  fjahr:
    re-ACK 5646e6c
  furszy:
    utACK 5646e6c
  sedited:
    ACK 5646e6c

Tree-SHA512: 6f41684d6a9fd9bb01239e9f2e39a12837554f247a677eadcc242f0c1a2d44a79979f87249c4e0305ef1aa708d7056e56dfc40e1509c6d6aec2714f202fd2e09

Author: fanquake

src/index/blockfilterindex.cpp

@@ -11,6 +11,7 @@
 #include <flatfile.h>
 #include <hash.h>
 #include <index/base.h>
+#include <index/db_key.h>
 #include <interfaces/chain.h>
 #include <interfaces/types.h>
 #include <logging.h>
@@ -25,7 +26,6 @@
 
 #include <cerrno>
 #include <exception>
-#include <ios>
 #include <map>
 #include <optional>
 #include <span>
@@ -46,12 +46,8 @@
  * disk location of the next block filter to be written (represented as a FlatFilePos) is stored
  * under the DB_FILTER_POS key.
  *
- * Keys for the height index have the type [DB_BLOCK_HEIGHT, uint32 (BE)]. The height is represented
- * as big-endian so that sequential reads of filters by height are fast.
- * Keys for the hash index have the type [DB_BLOCK_HASH, uint256].
+ * The logic for keys is shared with other indexes, see index/db_key.h.
  */
-constexpr uint8_t DB_BLOCK_HASH{'s'};
-constexpr uint8_t DB_BLOCK_HEIGHT{'t'};
 constexpr uint8_t DB_FILTER_POS{'P'};
 
 constexpr unsigned int MAX_FLTR_FILE_SIZE = 0x1000000; // 16 MiB
@@ -74,45 +70,6 @@ struct DBVal {
     SERIALIZE_METHODS(DBVal, obj) { READWRITE(obj.hash, obj.header, obj.pos); }
 };
 
-struct DBHeightKey {
-    int height;
-
-    explicit DBHeightKey(int height_in) : height(height_in) {}
-
-    template<typename Stream>
-    void Serialize(Stream& s) const
-    {
-        ser_writedata8(s, DB_BLOCK_HEIGHT);
-        ser_writedata32be(s, height);
-    }
-
-    template<typename Stream>
-    void Unserialize(Stream& s)
-    {
-        const uint8_t prefix{ser_readdata8(s)};
-        if (prefix != DB_BLOCK_HEIGHT) {
-            throw std::ios_base::failure("Invalid format for block filter index DB height key");
-        }
-        height = ser_readdata32be(s);
-    }
-};
-
-struct DBHashKey {
-    uint256 hash;
-
-    explicit DBHashKey(const uint256& hash_in) : hash(hash_in) {}
-
-    SERIALIZE_METHODS(DBHashKey, obj) {
-        uint8_t prefix{DB_BLOCK_HASH};
-        READWRITE(prefix);
-        if (prefix != DB_BLOCK_HASH) {
-            throw std::ios_base::failure("Invalid format for block filter index DB hash key");
-        }
-
-        READWRITE(obj.hash);
-    }
-};
-
 }; // namespace
 
 static std::map<BlockFilterType, BlockFilterIndex> g_filter_indexes;
@@ -278,7 +235,7 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
 std::optional<uint256> BlockFilterIndex::ReadFilterHeader(int height, const uint256& expected_block_hash)
 {
     std::pair<uint256, DBVal> read_out;
-    if (!m_db->Read(DBHeightKey(height), read_out)) {
+    if (!m_db->Read(index_util::DBHeightKey(height), read_out)) {
         return std::nullopt;
     }
 
@@ -311,35 +268,12 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c
     value.second.header = filter_header;
     value.second.pos = m_next_filter_pos;
 
-    m_db->Write(DBHeightKey(block_height), value);
+    m_db->Write(index_util::DBHeightKey(block_height), value);
 
     m_next_filter_pos.nPos += bytes_written;
     return true;
 }
 
-[[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
-                                                     const std::string& index_name, int height)
-{
-    DBHeightKey key(height);
-    db_it.Seek(key);
-
-    if (!db_it.GetKey(key) || key.height != height) {
-        LogError("unexpected key in %s: expected (%c, %d)",
-                  index_name, DB_BLOCK_HEIGHT, height);
-        return false;
-    }
-
-    std::pair<uint256, DBVal> value;
-    if (!db_it.GetValue(value)) {
-        LogError("unable to read value in %s at key (%c, %d)",
-                 index_name, DB_BLOCK_HEIGHT, height);
-        return false;
-    }
-
-    batch.Write(DBHashKey(value.first), std::move(value.second));
-    return true;
-}
-
 bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block)
 {
     CDBBatch batch(*m_db);
@@ -348,7 +282,7 @@ bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block)
     // During a reorg, we need to copy block filter that is getting disconnected from the
     // height index to the hash index so we can still find it when the height index entry
     // is overwritten.
-    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height)) {
+    if (!index_util::CopyHeightIndexToHashIndex<DBVal>(*db_it, batch, m_name, block.height)) {
         return false;
     }
 
@@ -363,24 +297,6 @@ bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block)
     return true;
 }
 
-static bool LookupOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVal& result)
-{
-    // First check if the result is stored under the height index and the value there matches the
-    // block hash. This should be the case if the block is on the active chain.
-    std::pair<uint256, DBVal> read_out;
-    if (!db.Read(DBHeightKey(block_index->nHeight), read_out)) {
-        return false;
-    }
-    if (read_out.first == block_index->GetBlockHash()) {
-        result = std::move(read_out.second);
-        return true;
-    }
-
-    // If value at the height index corresponds to an different block, the result will be stored in
-    // the hash index.
-    return db.Read(DBHashKey(block_index->GetBlockHash()), result);
-}
-
 static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start_height,
                         const CBlockIndex* stop_index, std::vector<DBVal>& results)
 {
@@ -397,9 +313,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
     size_t results_size = static_cast<size_t>(stop_index->nHeight - start_height + 1);
     std::vector<std::pair<uint256, DBVal>> values(results_size);
 
-    DBHeightKey key(start_height);
+    index_util::DBHeightKey key(start_height);
     std::unique_ptr<CDBIterator> db_it(db.NewIterator());
-    db_it->Seek(DBHeightKey(start_height));
+    db_it->Seek(index_util::DBHeightKey(start_height));
     for (int height = start_height; height <= stop_index->nHeight; ++height) {
         if (!db_it->Valid() || !db_it->GetKey(key) || key.height != height) {
             return false;
@@ -408,7 +324,7 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
         size_t i = static_cast<size_t>(height - start_height);
         if (!db_it->GetValue(values[i])) {
             LogError("unable to read value in %s at key (%c, %d)",
-                     index_name, DB_BLOCK_HEIGHT, height);
+                     index_name, index_util::DB_BLOCK_HEIGHT, height);
             return false;
         }
 
@@ -430,9 +346,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
             continue;
         }
 
-        if (!db.Read(DBHashKey(block_hash), results[i])) {
+        if (!db.Read(index_util::DBHashKey(block_hash), results[i])) {
             LogError("unable to read value in %s at key (%c, %s)",
-                     index_name, DB_BLOCK_HASH, block_hash.ToString());
+                     index_name, index_util::DB_BLOCK_HASH, block_hash.ToString());
             return false;
         }
     }
@@ -443,7 +359,7 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
 bool BlockFilterIndex::LookupFilter(const CBlockIndex* block_index, BlockFilter& filter_out) const
 {
     DBVal entry;
-    if (!LookupOne(*m_db, block_index, entry)) {
+    if (!index_util::LookUpOne(*m_db, {block_index->GetBlockHash(), block_index->nHeight}, entry)) {
         return false;
     }
 
@@ -466,7 +382,7 @@ bool BlockFilterIndex::LookupFilterHeader(const CBlockIndex* block_index, uint25
     }
 
     DBVal entry;
-    if (!LookupOne(*m_db, block_index, entry)) {
+    if (!index_util::LookUpOne(*m_db, {block_index->GetBlockHash(), block_index->nHeight}, entry)) {
         return false;
     }
 

src/index/coinstatsindex.cpp

@@ -13,6 +13,7 @@
 #include <crypto/muhash.h>
 #include <dbwrapper.h>
 #include <index/base.h>
+#include <index/db_key.h>
 #include <interfaces/chain.h>
 #include <interfaces/types.h>
 #include <kernel/coinstats.h>
@@ -28,7 +29,6 @@
 #include <validation.h>
 
 #include <compare>
-#include <ios>
 #include <limits>
 #include <span>
 #include <string>
@@ -40,8 +40,6 @@ using kernel::CCoinsStats;
 using kernel::GetBogoSize;
 using kernel::RemoveCoinHash;
 
-static constexpr uint8_t DB_BLOCK_HASH{'s'};
-static constexpr uint8_t DB_BLOCK_HEIGHT{'t'};
 static constexpr uint8_t DB_MUHASH{'M'};
 
 namespace {
@@ -85,47 +83,6 @@ struct DBVal {
         SER_READ(obj, obj.total_coinbase_amount = UintToArith256(coinbase));
     }
 };
-
-struct DBHeightKey {
-    int height;
-
-    explicit DBHeightKey(int height_in) : height(height_in) {}
-
-    template <typename Stream>
-    void Serialize(Stream& s) const
-    {
-        ser_writedata8(s, DB_BLOCK_HEIGHT);
-        ser_writedata32be(s, height);
-    }
-
-    template <typename Stream>
-    void Unserialize(Stream& s)
-    {
-        const uint8_t prefix{ser_readdata8(s)};
-        if (prefix != DB_BLOCK_HEIGHT) {
-            throw std::ios_base::failure("Invalid format for coinstatsindex DB height key");
-        }
-        height = ser_readdata32be(s);
-    }
-};
-
-struct DBHashKey {
-    uint256 block_hash;
-
-    explicit DBHashKey(const uint256& hash_in) : block_hash(hash_in) {}
-
-    SERIALIZE_METHODS(DBHashKey, obj)
-    {
-        uint8_t prefix{DB_BLOCK_HASH};
-        READWRITE(prefix);
-        if (prefix != DB_BLOCK_HASH) {
-            throw std::ios_base::failure("Invalid format for coinstatsindex DB hash key");
-        }
-
-        READWRITE(obj.block_hash);
-    }
-};
-
 }; // namespace
 
 std::unique_ptr<CoinStatsIndex> g_coin_stats_index;
@@ -253,30 +210,7 @@ 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.
-    m_db->Write(DBHeightKey(block.height), value);
-    return true;
-}
-
-[[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
-                                                     const std::string& index_name, int height)
-{
-    DBHeightKey key{height};
-    db_it.Seek(key);
-
-    if (!db_it.GetKey(key) || key.height != height) {
-        LogError("unexpected key in %s: expected (%c, %d)",
-                 index_name, DB_BLOCK_HEIGHT, height);
-        return false;
-    }
-
-    std::pair<uint256, DBVal> value;
-    if (!db_it.GetValue(value)) {
-        LogError("unable to read value in %s at key (%c, %d)",
-                 index_name, DB_BLOCK_HEIGHT, height);
-        return false;
-    }
-
-    batch.Write(DBHashKey(value.first), value.second);
+    m_db->Write(index_util::DBHeightKey(block.height), value);
     return true;
 }
 
@@ -287,7 +221,7 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block)
 
     // During a reorg, copy the block's hash digest from the height index to the hash index,
     // ensuring it's still accessible after the height index entry is overwritten.
-    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height)) {
+    if (!index_util::CopyHeightIndexToHashIndex<DBVal>(*db_it, batch, m_name, block.height)) {
         return false;
     }
 
@@ -300,32 +234,13 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block)
     return true;
 }
 
-static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result)
-{
-    // First check if the result is stored under the height index and the value
-    // there matches the block hash. This should be the case if the block is on
-    // the active chain.
-    std::pair<uint256, DBVal> read_out;
-    if (!db.Read(DBHeightKey(block.height), read_out)) {
-        return false;
-    }
-    if (read_out.first == block.hash) {
-        result = std::move(read_out.second);
-        return true;
-    }
-
-    // If value at the height index corresponds to an different block, the
-    // result will be stored in the hash index.
-    return db.Read(DBHashKey(block.hash), result);
-}
-
 std::optional<CCoinsStats> CoinStatsIndex::LookUpStats(const CBlockIndex& block_index) const
 {
     CCoinsStats stats{block_index.nHeight, block_index.GetBlockHash()};
     stats.index_used = true;
 
     DBVal entry;
-    if (!LookUpOne(*m_db, {block_index.GetBlockHash(), block_index.nHeight}, entry)) {
+    if (!index_util::LookUpOne(*m_db, {block_index.GetBlockHash(), block_index.nHeight}, entry)) {
         return std::nullopt;
     }
 
@@ -360,7 +275,7 @@ bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockRef>& block
 
     if (block) {
         DBVal entry;
-        if (!LookUpOne(*m_db, *block, entry)) {
+        if (!index_util::LookUpOne(*m_db, *block, entry)) {
             LogError("Cannot read current %s state; index may be corrupted",
                       GetName());
             return false;
@@ -415,7 +330,7 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block)
 
     // Ignore genesis block
     if (block.height > 0) {
-        if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
+        if (!m_db->Read(index_util::DBHeightKey(block.height - 1), read_out)) {
             return false;
         }
 
@@ -424,7 +339,7 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block)
             LogWarning("previous block header belongs to unexpected block %s; expected %s",
                       read_out.first.ToString(), expected_block_hash.ToString());
 
-            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
+            if (!m_db->Read(index_util::DBHashKey(expected_block_hash), read_out)) {
                 LogError("previous block header not found; expected %s",
                           expected_block_hash.ToString());
                 return false;