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/key.h (using templates for the index-specific DBVal).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32997.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | fjahr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Possible typos and grammar issues:
drahtbot_id_4_m
118@@ -119,23 +119,6 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
119
120 // Ignore genesis block
121 if (block.height > 0) {
122- std::pair<uint256, DBVal> read_out;
123- if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
ah, ok. I thought that it was used in the past for something but no longer is, because we load data into a local variable and let it go out of scope without using it. So the point is the return false / the logged warning? Is this a legitimitate situation we can get into outside of index corruption?
In any case, I’ll drop that commit with the next push, it’s not the main focus of this PR.
The base index class is responsible for detecting reorgs and calling the appropriate methods from the child class: CustomRemove() up to the forking point, then CustomAppend() to connect the new chain blocks. This happens during both initial sync and the validation event. I’m pretty confident we can safely remove the extra check.
Furthermore, since the index child class receives block connection and disconnection events in order, I don’t see why it should care about reorgs at all (speaking generally here, for all index child classes). That’s one of the base class responsibilities.
Also, if we do want to keep the previous block check for some reason, I think we could cache it during CustomInit(), then update it in both CustomAppend() and CustomRemove(). Or.. we could keep the base class’s m_best_block_index updated with every processed block too. Either way, I think we can avoid accessing disk for this.
The code was largely duplicated between coinstatsindex
and blockfilterindex.
Deduplicate it by moving it to a shared file.
slight change in behavior: the index name is no longer
part of the error msg in case of (un)serialization errors.
LookUpOne is used by both coinstatsindex and blockfilterindex,
the two implementations had already started to deviate slightly
for no apparent reason.
75@@ -76,4 +76,24 @@ template <typename DBVal>
76 return true;
77 }
78
79+template <typename DBVal>
80+static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result)
nit: could do IWYU I guess, even though this wasn’t followed that well in the index cpp files.
0#include <dbwrapper.h>
1#include <interfaces/types.h>
30@@ -30,8 +31,6 @@
31 * as big-endian so that sequential reads of filters by height are fast.
32 * Keys for the hash index have the type [DB_BLOCK_HASH, uint256].
33 */
34-constexpr uint8_t DB_BLOCK_HASH{'s'};
The comment above here still talks about three items stored for each block but after the change only one of the values is here. This would need to be adopted somehow and probably some of the docs moved over to index/key.h.
EDIT: Maybe the current comment can be left almost untouched but there needs to be a reference to where the keys are now.
8+#include <cstdint>
9+#include <string>
10+#include <stdexcept>
11+#include <utility>
12+
13+static constexpr uint8_t DB_BLOCK_HASH{'s'};
db_key.h since we have other kinds of keys but I don’t feel too strongly about it.
blockfilterindex.cpp you can consider my comments optional.