refactor(rpc): simplify the inner definitions of topics & address filters #3876

ngotchac · 2023-07-23T10:36:30Z

As suggested here: #3856 (comment) ; we could use simpler definitions for the filters (topics and address) in rpc.

A new type struct FilterSet<T: Eq + Hash>(HashSet<T>) has been setup, that is used by both Address and Topic filters. An address filter has type address: FilterSet<Address>, without any need for Option<...>. If the FilterSet is empty, it means that there is no address filter. It includes several utility functions:

matches to check if the given value: &T matches the current filter
to_bloom_filter to create a list of bloom filters
to_value_or_array for serialization.

Based on the definition of filters in https://ethereum.github.io/execution-apis/api-documentation/, the only case that isn't taken into account is null within a filter set, eg: topics: [[null], [0x...]] ; but I don't see how this can be useful.

mattsse

this simplifies it a lot but

the only case that isn't taken into account is null within a filter set, eg: topics: [[null], [0x...]] ; but I don't see how this can be useful.

would be a deal breaker because the spec allows this

does this mean this pr can't handle array topics with [null]?

ngotchac · 2023-07-24T12:20:20Z

would be a deal breaker because the spec allows this

Which spec though? In https://ethereum.github.io/execution-apis/api-documentation/ the definition of topics is:

So "topic": [null, ...] is allowed and fine ; but "topics":[[null], "0x1234..."] isn't, and should just be "topics":[[], "0x1234..."] or "topics":[null, "0x1234..."]

does this mean this pr can't handle array topics with [null]?

Cf. the examples above. Not that it would be easy to add such cases, but I thought we'd rather want to stick to "some" spec.

mattsse · 2023-07-24T12:31:28Z

crates/rpc/rpc-types/src/eth/filter.rs

+#[derive(Default, Debug, PartialEq, Eq, Clone, Deserialize)]
+/// FilterSet is a set of values that will be used to filter logs
+pub struct FilterSet<T: Eq + Hash>(HashSet<T>);


one problem here is that this type is not necessarily symmetric, so serde can lead to a different order.

I'm not sure how to correctly solve this besides making this a Vec<, which is probably fine since we don't expect this these to be more than a few items?

Yep, only drawback is having a worst inclusion check. This could be balanced by having a custom serialization that orders the Vec in ascending order? I'm quite sure how this would be an issue though?

mattsse · 2023-07-24T12:31:58Z

right,

looks like we supported optional array values even though null was never a valid value in the nested array:

so If I read this correctly then

[null,[null,A]] should not be supported?, only [null,[A]]

ngotchac · 2023-07-24T12:34:05Z

OK so I took a look at geth's implementation of decoding events: https://github.com/ethereum/go-ethereum/blob/a196f3e8a22b6ad22ced5c2e3baf32bc3ebd4ec9/eth/filters/api.go#L540-L579

It seems that if there's a single null value within a single topic filter (eg [[null, 0x12345...], ...]), it treats the whole single topic as "match all". Should we replicate that?

mattsse · 2023-07-24T12:37:28Z

OK so I took a look at geth's implementation of decoding events: https://github.com/ethereum/go-ethereum/blob/a196f3e8a22b6ad22ced5c2e3baf32bc3ebd4ec9/eth/filters/api.go#L540-L579

It seems that if there's a single null value within a single topic filter (eg [[null, 0x12345...], ...]), it treats the whole single topic as "match all". Should we replicate that?

this is so bad -.-
so [null, ..] are valid rpc params but nulls are just ignored?

ngotchac · 2023-07-24T12:42:55Z

Yeah it's bad aha! So my understanding is that, given that the topics are converted into some sort of Vec<Vec<H256>> (or Vec<HashSet<H256>>):

[null, [0x1234]] -> [ [], [0x1234] ]
[[], [0x1234]] -> [ [], [0x1234] ]
[ [0x56789, null], [0x1234] ] -> [ [], [0x1234] ]

So the null in the last case acts as a wildcard for the whole single-topic filter, and skips the 0x56789 one completely. I'm not sure what's the intention there, but alas...

mattsse · 2023-07-24T12:53:23Z

[ [0x56789, null], [0x1234] ] -> [ [], [0x1234] ]

Perhaps the reasoning for this is, arrays or OR concatenations so if there's a null that this essentially matches anything and can be simplified to just [], which is [null]

ngotchac · 2023-07-24T12:55:17Z

Perhaps the reasoning for this is, arrays or OR concatenations so if there's a null that this essentially matches anything and can be simplified to just [], which is [null]

Yeah, sounds reasonable when said like this, but no sane human or framework would allow building such a query: match (A OR anything) 😄

mattsse · 2023-07-24T12:55:50Z

yeah...

sigh

error: incorrect implementation of `clone` on a `Copy` type cf. paritytech/parity-common#767

Use a new FilterSet<T> type for filters (each topic and address) that gets serialized/de-serialized from/to the expected type. This simplifies the matches logic. BloomFilters are also simplified, removing the need to use an `Option`.

ngotchac · 2023-07-27T13:53:50Z

I update my PR to take into account the null values within an array, treated as an overwriting wildcard for the whole array (with a test).

Note that I had to add another temporary commit updating fixed-hash in order for clippy linting to pass.

mattsse

this is great!

will take a look at the fixed hash thing separately.
rolled the fixed-hash dep back because we track this #3544

codecov · 2023-07-28T10:59:34Z

Codecov Report

Merging #3876 (8677c13) into main (d2cdd10) will increase coverage by 0.03%.
The diff coverage is 87.43%.

Files Changed	Coverage Δ
crates/rpc/rpc/src/eth/filter.rs	`17.66% <0.00%> (+0.12%)`	⬆️
crates/rpc/rpc-types/src/eth/filter.rs	`74.68% <87.83%> (+4.22%)`	⬆️
crates/rpc/rpc-types/src/eth/pubsub.rs	`35.18% <100.00%> (ø)`

... and 7 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.38% <0.00%> (-0.01%)`	⬇️
unit-tests	`64.46% <87.43%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`27.16% <ø> (ø)`
blockchain tree	`83.04% <ø> (ø)`
pipeline	`89.82% <ø> (ø)`
storage (db)	`74.30% <ø> (ø)`
trie	`94.70% <ø> (ø)`
txpool	`45.58% <ø> (ø)`
networking	`77.61% <ø> (-0.04%)`	⬇️
rpc	`58.62% <87.43%> (+0.17%)`	⬆️
consensus	`64.46% <ø> (ø)`
revm	`33.68% <ø> (ø)`
payload builder	`6.61% <ø> (ø)`
primitives	`87.82% <ø> (ø)`

ngotchac requested review from mattsse, Rjected and gakonst as code owners July 23, 2023 10:36

onbjerg changed the title ~~rpc: Simplify the inner definitions of Topics & Address filters~~ refactor(rpc): simplify the inner definitions of topics & address filters Jul 24, 2023

onbjerg added C-debt Refactor of code section that is hard to understand or maintain A-rpc Related to the RPC implementation labels Jul 24, 2023

mattsse reviewed Jul 24, 2023

View reviewed changes

[TMP] Update fixed-hash dependency to fix Clippy error

028a718

error: incorrect implementation of `clone` on a `Copy` type cf. paritytech/parity-common#767

ngotchac force-pushed the ngotchac/fix-events-topics-filter branch from 5c40853 to e068cdc Compare July 27, 2023 09:07

ngotchac force-pushed the ngotchac/fix-events-topics-filter branch from e068cdc to 1c1f3b4 Compare July 27, 2023 13:51

mattsse approved these changes Jul 28, 2023

View reviewed changes

mattsse added 2 commits July 28, 2023 12:47

Merge branch 'main' into ngotchac/fix-events-topics-filter

3b8b5a1

clippy

8677c13

mattsse enabled auto-merge July 28, 2023 10:52

mattsse added this pull request to the merge queue Jul 28, 2023

Merged via the queue into paradigmxyz:main with commit 0892833 Jul 28, 2023

ngotchac deleted the ngotchac/fix-events-topics-filter branch July 28, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(rpc): simplify the inner definitions of topics & address filters #3876

refactor(rpc): simplify the inner definitions of topics & address filters #3876

ngotchac commented Jul 23, 2023

mattsse left a comment

ngotchac commented Jul 24, 2023

mattsse Jul 24, 2023

ngotchac Jul 24, 2023

mattsse commented Jul 24, 2023 •

edited

Loading

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023 •

edited

Loading

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023

ngotchac commented Jul 27, 2023

mattsse left a comment •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

refactor(rpc): simplify the inner definitions of topics & address filters #3876

refactor(rpc): simplify the inner definitions of topics & address filters #3876

Conversation

ngotchac commented Jul 23, 2023

mattsse left a comment

Choose a reason for hiding this comment

ngotchac commented Jul 24, 2023

mattsse Jul 24, 2023

Choose a reason for hiding this comment

ngotchac Jul 24, 2023

Choose a reason for hiding this comment

mattsse commented Jul 24, 2023 • edited Loading

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023 • edited Loading

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023

ngotchac commented Jul 24, 2023

mattsse commented Jul 24, 2023

ngotchac commented Jul 27, 2023

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

mattsse commented Jul 24, 2023 •

edited

Loading

mattsse commented Jul 24, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading