fix: filter - enhancements in subscription management #3198
Conversation
* waku_filter_v2: idiomatic way run periodic subscription manager * filter subscriptions: add more debug logs * filter: make sure the custom start and stop procs are called * make sure filter protocol is started if it is mounted * filter: dial push connection onsubscribe only
Ivansete-status
changed the title
|
You can find the image built from this PR at Built from 9064bb8 |
Ivansete-status
requested review from
NagyZoltanPeter,
richard-ramos,
darshankabariya and
gabrielmer
waku/waku_peer_exchange/protocol.nim
Outdated
| waku_px_errors.inc(labelValues = [exc.msg]) | ||
| callResult = ( | ||
| status_code: PeerExchangeResponseStatusCode.SERVICE_UNAVAILABLE, | ||
| status_desc: some($exc.msg), | ||
| ) | ||
| finally: | ||
| debug "JJJJ" |
There was a problem hiding this comment.
We probably need to remove this, although I liked more when we used jamon for random debug logs 😆
waku/waku_peer_exchange/protocol.nim
Outdated
| @@ -272,6 +286,7 @@ proc initProtocolHandler(wpx: WakuPeerExchange) = | |||
| ).isOkOr: | |||
| error "Failed to respond with TOO_MANY_REQUESTS:", error = $error | |||
| # close, no data is expected | |||
| debug "JJJJ" | |||
There was a problem hiding this comment.
I totally agree on re-thinking how we manage message-push and its requirements.
Can you add some explanation how this solution exactly solves the issue appeared for Railgun? I see some hole in the changed connection management as is now.
Can we brainstorm on this?
|
|
||
| if not wf.subscriptions.isSubscribed(peerId): | ||
| debug "pinging peer has no subscriptions", peerId = peerId | ||
| error "pinging peer has no subscriptions", peerId = peerId |
There was a problem hiding this comment.
I'm not pretty sure if it is a real error here. I think it can be a real scenario when a client first pings than start subscribe in case of negative answer. Like edge nodes go sleep and when come back it starts a ping to check if it needs to renew the subs or not.
| # TODO: check if this condition is valid??? | ||
| if pubsubTopic.isNone() or contentTopics.len == 0: | ||
| error "pubsubTopic and contentTopics must be specified", peerId = peerId |
There was a problem hiding this comment.
Hm, similarly here, this is an error response here but it is not an error (maybe a warn that the service might be configured with larger pools).
| return err( | ||
| FilterSubscribeError.badRequest("pubsubTopic and contentTopics must be specified") | ||
| ) | ||
|
|
||
| if contentTopics.len > MaxContentTopicsPerRequest: | ||
| error "exceeds maximum content topics", peerId = peerId |
| @@ -236,7 +250,7 @@ proc handleMessage*( | |||
| let subscribedPeers = | |||
| wf.subscriptions.findSubscribedPeers(pubsubTopic, message.contentTopic) | |||
| if subscribedPeers.len == 0: | |||
| trace "no subscribed peers found", | |||
| error "no subscribed peers found", | |||
There was a problem hiding this comment.
I don't think it is an error if there is noone that subscribed to that message. Even further in general it is an annoying log as trace. Rather we log only positives in such cases.
| return err("node has reached maximum number of subscriptions") | ||
| return err("node has reached maximum number of subscriptions: " & $(s.maxPeers)) | ||
|
|
||
| let connRes = await peerManager.dialPeer(peerId, WakuFilterPushCodec) |
There was a problem hiding this comment.
I see the intent to avoid unnecessary dials over message push cycles, although opening a stream at subscribe seems to me a over optimization + an attack vector where a harmful actor may subscribe to filters will never match and eat up our connections pool.
Also it can be that a subscription may not result in push in a close future without any intent to harm the service.
| # Protocol specifies no response for now | ||
| return | ||
| ## Notice that the client component is acting as a server of WakuFilterPushCodec messages | ||
| while not conn.atEof(): |
There was a problem hiding this comment.
How exactly this helps on being a server like? I though it is libp2p duty to maintain it.
Description
Railgun notified about filter issues. This PR tackled that.
Railgun informed that after 4 days working properly, the nodes stopped sending messages. There were also issues that caused a max of 100 messages being sent over one connection and the reason for that was that we were
dialPeering on every single message the filter server sent to the filter subscribers.Changes
How to test
Issue
closes #3191
closes #3192