-
Notifications
You must be signed in to change notification settings - Fork 1k
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-store): introduce libp2p-peer-store #5724
base: master
Are you sure you want to change the base?
Conversation
This is a very basic implementation, would love to hear more ideas on how to implement this! |
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.
Thanks for starting this effort @drHuangMHT!
If I understand the current implementation correctly, the purpose of this store is just to track connected peers, and explicitly added addresses. However, I think we have to keep in mind that protocols like kademlia or identify very frequently report all addresses that they learn as NewExternalAddrOfPeer
. So with the current MemoryStore
implementation, the store would grow unbounded with all of these addresses. I think there needs to be some GC strategy.
Also, after reading #4103 (comment), I think a peer store implementation could also do much more than just track explicitly added addresses, e.g.:
- track how valuable a know address is, by using infos about how the address info was received, if we connected to it already, etc
- implement a TTL for address records
- track other meta data for a peer, e.g. public key
I don't think this needs to be implemented in this PR. But I think we could forward more of the already present info to the Store
trait, so that a custom implementation can have more sophisticated logic.
simplify Store trait, don't report conencted unless confirmed by swarm event
Oops that |
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.
Thanks for starting this @drHuangMHT, already looking good! left some notes to allow further customization.
cc @elenaf9
Store now handles FromSwarm directly; rename some on_* methods; allow removing address; update records on FromSwarm::ConnectionEstablished
This can get very complicated very soon, for example how to record the activitiy on the address(there will be some overhead) and how to make it machine-readable(scoring system). The discussion will be quite lengthy.
I didn't see a good way to implement TTL(garbage collect) for records, any pointer?
I don't see the remote public key being available through swarm itself, be it in the form of |
I don't think we need to solve these in this PR, or even within this repo for that matter. These were just meant as examples for things that user's might want to use the new |
NP, |
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.
Thank you for the follow ups!
Couple more comments.
only provide signed address when strict_mode is set to true
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.
Hey @drHuangMHT!
I tried to use the peer store. I like it! I've added some comments with changes I needed to make.
Also, I agree with @jxs's comment that it would be useful to add a generic parameter to the memory store for user data. I know that it is only a reference implementation and users are free to implement Store
themselves, but I feel like custom data would be a rather common use case, and it would be unfortunate if users had to copy-paste the memory store in too many cases.
Hi @drHuangMHT, thanks, the changes look great! |
I think it would be better to implement this on the store implementation(not on |
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.
Hi @drHuangMHT thanks for driving this.
I had a call Today with @elenaf9 and @dknopik to discuss this PR, as we are using this it to understand how it may help us across two projects where we need a Peer Store.
I would like to avoid adding features that don't have real demand and be as simple as possible in this phase, to then iterate adding them later when needed.
I Left some comments that stemmed from that call, Elena and Daniel feel free to add and correct anything that I missed. DrHuang give your thoughts as well ofc.
Thanks!
addresses: LruCache<Multiaddr, AddressRecord>, | ||
custom: Option<T>, | ||
} | ||
impl<T> PeerRecord<T> { |
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.
I don't know if we need all these methods for now (the CRUD seem duplicated on the MemoryStore
), can we just have PeerRecord
as a struct and have it in the upper module?
Description
Introduce
libp2p-peer-store
for a peer store implementation.Related: #4103
Notes & open questions
Change checklist