index: Deduplicate HashKey / HeightKey handling #32997
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32997. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
Concept ACK on the deduplication parts |
6988e01 to
03b67a2
Compare
src/index/key.h
Outdated
| @@ -76,4 +76,24 @@ template <typename DBVal> | |||
| return true; | |||
| } | |||
|
|
|||
| template <typename DBVal> | |||
| static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result) | |||
There was a problem hiding this comment.
nit: could do IWYU I guess, even though this wasn't followed that well in the index cpp files.
#include <dbwrapper.h>
#include <interfaces/types.h>
There was a problem hiding this comment.
added these and serialize.h, hope that'll do. (never got iwyu running locally and the tidy job didn't cover the new file for some reason).
| @@ -30,8 +31,6 @@ | |||
| * 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]. | |||
| */ | |||
| constexpr uint8_t DB_BLOCK_HASH{'s'}; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, I think the part with the "three items" is still correct, it talks about the values of the index, not the keys.
I replaced the part about the key from blockfilterindex.cpp by a reference to db_key.h
src/index/key.h
Outdated
| #include <stdexcept> | ||
| #include <utility> | ||
|
|
||
| static constexpr uint8_t DB_BLOCK_HASH{'s'}; |
There was a problem hiding this comment.
Aside from the documentation moving necessary mentioned above maybe there could be 1-2 general sentences here that this deals with the hash/height keys and that it's mostly relevant to indexes that use these.
There was a problem hiding this comment.
Also, very minor naming nit: File could be called db_key.h since we have other kinds of keys but I don't feel too strongly about it.
There was a problem hiding this comment.
Done - I added a comment to the file, using parts from blockfilterindex.cpp.
I agree that db_key.h is nicer, changed it to that.
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.
03b67a2 to
936bd87
Compare
|
utACK 936bd87 |
There was a problem hiding this comment.
Since DBHeightKey and DBHashKey are pretty similar. What do you think about a generic template for both? furszy@e5dd2b0.
(Only needed to specialize the serialization for the value)
The logic for
DBHashKey/DBHeightKeyhandling and lookup of entries is shared bycoinstatsindexandblockfilterindex, leading to many lines of duplicated code. De-duplicate this by moving the logic toindex/db_key.h(using templates for the index-specificDBVal).