Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate new filter protocol, other improvements #1637

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

jm-clius
Copy link
Contributor

What is in this PR?

This PR does the following:

  • integrates the new filter service protocol in the waku node
  • extend test cases to include error conditions
  • some improvements to metrics, maintenance and monitoring

What about the legacy filter protocol?

For now, when filter is configured, the node will mount both the new and legacy filter protocols. This is to maintain backwards compatibility until filter clients have dogfooded and switched to the new filter protocol. The idea is to remove all legacy code in the future.

What about the nwaku filter client?

Note that the new filter client has not been integrated yet. Since nwaku is rarely used as a filter client, this client protocol can be integrated and the Filter APIs updated once dogfooding has progressed without supporting the legacy filter client.

@jm-clius
Copy link
Contributor Author

jm-clius commented Apr 3, 2023

Update: although tests pass locally, they fail during CI due to a segfault getting triggered on the underlying peerStore attempting to access its peerStore.books field. At first glance this should be impossible, but I'm investigating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace changes only. Hide whitespace in GH settings to ignore this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hide whitespace in GH settings to view only effective changes in this file.

@jm-clius
Copy link
Contributor Author

jm-clius commented Apr 4, 2023

Update: in order to get rid of segfaults and corrupted memory I had to

  • explicitly cancel any scheduled timer tasks
  • remove any ambiguity in LPProtocol symbols (e.g. renaming the old WakuFilter to WakuFilterLegacy, as no amount of import path qualifications would not lead to memory corruption)

@jm-clius jm-clius marked this pull request as ready for review April 4, 2023 16:19
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! new filter protocol looking great!

Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

LGTM ✅ 💯

@jm-clius jm-clius merged commit 418efca into master Apr 11, 2023
@jm-clius jm-clius deleted the feat/filter-v2-integrate branch April 11, 2023 08:12
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