util: Remove `FilterHeaderHasher` #34351

rustaceanrob commented at 1:30 PM on January 20, 2026: contributor

With respect to std::unordered_map documentation, the Hash type defined in the template is over the Key and not T, the value. This hasher is incorrectly named as the FilterHeader is the value within this map. I consider this a bug as opposed to a refactor as the key and value relationship is implied to be filter header -> block hash when it is the opposite.

Further, the hasher for the key already exists via BlockHasher.

ref: https://en.cppreference.com/w/cpp/container/unordered_map.html

DrahtBot added the label Utils/log/libs on Jan 20, 2026

DrahtBot commented at 1:30 PM on January 20, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34351.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, andrewtoth, maflcko

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in src/index/blockfilterindex.h:54 in 5deb7ae960

  50 | @@ -51,7 +51,7 @@ class BlockFilterIndex final : public BaseIndex
  51 |  
  52 |      Mutex m_cs_headers_cache;
  53 |      /** cache of block hash to filter header, to avoid disk access when responding to getcfcheckpt. */
  54 | -    std::unordered_map<uint256, uint256, FilterHeaderHasher> m_headers_cache GUARDED_BY(m_cs_headers_cache);
  55 | +    std::unordered_map<uint256, uint256, Uint256Hasher> m_headers_cache GUARDED_BY(m_cs_headers_cache);

andrewtoth commented at 2:40 PM on January 20, 2026:

But, this is a map of block hash -> filter header. The key is a block hash, so wouldn't just replacing this with the existing BlockHasher be a more appropriate fix?

rustaceanrob commented at 3:01 PM on January 20, 2026:

I think the name would be BlockHashHasher, which felt strange but I am impartial to the naming here. I went with Uint256Hasher as it was more general.

maflcko commented at 3:30 PM on January 20, 2026:

I went with Uint256Hasher as it was more general.

I think it is not really a general uint256 hasher. The assumption is that the uint256 is a block hash with pow (or derived from a block hash). Otherwise, it seems risky to use without salt. So i think putting "block" somewhere in the name makes sense. Otherwise, it is easier to mis-use.

rustaceanrob commented at 4:05 PM on January 20, 2026:

I see, makes sense. Updated 454b213f8890658d39be86804550e8ddba9b7364

andrewtoth commented at 4:20 PM on January 20, 2026:

Why not instead rename the existing BlockHasher to BlockHashHasher and remove this hasher specific to filter headers?

l0rinc commented at 4:21 PM on January 20, 2026:

yes, similar comment to @andrewtoth: #34351 (review)

rustaceanrob commented at 4:24 PM on January 20, 2026:

so wouldn't just replacing this with the existing BlockHasher be a more appropriate fix?

Sorry, I overlooked the word "existing" in the review above and did not parse the file very carefully. Yes, makes sense.

rustaceanrob commented at 4:29 PM on January 20, 2026:

Updated ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f

rustaceanrob force-pushed on Jan 20, 2026

rustaceanrob renamed this:
~~util: Rename `FilterHeaderHasher` to `Uint256Hasher`~~
util: Rename `FilterHeaderHasher` to `BlockHashHasher`
on Jan 20, 2026

in src/index/blockfilterindex.h:54 in 454b213f88

  50 | @@ -51,7 +51,7 @@ class BlockFilterIndex final : public BaseIndex
  51 |  
  52 |      Mutex m_cs_headers_cache;
  53 |      /** cache of block hash to filter header, to avoid disk access when responding to getcfcheckpt. */
  54 | -    std::unordered_map<uint256, uint256, FilterHeaderHasher> m_headers_cache GUARDED_BY(m_cs_headers_cache);
  55 | +    std::unordered_map<uint256, uint256, BlockHashHasher> m_headers_cache GUARDED_BY(m_cs_headers_cache);

l0rinc commented at 4:20 PM on January 20, 2026:

why not use https://github.com/bitcoin/bitcoin/blob/9ca52a4cbece2963363d201ef2b15d58d96ea221/src/util/hasher.h#L104-L110 instead, it's exactly the same.

Or if we decide to keep it for some reason (since this has the same pattern as https://github.com/bitcoin/bitcoin/blob/fa942332b40c97375af0722f32f7575bca3af819/src/hash.h#L132-L138), we could name it something like CheapBlockHasher to indicate it's truncated version of its hash.

l0rinc commented at 4:50 PM on January 20, 2026:

Please resolve this comment

l0rinc changes_requested

util: Remove `FilterHeaderHasher`

With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.

Further, the hasher for the key already exists via `BlockHasher`.

ref: https://en.cppreference.com/w/cpp/container/unordered_map.html

ccf9172ab3

rustaceanrob force-pushed on Jan 20, 2026

rustaceanrob renamed this:
~~util: Rename `FilterHeaderHasher` to `BlockHashHasher`~~
util: Remove `FilterHeaderHasher`
on Jan 20, 2026

DrahtBot added the label CI failed on Jan 20, 2026

l0rinc approved

l0rinc commented at 4:36 PM on January 20, 2026: contributor

the PR title and description are needlessly complicated now that it's just a deduplication

ismaelsadeeq approved

ismaelsadeeq commented at 4:49 PM on January 20, 2026: member

ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f 👍🏾

DrahtBot removed the label CI failed on Jan 20, 2026

andrewtoth commented at 10:28 PM on January 24, 2026: contributor

ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f

maflcko commented at 8:58 AM on January 26, 2026: member

lgtm ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f

fanquake merged this on Jan 26, 2026

fanquake closed this on Jan 26, 2026