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: update ENR based on RPC topic subscribe #1843

Closed
1 task done
SionoiS opened this issue Jul 5, 2023 · 5 comments · Fixed by #1875
Closed
1 task done

feat: update ENR based on RPC topic subscribe #1843

SionoiS opened this issue Jul 5, 2023 · 5 comments · Fixed by #1875
Assignees
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details

Comments

@SionoiS
Copy link
Contributor

SionoiS commented Jul 5, 2023

Problem

Subscribing to a new topics via RPC does not update the ENR when using shards.

Suggested solution

discv5 has a way to update the record. Create the same in waku discv5.

Alternatives considered

Restarting the node with new topics always works but is annoying.

Additional context

https://github.com/status-im/nim-eth/blob/285da12bf318a2e21182dc2453a4a30c58f73067/eth/p2p/discoveryv5/protocol.nim#L247

Acceptance criteria

  • Subbing to a shard topic like /waku/2/rs/16/0 via RPC should update the ENR in waku discv5.
@SionoiS SionoiS moved this to Priority in Waku Jul 25, 2023
@vpavlin vpavlin added this to the Release 0.20.0 milestone Aug 1, 2023
@SionoiS SionoiS self-assigned this Aug 1, 2023
@SionoiS SionoiS linked a pull request Aug 2, 2023 that will close this issue
2 tasks
@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 2, 2023

A bit of a conundrum here. Discv5 is no longer in waku_node but rpc handlers call node.subscribe()

It seams like ALL protocols must be in waku_node otherwise we can't access them.

Architecturally, sending information from rpc handlers to whichever protocol needs it in an abstract way (channels?) would be best.

The alternative would be to pass app to the rpc handlers!?

@alrevuelta
Copy link
Contributor

alrevuelta commented Aug 7, 2023

good catch. random idea. perhaps some kind of action can be triggered after subs/unsubs to new topics. like this we can avoid this depencancy. some hook?

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 16, 2023

I talked with @fryorcraken and I guess there's 3 options

  1. Don't allow live subscriptions. (remove the API?)
  2. Allow but full reset of all connections (shotgun approach!)
  3. Design and impl. a smarter system (Is there any benefit?)

The decision will affect peer management.

This seams to be less related to autosharding so I changed the epic.

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 18, 2023

I decided to go with option 4. which is just update the discv5 ENR and don't change anything else.

@github-project-automation github-project-automation bot moved this from Priority to Done in Waku Aug 23, 2023
@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 23, 2023

For topic subscriptions we are now using an event system.

@fryorcraken fryorcraken added E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details and removed E:2023-peer-mgmt labels Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants