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: peer-exchange issue #2889

Merged
merged 4 commits into from
Aug 23, 2024
Merged

fix: peer-exchange issue #2889

merged 4 commits into from
Aug 23, 2024

Conversation

darshankabariya
Copy link
Contributor

@darshankabariya darshankabariya commented Jul 9, 2024

closes #2414

This implementation will enhance the reliability of the peers shared by the peer-exchange protocol with its clients. because now the cache updates every 10 min.

After the resolution of the peer_manager issue #2905, the reliability of peer exchanges has also been improved.

Copy link

github-actions bot commented Jul 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2889

Built from faee769

@darshankabariya
Copy link
Contributor Author

darshankabariya commented Jul 9, 2024

After understanding the discussion in issue #2414, try to implement @alrevuelta's proposed solution. Please review the code.

Is it possible to test this complex scenario in a unit test?
I chose three hours. Is there any comment on reduce or increase that span ?

cc @gabrielmer @chaitanyaprem

@darshankabariya darshankabariya changed the title fix: initial implement for peer-exchange fix: peer-exchange issue Jul 9, 2024
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! 💯
I just added a naïve question :)

@@ -140,8 +145,10 @@ proc getPeersByProtocol*(peerStore: PeerStore, proto: string): seq[RemotePeerInf
return peerStore.peers.filterIt(it.protocols.contains(proto))

proc getReachablePeers*(peerStore: PeerStore): seq[RemotePeerInfo] =
let threshold = Moment.fromNow(millis(-3*60*60*1000))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure the units are in seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the secs and hours functions cause compilation errors. will try to move hourly formula. regarding 3 hours I will check with alvaro before merging.

Copy link
Contributor

@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.

is it just me or something got messed up during force push?

I literally don't see any diff at all. Here is what i am seeing

Screenshot 2024-07-12 at 3 58 29 PM

@darshankabariya
Copy link
Contributor Author

is it just me or something got messed up during force push?

I literally don't see any diff at all. Here is what i am seeing

Screenshot 2024-07-12 at 3 58 29 PM

Currently, there's nothing in this PR. I implemented Alvaro's suggested proposal, but Hanno recommended a more feasible long-term solution. so i revert older version. I'm working on that now and will update once it's done. Actually discussion going on this thread, https://discord.com/channels/1110799176264056863/1260493155791405096

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.

This is a good start. Perhaps we can evaluate if things improve? Afaics this is a very cheap refresh, so we may even lower interval further if needed.

@darshankabariya
Copy link
Contributor Author

darshankabariya commented Jul 31, 2024

This is a good start. Perhaps we can evaluate if things improve? Afaics this is a very cheap refresh, so we may even lower interval further if needed.

Thank you for your suggestion. I've already created a draft PR to analyze peer exchange behavior: #2940. The plan is to investigate whether increasing the frequency of cache refreshes helps. If it doesn't, we would be unnecessarily loading the node. soon I will present matrics of analysis using these tools and then we will decide.

cc @jm-clius

@darshankabariya darshankabariya marked this pull request as draft August 12, 2024 08:10
@darshankabariya darshankabariya marked this pull request as ready for review August 23, 2024 15:24
@darshankabariya
Copy link
Contributor Author

darshankabariya commented Aug 23, 2024

This is a good start. Perhaps we can evaluate if things improve. Afaics this is a very cheap refresh, so we may even lower interval further if needed.

As you suggest @jm-clius
I've been developing tools to analyze peer exchange availability for the waku network. You can find the code in PR #2940

Tool Overview
This tool evaluates the effectiveness of peer exchange responses. Typically, the peer list updates every 15 minutes. In my analysis, I requested 5 peers every 2 minutes, checking their availability by attempting to connect at the P2P level.

The process involved 60 iterations, accumulating 300 peer requests over 3 hours. I generated percentage charts to visualize success rates at different cache refresh intervals. The results are as follows:

15-minute interval: 265/300
10-minute interval: 278/300
5-minute interval: 277/300

Conclusion
Increasing the frequency by decreasing the cache refresh interval appears effective only for 10 minutes. At 5 minutes, similar to 10 minutes. I recommend updating the cache refresh rate from 15 to 10 minutes.

I’d welcome any suggestions or alternative approaches to this analysis

cc @gabrielmer @chaitanyaprem @Ivansete-status @NagyZoltanPeter

@Ivansete-status
Copy link
Collaborator

Thanks for the analysis in #2889 (comment) @darshankabariya . It seems reasonable to set it to 10min (considering very few test iterations but it is fine.)
Make sure to enhance the CI before merging :)

@darshankabariya darshankabariya merged commit 4315710 into master Aug 23, 2024
9 of 11 checks passed
@darshankabariya darshankabariya deleted the bug_2414 branch August 23, 2024 18:01
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.

bug: peer exchange returns nodes that no longer exist.
4 participants