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: ENR can now be updated #1875

Merged
merged 1 commit into from
Aug 23, 2023
Merged

feat: ENR can now be updated #1875

merged 1 commit into from
Aug 23, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Aug 2, 2023

Description

I added a way to update the Discv5 ENR and used a AsyncEventQueue to broadcast topic subscriptions.

Changes

  • Discv5 add/remove shard from ENR
  • event system for topic subscriptions

Fix #1843

Followed by #1918

@SionoiS SionoiS linked an issue Aug 2, 2023 that may be closed by this pull request
1 task
@SionoiS SionoiS self-assigned this Aug 2, 2023
@github-actions
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1875

@SionoiS SionoiS marked this pull request as ready for review August 17, 2023 18:52
apps/wakunode2/app.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Approving to not be a bottleneck, but I'm not sure I agree with passing in discv5 into the API. The API should have no knowledge about discovery in general. Wouldn't it be possible (and simpler) to just have a "watcher" task on wakunode2/app.nim that monitors the subscribed topics on the node and triggers the discv5 ENR update when it notices a new/removed topic?

waku/waku_enr/sharding.nim Outdated Show resolved Hide resolved
waku/waku_discv5.nim Outdated Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 21, 2023

Approving to not be a bottleneck, but I'm not sure I agree with passing in discv5 into the API. The API should have no knowledge about discovery in general. Wouldn't it be possible (and simpler) to just have a "watcher" task on wakunode2/app.nim that monitors the subscribed topics on the node and triggers the discv5 ENR update when it notices a new/removed topic?

Yes it is a weird thing to do. I don't like your watcher idea either. Why watch for changes when the info could be sent directly.

I'll try to find something better. I wish I was more knowledgeable about NIm :(

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 22, 2023

I discussed with @richard-ramos go-waku way of sending the info. from the relay api to various waku sub-systems and it seams like more or less like I would have done it in Rust.

I still don't know what to do in Nim thought...

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 22, 2023

OK!

I'm now satisfied with this solution and we can expand upon it later too.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Is this PR duplicated? I feel like a dejavu :)
( #1918 )

tests/wakunode_rest/test_rest_relay.nim Outdated Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 23, 2023

Is this PR duplicated? I feel like a dejavu :) ( #1918 )

Yeah sorry should not have ask to review the follow-up PR before merging this.

@SionoiS SionoiS removed the request for review from vpavlin August 23, 2023 11:54
@github-actions
Copy link

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

@SionoiS SionoiS merged commit c369b32 into master Aug 23, 2023
15 checks passed
@SionoiS SionoiS deleted the enr branch August 23, 2023 13:53
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.

feat: update ENR based on RPC topic subscribe
4 participants