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: upgrade peer exchange mounting #2953

Merged
merged 3 commits into from
Aug 6, 2024
Merged

chore: upgrade peer exchange mounting #2953

merged 3 commits into from
Aug 6, 2024

Conversation

darshankabariya
Copy link
Contributor

This PR upgrades the mounting scheme for peer_exchange. With this update, a specific node address is no longer required when mounting peer_exchange from the node side.

Copy link

github-actions bot commented Aug 5, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2953

Built from 46e61d0

@gabrielmer
Copy link
Contributor

Not sure I understand the change.

Before this change there were 2 cases:

  1. only peerExchangeNode was provided and in that case, we mounted the protocol and set the service peer with the address provided
  2. peerExchange is set to true but without a peerExchangeNode provided, in which case we mount the protocol but don't set a service peer
  3. none of them are set to true and we don't mount peer exchange

Not sure what's wrong with that logic, maybe I'm missing something? But I see that with the changes now, we are eliminating the possibility of configuring the node as a peer exchange client

@darshankabariya
Copy link
Contributor Author

Not sure I understand the change.

Before this change there were 2 cases:

  1. only peerExchangeNode was provided and in that case, we mounted the protocol and set the service peer with the address provided
  2. peerExchange is set to true but without a peerExchangeNode provided, in which case we mount the protocol but don't set a service peer
  3. none of them are set to true and we don't mount peer exchange

Not sure what's wrong with that logic, maybe I'm missing something? But I see that with the changes now, we are eliminating the possibility of configuring the node as a peer exchange client

This change is based on the chat in this Discord thread. Previously, nodes (server-side) would only add the node address if it provided the peer exchange service. Now, that's not necessary; any client can ask any node that provides the peer exchange service. The old method was somewhat permission-based, where only the node could decide who received the peer exchange service. The new way is more permissionless.

Any suggestions or corrections are appreciated, @gabrielmer.
Please let me know if I misunderstood anything, @Ivansete-status.

@gabrielmer
Copy link
Contributor

But we currently don't need a peerExchangeNode in order to mount peer exchange. The condition to mount it is

if conf.peerExchangeNode != "" or conf.peerExchange:

The condition to require a peerExchangeNode to mount it would be with an and instead of an or. But with the or, if there's no peerExchangeNode and peerExchange is true then we do mount it.

Again, I might be missing something 😶

@darshankabariya
Copy link
Contributor Author

But we currently don't need a peerExchangeNode in order to mount peer exchange. The condition to mount it is

if conf.peerExchangeNode != "" or conf.peerExchange:

The condition to require a peerExchangeNode to mount it would be with an and instead of an or. But with the or, if there's no peerExchangeNode and peerExchange is true then we do mount it.

Again, I might be missing something 😶

I just updated the PR. Can you please look into it and try isolating both things so nothing hurts 😊?

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.

Looks reasonable to me. :)

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.

LGTM! Thanks so much!

@darshankabariya darshankabariya merged commit 42f1bed into master Aug 6, 2024
9 of 11 checks passed
@darshankabariya darshankabariya deleted the chore_px branch August 6, 2024 07:57
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.

3 participants