docs: document degraded-filtering fallback path for event topics - #349
docs: document degraded-filtering fallback path for event topics#349joan-bisbal wants to merge 3 commits into
Conversation
| @@ -0,0 +1,3 @@ | |||
| # Event Topic Filtering Optimization | |||
|
|
|||
| Replaces linear topic scans with indexed lookup table for faster event parsing. | |||
There was a problem hiding this comment.
The commit is refactor: optimize event topic filtering logic and the file claims it "replaces linear topic scans with indexed lookup table", but nothing in this diff touches the filtering code — it is a three-line description of a change that was not made.
The actual code is in crates/indexer/src/streamer/mod.rs (plan_filters, and the topic handling in poll_once), with the RPC-side filters in crates/indexer/src/rpc/mod.rs. Note that topic filtering is already pushed server-side into the getEvents RPC call via EventFilter (issue #203), so before optimising anything locally it is worth measuring whether the linear scan is actually on a hot path.
If there is a real win there, a PR containing the code change plus a benchmark or test demonstrating it would be very welcome. Please also target dev rather than main, and drop the UTF-8 BOM.
| @@ -0,0 +1,55 @@ | |||
| # Event Topic Filtering Optimization Specification | |||
There was a problem hiding this comment.
Blocking on this one, and I want to explain the reasoning rather than just flag it.
The title says refactor: optimize event topic filtering logic, but no code changes — the diff is a single new markdown file. That mismatch matters here more than it would for a docs PR, because the title claims a performance change that reviewers and future git-log readers will assume landed. It didn't.
The deeper issue is that the optimization described has already been done, differently and better. See the line comments below.
This isn't a rebase-and-merge situation — the content would need substantive rework to be correct. Happy to give direction if you want to take another run at it; there's a real doc worth writing here about the degraded-filtering fallback path, which is genuinely under-documented.
| - The linear scan $O(N \cdot M)$ over topic arrays is optimized by building an indexed hash lookup table ($O(1)$ constant time lookup). | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
This is the core problem. The doc frames the current implementation as an O(N·M) linear scan that should be replaced with a hash lookup — but that's not what dev does.
crates/indexer/src/streamer/mod.rs already performs server-side filter pushdown: plan_filters() builds a FilterPlan from the contract allowlist, and build_event_filters() (in crates/indexer/src/rpc/filters.rs) compiles it into the filters array of the getEvents RPC request. The RPC never sends events we'd discard, so there is no large in-memory scan to optimize in the normal path.
The remaining client-side check is deliberate, not an oversight — it's the correctness boundary for when server filtering is degraded or when an RPC ignores the filter. Replacing it with the HashSet matcher below would not speed up the hot path, and it discards the fallback semantics.
This is documented already in docs/indexer-event-filtering.md (issue #203), which describes the actual design including the degraded modes.
| ```rust | ||
| use std::collections::HashSet; | ||
|
|
||
| pub struct TopicMatcher { |
There was a problem hiding this comment.
The TopicMatcher here only supports exact-match via HashSet<String>, but the paragraph above it justifies in-memory evaluation by citing "wildcards, regex patterns, or combined contract address filters."
A HashSet cannot express any of those three. So the proposed design doesn't solve the case it's introduced to solve — for wildcard or prefix matching you'd still need a scan or a trie.
Also worth noting: this struct doesn't exist in the codebase. Presenting it as a design spec is fine, but the PR title says refactor:, which implies it was implemented.
|
|
||
| Run benchmarks to evaluate filtering performance: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
This benchmark command can't run:
cargo bench --package trident-indexer --bench topic_filtering
There is no benches/ directory anywhere in the repo and no [[bench]] target named topic_filtering in any Cargo.toml. Anyone following these instructions gets an error.
If the perf claim is central to the doc, it needs an actual bench committed alongside it — otherwise the numbers are unfalsifiable. Please either add the benchmark target or drop this section.
joan-bisbal
changed the title
|
Thanks for the detailed review. Completely reworked this PR:
Ready for re-review. |
2 participants
Documents the degraded-filtering fallback path in the indexer streamer: why the client-side topic check exists as a correctness boundary when RPC server-side pushdown is bypassed, degraded, or returns broader event sets than requested.
Covers how plan_filters() and build_event_filters() push filtering server-side via getEvents, and why the remaining client-side linear scan is deliberate rather than an optimization target.
No code changes -- documentation only.