Decouple `BlockFilter` from GCS implementation #35158

rustaceanrob commented at 8:29 AM on April 25, 2026: contributor

In researching potential additional block filter types, I found that BlockFilter is coupled with the GCS implementation. Callers of BlockFilter.GetFilter are interested in Match and MatchAny, so new filter types may simply implement those two methods. This change allows BlockFilter to hold arbitrary filter constructions while preserving how callers interact with block filters.

index: Introduce `BlockFilterBase` virtual class

The `BlockFilter` type is coupled to GCS as the implementation of
approximate set membership query. Callsites that query the block filter
do not need to be aware of the underlying filter type. This commit
introduces an opaque type of the properties a block filter has, and
implements these properties for GCS. Any future filter type need only
to implement this virtual class.

84524f1de5

DrahtBot commented at 8:29 AM on April 25, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	optout21

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

LLM Linter (✨ experimental)

Possible typos and grammar issues:

This is more efficient that checking Match on multiple elements separately. -> than [“that” is a typo here; it should be “than”]

2026-04-25 12:41:17

DrahtBot added the label CI failed on Apr 25, 2026

DrahtBot commented at 9:08 AM on April 25, 2026: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/24926767830/job/72997922791 LLM reason (✨ experimental): Fuzz build failed with a C++ type-conversion error: const BlockFilterBase could not be converted to const GCSFilter in blockfilter.cpp (no viable conversion).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

refactor: Use `BlockFilterBase` within `BlockFilter`

This commit allows for `BlockFilter` to hold any type that implements
the properties of a block filter, rather than concretely holding a GCS
filter.

eff833c063

rustaceanrob force-pushed on Apr 25, 2026

in src/blockfilter.h:145 in eff833c063

 142 |      bool BuildParams(GCSFilter::Params& params) const;
 143 |  
 144 |  public:
 145 |  
 146 | -    BlockFilter() = default;
 147 | +    BlockFilter() : m_filter(std::make_unique<GCSFilter>()) {}

junbyjun1238 commented at 1:25 PM on April 25, 2026:

Question: LookupFilterRange() calls filters_out.resize(entries.size()), which now allocates a GCSFilter object for each default-constructed BlockFilter, only to have it immediately replaced by the filter read from disk.

Before this PR, default construction already allocated the encoded vector, so the extra allocation is probably small compared to disk I/O. Still, this looks like an added cost of having BlockFilter own the filter through the new abstraction. Is this a worthwhile trade-off given the extensibility goals here?

DrahtBot removed the label CI failed on Apr 25, 2026

dergoegge commented at 10:41 AM on April 27, 2026: member

Concept ~0

This seems like a fine change but I don't see the upside in doing it now, unless we also actually add more filter types.

optout21 commented at 2:01 PM on April 30, 2026: contributor

Concept NACK eff833c063f90b7aee8948ac518c82a20983b4f1

While the proposed generalization is architecturally sound, the motivation is not clear. Are there any benefits other than "it makes addition of a future filter easier"? I think such change would be justified only when a proposal for another concrete filter is on the horizon. If you have such, propose it, and then carve out this change as a pre-factor.

sedited commented at 2:08 PM on April 30, 2026: contributor

I'm guessing this was motivated by https://delvingbitcoin.org/t/binary-fuse-filters-as-an-alternative-to-bip-158-gcs/2428/4 . If this a simple refactor that makes working through alternatives easier (haven't taken a look yet), that seem worthwhile. Can you elaborate on your motivation @rustaceanrob?

rustaceanrob commented at 4:01 PM on April 30, 2026: contributor

Indeed I was following up to that delving post with a concrete implementation. This refactor is a step in benchmarking construction and storage of that filter without intrusive modifications to the index. It appears this change is poorly motivated without further demonstration of a new filter, so I can place this as a draft and use it locally for my development branches.

rustaceanrob marked this as a draft on Apr 30, 2026