-
Notifications
You must be signed in to change notification settings - Fork 66
feat(kad-dht): findPeer #1624
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(kad-dht): findPeer #1624
Conversation
🏁 Performance SummaryCommit:
📊 View Latency History and full Container Resources in the Workflow Summary |
Ben-PH
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. just some things that you might like to change if they align with your style.
| kad.switch.peerStore[AddressBook][p.peerId] = p.addrs | ||
| # Nodes might return different addresses for a peer, so we append instead of replacing | ||
| var existingAddresses = | ||
| kad.switch.peerStore[AddressBook][p.peerId].toHashSet() |
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.
Is there any reason to keep it in a seq instead of hash set? Wolud a BTreeMap/Set be appropriate? it's an ordered, sparse collection, but as discussed earlier, scaling isn't a concern: I'm more thinking that the nature of this collection is "indexible by key, sparse in nature, keys are inherently ordered" which fits the bill for a BTree of some sort quite nicely
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.
Not sure of the reason why the AddressBook uses a sequence to store its addresses instead of a different data structure. Sadly it's marked as {.public.} which makes refactoring harder as it's already part of the Public API
AddressBook* {.public.} = ref object of SeqPeerBook[MultiAddress]
Adds an utils function to find specific peers. Also does some filtering around the node's own peerId and append addresses to the peer store instead of replacing them