refactor: improve and simplify event filters #2140
Conversation
prestwich
requested review from
DaniPopes,
gakonst,
klkvr,
mattsse and
onbjerg
as code owners
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
some qs, hesitant to touch any of the filter logic -.-
| } | ||
| #[cfg(feature = "std")] | ||
| #[cfg_attr(feature = "serde", serde(skip, default))] | ||
| bloom: std::sync::OnceLock<Bloom>, |
There was a problem hiding this comment.
I think we can use once_cell here and get rid of the std features like this once_cell should be already part of the dep graph anyway
There was a problem hiding this comment.
once_cell's no_std support is a little wonky. do you use it with race or critical section?
| } | ||
|
|
||
| /// Returns true if the filter matches the given block number | ||
| pub fn filter_block_range(&self, block_number: u64) -> bool { |
There was a problem hiding this comment.
what's the equivalent of this now?
because we need this on the server side
There was a problem hiding this comment.
matches_block_rangefor numbersmatches_block_hashfor hashesmatches_blockfor numhashes
| /// Support for matching [Filter]s | ||
| #[derive(Debug, Default)] | ||
| pub struct FilteredParams { | ||
| /// The original filter, if any | ||
| pub filter: Option<Filter>, | ||
| } |
There was a problem hiding this comment.
what's the right way to handle an optional filter now?
There was a problem hiding this comment.
an empty Filter via Filter::default() should have the same behavior
There was a problem hiding this comment.
another easy approach would be filter_opt.map(|f| f.matches(thing)).unwrap_or(true)
|
bump |
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
this is def better, only have 1 serde nit
dealing with filters and logs makes me feel dizzy, and I dread touching any of this https://github.com/paradigmxyz/reth/blob/f24570844545b55f2293a3a3c475133a3f738b68/crates/rpc/rpc/src/eth/filter.rs#L523-L525
so I'd need to prep a reth pr first just to make sure we're not missing anything
crates/rpc-types-eth/src/filter.rs
Outdated
| let Some(number) = log.block_number else { return false }; | ||
| let Some(hash) = log.block_hash else { return false }; |
There was a problem hiding this comment.
this will never match for pending logs,
should this behave like an OR
There was a problem hiding this comment.
fixed by adding matches_log_block
| #[cfg_attr(feature = "serde", derive(serde::Deserialize))] | ||
| pub struct FilterSet<T: Eq + Hash> { | ||
| set: HashSet<T>, |
There was a problem hiding this comment.
I believe this deserde impl no longer behaves like before, but unsure where this is even used, but we should treat this as serde transparent
There was a problem hiding this comment.
serde(from = "HashSet<T>") should cover this, ya?
|
@prestwich I haven't started migrating paradigmxyz/reth#16077 but maybe this is actually quite easy |
|
it should be a straightforward migration. I can look more later today if you'd like |
|
that'd be much appreciated |
prestwich
force-pushed
the
prestwich/filter-improvements
branch
from
5f20830 to
427cdee
Compare
* refactor: improve and simplify event filters * docs: minor fixes * feat: mutable filter access with unsealing * fix: cfg_attr * touchup * fix: test * fix: preserve deserialize behavior * fix: matches_log_block and type of rpc_matches_parsed * fix: don't disregard address bloom --------- Co-authored-by: Matthias Seitz <[email protected]>
Motivation
Filtering is a bit wonky and crufty, presumably because the logic was ported from elsewhere.
Solution
FilterSetwhen"std"is enabledBloomFiltertype, use a SINGLE bloom filter perFilterSetinstead ofVec<Bloom>. This appears to have been resource over-use. Blooms are designed to be accruedimpl Defaultas the derived impl has aT: Defaultbound that is unnecessaryFilterSetinnerHashSetprivate, to avoid invalidating memoized bloomsFilterSetdrive-by
PR Checklist