-
Notifications
You must be signed in to change notification settings - Fork 78
fix: remove ENR cache from peer exchange (#3578) #3652
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
Conversation
* remove WakuPeerExchange.enrCache * add forEnrPeers to support fast PeerStore search * add getEnrsFromStore * fix peer exchange tests
|
You can find the image built from this PR at Built from 4696901 |
Ivansete-status
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for it! 💯
Just adding some nitpicks that I hope you find useful
Co-authored-by: Ivan FB <[email protected]>
Co-authored-by: Ivan FB <[email protected]>
|
NOTE: the ci.yml workflow is broken, likely due to the project name change. It was blowing up on line 135 of ci.yml. I changed it to: Now it blows up on line 140, which still references waku.org. |
Can you do a dedicated PR to fixing the CI so we can get it merged asap (feel free to ping me for review)? |
On it |
4f97986 to
997a635
Compare
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awsome, thank you! Added one recommendation for the template if you may consider to apply.
ab35cfa to
997a635
Compare
* remove WakuPeerExchange.enrCache * add forEnrPeers to support fast PeerStore search * add getEnrsFromStore * fix peer exchange tests
Description
This PR removes the ENR cache and the worker loop that fed it in WakuPeerExchange, and replaces it with a direct query to the PeerStore. To avoid slowdowns, the new
getEnrsFromStoredoesn't allocate any memory other than the resulting sequence of ENR records to the caller.Changes
Issue
closes #3578