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

chore: filter remove all subscription from a peer that is leaving #3267

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented Jan 30, 2025

Description

@fbarbu15 reported that waku-interop-tests started to fail.
The problem appeared when one filter-client completely disconnects, e.g., node shutdown. In this case, the waku_filter_2/subscriptions.nim module was still keeping an incorrect Connection instance.

  • waku/waku_filter_v2/protocol.nim starts keeping track of the filter-client's Connections in peerConnections: Table[PeerId, Connection]
  • waku/waku_filter_v2/protocol.nim adds a new Connection when a new message needs to be pushed to filter-client.
  • waku/waku_filter_v2/protocol.nim listens to filter-client-peer disconnect events, in which case, the previous Connection object is dropped.

Issue

closes: #3263
closes: #3254

Copy link

github-actions bot commented Jan 30, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3267

Built from 908cd20

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

I think we miss adding new connection to our table.

waku/waku_filter_v2/protocol.nim Outdated Show resolved Hide resolved
waku/waku_filter_v2/protocol.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

I would consider not make pushToPeers sequential, but the other seems ok!

waku/waku_filter_v2/protocol.nim Outdated Show resolved Hide resolved
@Ivansete-status Ivansete-status marked this pull request as ready for review January 31, 2025 14:34
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Ooh amazing PR! Thanks so much! 🤩

@Ivansete-status Ivansete-status merged commit 46747fd into master Jan 31, 2025
11 of 14 checks passed
@Ivansete-status Ivansete-status deleted the filter-peer-left-fix branch January 31, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants