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: add filter v2 rpc #798

Merged
merged 7 commits into from
Oct 26, 2023
Merged

feat: add filter v2 rpc #798

merged 7 commits into from
Oct 26, 2023

Conversation

harsh-98
Copy link
Contributor

@harsh-98 harsh-98 commented Oct 9, 2023

ping, subscribe/unsubscribe and unsubscribeAll.

Description

Filter rest endpoints for #746 . Getting current messages is not implemented.

Changes

  • Add rest codebase for filterv2 endpoints ping, subscribe/unsubscribe and unsubscribeAll.
  • Fix writeResponse. http.ResponseWriter doesn't support writing multiple statusCode from 2xx-5xx. w.Write implicitly write http.StatusOK. Other code can't be written.
  • Fix fix(filter-subscribe): params.selectedPeer not set #836
  • Change ping function on filter client pass PingOptions for requestID.

Tests

  • Write test for pingFailure, subscribe and then check if ping works, subscribe/unsubscribe and multiple subscription then unsubscribeAll.

TODO

  • Separate PR for supporting getting messages from filter client.

@harsh-98
Copy link
Contributor Author

harsh-98 commented Oct 9, 2023

@richard-ramos rest endpoints are internally using filterLightNode. how to select the peer for all the endpoints?

What is the peerSelection strategy to use? Rest request only have requestId, peer selection is internal to the node.

@status-im-auto
Copy link

status-im-auto commented Oct 9, 2023

Jenkins Builds

Click to see older builds (30)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c6ab16b #1 2023-10-09 21:17:34 ~2 min nix-flake 📄log
✔️ c6ab16b #1 2023-10-09 21:17:47 ~2 min linux 📦deb
✔️ c6ab16b #1 2023-10-09 21:18:40 ~3 min tests 📄log
✔️ c6ab16b #1 2023-10-09 21:18:47 ~3 min tests 📄log
✔️ c6ab16b #1 2023-10-09 21:19:35 ~4 min android 📦tgz
✔️ c6ab16b #1 2023-10-09 21:20:42 ~5 min ios 📦tgz
✖️ 5a6d585 #2 2023-10-25 10:04:07 ~1 min tests 📄log
✖️ 5a6d585 #2 2023-10-25 10:04:09 ~1 min tests 📄log
✔️ 5a6d585 #2 2023-10-25 10:04:44 ~2 min nix-flake 📄log
✔️ 5a6d585 #2 2023-10-25 10:05:05 ~2 min linux 📦deb
✔️ 5a6d585 #2 2023-10-25 10:06:49 ~4 min android 📦tgz
✔️ 5a6d585 #2 2023-10-25 10:07:51 ~5 min ios 📦tgz
✔️ dbfb287 #3 2023-10-25 10:12:41 ~1 min nix-flake 📄log
✔️ dbfb287 #3 2023-10-25 10:13:19 ~2 min linux 📦deb
✔️ dbfb287 #3 2023-10-25 10:14:17 ~3 min ios 📦tgz
✔️ dbfb287 #3 2023-10-25 10:14:20 ~3 min tests 📄log
✔️ dbfb287 #3 2023-10-25 10:14:24 ~3 min tests 📄log
✔️ dbfb287 #3 2023-10-25 10:14:38 ~3 min android 📦tgz
✖️ 457b048 #4 2023-10-25 21:56:54 ~25 sec tests 📄log
✖️ 457b048 #4 2023-10-25 21:57:27 ~59 sec nix-flake 📄log
457b048 #4 2023-10-25 21:57:30 ~1 min linux 📄log
✖️ 457b048 #4 2023-10-25 21:58:05 ~1 min tests 📄log
✔️ 457b048 #4 2023-10-25 21:59:48 ~3 min android 📦tgz
✔️ 457b048 #4 2023-10-25 22:00:22 ~3 min ios 📦tgz
✔️ eea12f8 #5 2023-10-25 22:03:38 ~1 min linux 📦deb
✔️ eea12f8 #5 2023-10-25 22:04:42 ~2 min tests 📄log
✔️ eea12f8 #5 2023-10-25 22:04:56 ~2 min tests 📄log
✔️ eea12f8 #5 2023-10-25 22:05:40 ~3 min android 📦tgz
✔️ eea12f8 #5 2023-10-25 22:06:12 ~3 min nix-flake 📄log
✔️ eea12f8 #5 2023-10-25 22:06:19 ~3 min ios 📦tgz
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5f2c483 #6 2023-10-26 04:41:33 ~1 min linux 📦deb
✔️ 5f2c483 #6 2023-10-26 04:41:50 ~2 min nix-flake 📄log
✔️ 5f2c483 #6 2023-10-26 04:42:04 ~2 min tests 📄log
✔️ 5f2c483 #6 2023-10-26 04:42:10 ~2 min tests 📄log
✔️ 5f2c483 #6 2023-10-26 04:43:56 ~4 min android 📦tgz
✔️ 5f2c483 #6 2023-10-26 04:44:12 ~4 min ios 📦tgz
✔️ a2cff9f #7 2023-10-26 23:16:34 ~1 min linux 📦deb
✔️ a2cff9f #7 2023-10-26 23:17:21 ~1 min nix-flake 📄log
✔️ a2cff9f #7 2023-10-26 23:18:59 ~3 min tests 📄log
✔️ a2cff9f #7 2023-10-26 23:19:09 ~3 min tests 📄log
✔️ a2cff9f #7 2023-10-26 23:19:28 ~4 min android 📦tgz
✔️ a2cff9f #7 2023-10-26 23:19:42 ~4 min ios 📦tgz

@richard-ramos
Copy link
Member

Peer selection is done automatically. Look here how it is done in nwaku:
https://github.com/waku-org/nwaku/blob/880d018eaf9fbc8c94bb0da394140993f8d94e8c/waku/node/rest/filter/handlers.nim#L162-L169

Here's the relevant logic for how peers are selected https://github.com/waku-org/nwaku/blob/d5c3ade5e21eafe17ca1c3b6e43e0563df923beb/waku/node/peer_manager/peer_manager.nim#L700-L707 do note that service slots have priority. If there are no peers in the service slots, then a random peer with filter protocol support is selected

@harsh-98
Copy link
Contributor Author

Peer selection is done automatically. Look here how it is done in nwaku: https://github.com/waku-org/nwaku/blob/880d018eaf9fbc8c94bb0da394140993f8d94e8c/waku/node/rest/filter/handlers.nim#L162-L169

Here's the relevant logic for how peers are selected https://github.com/waku-org/nwaku/blob/d5c3ade5e21eafe17ca1c3b6e43e0563df923beb/waku/node/peer_manager/peer_manager.nim#L700-L707 do note that service slots have priority. If there are no peers in the service slots, then a random peer with filter protocol support is selected

Two questions:

  • For subscription, we are selecting a random peer and creating connection with it for given pubSub and ContentTopics. BUt for unsubscribe/UnsubscribeAll we are again selecting a randomPeer. What is the guarantee that random Peer will have a connection for given pubSub and ContentTopics?

  • In nwaku rest/filter/handler peerManager is exposed to the handler. Where as in gowaku filter_v2 it is used internally via AutomaticPeerSelection FilterOpts. In gowaku WakuNode doesn't have method for returning the PeerManager so how can i use it in rest/filter.go for ping endpoint. Ping Endpoint requires the peerId not FilterOpts.

@richard-ramos
Copy link
Member

Hm. Indeed, this seems to me to something that the filter REST API might be missing. Unsubscribing should be associated to an specific peer, or maybe, to all peers that you're subscribed to, while the way it's implemented in nwaku seems to not be deterministic https://github.com/waku-org/nwaku/blob/880d018eaf9fbc8c94bb0da394140993f8d94e8c/waku/node/rest/filter/handlers.nim#L251

cc: @NagyZoltanPeter for insights.

@richard-ramos
Copy link
Member

The same applies to Ping. The request seems to be missing the peerId.

@harsh-98 harsh-98 self-assigned this Oct 25, 2023
@harsh-98 harsh-98 marked this pull request as ready for review October 25, 2023 10:14
waku/v2/node/wakuoptions.go Outdated Show resolved Hide resolved
waku/v2/protocol/filter/client.go Outdated Show resolved Hide resolved
waku/v2/protocol/filter/client.go Outdated Show resolved Hide resolved
cmd/waku/server/rest/filter.go Outdated Show resolved Hide resolved
cmd/waku/server/rest/filter.go Show resolved Hide resolved
cmd/waku/server/rest/filter.go Show resolved Hide resolved
cmd/waku/server/rest/filter.go Show resolved Hide resolved
cmd/waku/server/rest/filter.go Show resolved Hide resolved
cmd/waku/server/rest/filter_api.yaml Show resolved Hide resolved
cmd/waku/server/rest/filter_api.yaml Show resolved Hide resolved
Copy link
Collaborator

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

LGTM

@harsh-98 harsh-98 merged commit 0868f5d into master Oct 26, 2023
2 checks passed
@harsh-98 harsh-98 deleted the Filterv2REST branch October 26, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants