Skip to content

Comments

fix: handle empty filter topic in ensure_logs_match_filter#517

Merged
ncitron merged 3 commits intoa16z:masterfrom
eshaan7:fix/gh-507-follow-up
Feb 14, 2025
Merged

fix: handle empty filter topic in ensure_logs_match_filter#517
ncitron merged 3 commits intoa16z:masterfrom
eshaan7:fix/gh-507-follow-up

Conversation

@eshaan7
Copy link
Contributor

@eshaan7 eshaan7 commented Feb 13, 2025

This fixes a bug introduced in PR #507.

Changes

  • The ensure_logs_match_filter fn was not handling the null case for filter topics.
  • Removed the unnecessary async on ensure_logs_match_filter fn.

@ncitron
Copy link
Collaborator

ncitron commented Feb 13, 2025

In topic.matches it actually checks if the topic is empty, and if so, considers it a match. This is because an empty topic is effectively a matching topic (the lack of a given topic in a fitler implies that we don't care what it is).

Additionally I think by filtering out empty topics we would be creating a mismatch of indices (although I think since empty topics don't really have a valid serialization it shouldn't matter anyway, but lets supposed someone constructs a filter with an empty topic while calling getLogs while using Helios as a library. This would create a mismatch of indices when we filter out the empty topics, leading to us rejecting a valid filter.

For example [topic0, EMPTY_TOPIC, topic2] filters to [topic0, topic2], which would match with [topic0, topic2], when we would really want it to match with [topic0, ANY_TOPIC, topic2] instead.

Also I think it doesn't make sense to move this into the Proof module, as I'd like to keep this for merkle proof logic only.

Feel free to double check my understanding though, as I don't have that deep knowledge about how log filtering works.

@eshaan7
Copy link
Contributor Author

eshaan7 commented Feb 13, 2025

Additionally I think by filtering out empty topics we would be creating a mismatch of indices

That's a very valid point -- my bad for missing that. Will do a lil more testing.

@eshaan7
Copy link
Contributor Author

eshaan7 commented Feb 13, 2025

@ncitron; Pushed a fix which makes it work with [topic0, null, topic2]. PTAL.

@eshaan7 eshaan7 force-pushed the fix/gh-507-follow-up branch from 7ef504e to 927ed3c Compare February 13, 2025 17:45
@ncitron
Copy link
Collaborator

ncitron commented Feb 13, 2025

So I think no change is needed to handle an empty filter since if you look in the code for the matches method, it returns self.is_empty() || self.0.contains(value) which means our code should handle this already. Additionally as it has been rewritten, it will return that the filter matches as soon as it encounters a single empty topic, which means it won't check for matching any subsequent topics, which is not right.

So I think if I understand correctly, there aren't any code changes needed here (besides maybe moving the check filter method to be a function instead of a method).

@ncitron ncitron closed this Feb 13, 2025
@ncitron ncitron reopened this Feb 13, 2025
@eshaan7
Copy link
Contributor Author

eshaan7 commented Feb 14, 2025

image

If you see the example in this screenshot, the current logic (in master) fails to handle the [topic0, null, topic2] case.

@eshaan7
Copy link
Contributor Author

eshaan7 commented Feb 14, 2025

So I think no change is needed to handle an empty filter since if you look in the code for the matches method, it returns self.is_empty() || self.0.contains(value) which means our code should handle this already.

So this is not working as intended because filter.topics.iter() will always yield 4 topics. So even if the request is [topic0], the Filter struct makes it [topic0, null, null, null]. This means when checking against a log that has less than 4 topics, the code always eventually enters the else case where we return false by default.

To avoid this, we need the is_empty check on filter topic beforehand to basically ignore that index and continue the loop. Pushed this fix in 5cdeedf.

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

Ah this makes sense now.

LGTM!

@ncitron ncitron merged commit 6cb736e into a16z:master Feb 14, 2025
4 of 6 checks passed
@eshaan7 eshaan7 deleted the fix/gh-507-follow-up branch February 15, 2025 08:54
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.

2 participants