Skip to content

Decouple BlockFilter from GCS implementation#35158

Open
rustaceanrob wants to merge 2 commits intobitcoin:masterfrom
rustaceanrob:26-4-24-filter-abs
Open

Decouple BlockFilter from GCS implementation#35158
rustaceanrob wants to merge 2 commits intobitcoin:masterfrom
rustaceanrob:26-4-24-filter-abs

Conversation

@rustaceanrob
Copy link
Copy Markdown
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.

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.
@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Apr 25, 2026

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

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
Copy link
Copy Markdown
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).

Hints

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.

This commit allows for `BlockFilter` to hold any type that implements
the properties of a block filter, rather than concretely holding a GCS
filter.
Comment thread src/blockfilter.h
public:

BlockFilter() = default;
BlockFilter() : m_filter(std::make_unique<GCSFilter>()) {}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants