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

fix: update peers ENRs in peer store in case they are updated #2818

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Jun 18, 2024

Description

Checking if the ENR of an incoming peer is the same as the one we have saved and if not, update it in the peer store.

Changes

  • updating peer store for existing peers with changed ENRs

Issue

closes #2817

Copy link

github-actions bot commented Jun 18, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2818-rln-v2

Built from 3623419

Copy link

github-actions bot commented Jun 18, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2818-rln-v1

Built from 3623419

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

PR looks good to me, but I wonder if it could be the case that when discovering you receive the same peer but with older information. Since we are not looking at the sequence number of the enr, we might end up overriding the peer information with outdated info

@richard-ramos
Copy link
Member

Ah do note that this is a scenario that i have seen in go-waku. I do not know if such thing can happen in the nim implementation

@gabrielmer
Copy link
Contributor Author

PR looks good to me, but I wonder if it could be the case that when discovering you receive the same peer but with older information. Since we are not looking at the sequence number of the enr, we might end up overriding the peer information with outdated info

Oh great catch! So basically before updating the ENR, we have to also check for the seqNum to be equal or greater than the one in the ENR currently saved in the peer store?

If that makes sense, will implement that fix too :)

@richard-ramos
Copy link
Member

Oh great catch! So basically before updating the ENR, we have to also check for the seqNum to be equal or greater than the one in the ENR currently saved in the peer store?

Yes, that's what we do in go-waku! we check if the sequence number is greater to update the peer info!

@gabrielmer
Copy link
Contributor Author

Yes, that's what we do in go-waku! we check if the sequence number is greater to update the peer info!

Done! Updated the PR, lmk in case there's something wrong with the fix.

Thanks so much!

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.

LGTM! Thanks! 💯

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
@gabrielmer gabrielmer merged commit cda18f9 into master Jun 19, 2024
12 of 15 checks passed
@gabrielmer gabrielmer deleted the chore-update-peer-if-changed-enr branch June 19, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: peerStore should update ENRs of existing peers in case they change
3 participants