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(peer-exchange): support continuous peer information updates #2088

Merged
merged 7 commits into from
Jul 26, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Jul 25, 2024

Problem

In a case where Peer Exchange discovers a peer, and stores its information including shard it supports & the listening multiaddresses it has, when and if the same peer is discovered again, we check for its presence in the Peer Store and if it exists we don't take any action.

This can be problematic as discovered in waku-org/go-waku#1128, where the peer discovered again MIGHT have new shard information it supports, or new listening addresses, while having the same peer ID (essentially being seen as the same peer)

Solution

Once the peer is discovered again, do a check on the information received by Peer Exchange in respect to the shard information and multiaddrs received now, and compare it with information we have in the Peer Store -- this will guide us to either patching the Peer Store with new updated information, or continuing with confirmation nothing changed.

Also adds tests for updated shard information & listening addresses.

Notes

Contribution checklist:

  • covered by unit tests;
  • covered by e2e test;
  • add ! in title if breaks public API;

@danisharora099 danisharora099 requested a review from a team as a code owner July 25, 2024 16:04
@danisharora099 danisharora099 changed the title feat(peer-exchange): support continuous discovery updates feat(peer-exchange): support continuous peer information updates Jul 25, 2024
Copy link

github-actions bot commented Jul 25, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 118.95 KB (0%) 2.4 s (0%) 3.3 s (+5.1% 🔺) 5.7 s
Waku Simple Light Node 183.84 KB (+0.09% 🔺) 3.7 s (+0.09% 🔺) 2.8 s (-17.34% 🔽) 6.5 s
ECIES encryption 23.12 KB (0%) 463 ms (0%) 659 ms (+9.79% 🔺) 1.2 s
Symmetric encryption 22.6 KB (0%) 452 ms (0%) 1.1 s (+124.54% 🔺) 1.6 s
DNS discovery 72.49 KB (0%) 1.5 s (0%) 2 s (+20.37% 🔺) 3.4 s
Peer Exchange discovery 74.37 KB (+0.21% 🔺) 1.5 s (+0.21% 🔺) 1.8 s (-21.9% 🔽) 3.3 s
Local Peer Cache Discovery 67.68 KB (0%) 1.4 s (0%) 1.7 s (-31.52% 🔽) 3.1 s
Privacy preserving protocols 38.9 KB (0%) 778 ms (0%) 1.7 s (+59.85% 🔺) 2.5 s
Waku Filter 113.31 KB (0%) 2.3 s (0%) 2.4 s (+32.41% 🔺) 4.6 s
Waku LightPush 111.31 KB (0%) 2.3 s (0%) 2.5 s (+20.46% 🔺) 4.7 s
History retrieval protocols 111.81 KB (0%) 2.3 s (0%) 2 s (+13% 🔺) 4.2 s
Deterministic Message Hashing 7.29 KB (0%) 146 ms (0%) 281 ms (+26.66% 🔺) 427 ms


const existingMas = peer.addresses.map((a) => a.multiaddr.toString());
const newMas = peerInfo.multiaddrs.map((ma) => ma.toString());
const hasMaDiff = existingMas.some((ma) => !newMas.includes(ma));
Copy link
Member

Choose a reason for hiding this comment

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

What happens if newMas has a multiaddr that's not in existingMas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We patch the Peer Store with the new multiaddrs (overwrite these), as that would imply the older addresses are now invalid, thus replacing with the new ones

}),
...(hasShardDiff &&
shardInfo && {
metadata: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

will it replace previous value of metadata or will it combine them?

also, if transition happens from shardInfo={...} to shardInfo=null - should it be erased from peerStore?

Copy link
Member

Choose a reason for hiding this comment

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

using patch will replace all existing metadata, might be better to use merge here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will it replace previous value of metadata or will it combine them?

also, if transition happens from shardInfo={...} to shardInfo=null - should it be erased from peerStore?

Yes: #2088 (comment)

using patch will replace all existing metadata, might be better to use merge here

Good point about that. We only use metadata for shardInfo and TAGS for now, but merge is a better option here. Thank you! :D

@danisharora099 danisharora099 force-pushed the feat/continuous-discovery branch from db0de08 to 44215b3 Compare July 26, 2024 08:23
@danisharora099 danisharora099 merged commit defe41b into master Jul 26, 2024
11 checks passed
@danisharora099 danisharora099 deleted the feat/continuous-discovery branch July 26, 2024 11:44
@weboko weboko mentioned this pull request Jul 25, 2024
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.

chore(peer-exchange): support updates of previously discovered peer's addresses
3 participants