feat(peer-store): introduce libp2p-peer-store #5724

drHuangMHT · 2024-12-07T07:36:19Z

Description

Introduce libp2p-peer-store for a peer store implementation.

Related: #4103

Notes & open questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

drHuangMHT · 2024-12-07T07:49:28Z

This is a very basic implementation, would love to hear more ideas on how to implement this!

elenaf9

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.

misc/peer-store/src/store.rs

misc/peer-store/src/lib.rs

misc/peer-store/src/store.rs

misc/peer-store/src/behaviour.rs

misc/peer-store/src/store.rs

misc/peer-store/src/behaviour.rs

misc/peer-store/src/store.rs

misc/peer-store/src/behaviour.rs

simplify Store trait, don't report conencted unless confirmed by swarm event

…ture denial

drHuangMHT · 2024-12-09T06:32:48Z

Oops that use<'a> syntax was stabilized in rust 1.82, the compiler on 1.80 wasn't happy with it.

jxs

Thanks for starting this @drHuangMHT, already looking good! left some notes to allow further customization.
cc @elenaf9

misc/peer-store/Cargo.toml

misc/peer-store/src/behaviour.rs

misc/peer-store/src/store.rs

misc/peer-store/src/behaviour.rs

Store now handles FromSwarm directly; rename some on_* methods; allow removing address; update records on FromSwarm::ConnectionEstablished

drHuangMHT · 2024-12-16T06:22:29Z

track how valuable a know address is, by using infos about how the address info was received, if we connected to it already, etc

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.

implement a TTL for address records

I didn't see a good way to implement TTL(garbage collect) for records, any pointer?

track other meta data for a peer, e.g. public key

I don't see the remote public key being available through swarm itself, be it in the form of PeerRecord, SignedEnvelope or just PublicKey. But the pubkey is indeed a very important metadata of a peer.

…into peer-store

misc/peer-store/src/behaviour.rs

dknopik

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.

misc/peer-store/src/behaviour.rs

misc/peer-store/src/memory_store.rs

dknopik · 2025-02-03T14:31:41Z

Hi @drHuangMHT,

thanks, the changes look great!
Another thing that I found that would be useful is providing a way to iterate over the peers we have in the store. I am not sure though if this should be offered by the Behaviour, or only by the Store implementation. What do you think?

drHuangMHT · 2025-02-04T12:14:26Z

thanks, the changes look great! Another thing that I found that would be useful is providing a way to iterate over the peers we have in the store. I am not sure though if this should be offered by the Behaviour, or only by the Store implementation. What do you think?

I think it would be better to implement this on the store implementation(not on Behaviour or Store trait itself) so we don't clutter the interfaces, and we already expose the internal store on Behaviour.

jxs

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!

Cargo.toml

misc/peer-store/Cargo.toml

libp2p/Cargo.toml

libp2p/CHANGELOG.md

misc/peer-store/src/behaviour.rs

misc/peer-store/src/store.rs

misc/peer-store/src/memory_store.rs

jxs · 2025-02-05T11:28:47Z

misc/peer-store/src/memory_store.rs

+        addresses: LruCache<Multiaddr, AddressRecord>,
+        custom: Option<T>,
+    }
+    impl<T> PeerRecord<T> {


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?

We need this when iterating over all records since custom is private.

misc/peer-store/src/store.rs

misc/peer-store/src/memory_store.rs

drHuangMHT · 2025-02-17T08:24:32Z

I forgot this very important reason why I want to support libp2p::core::PeerRecord(signed addresses) here in peer store: gossipsub PeerEXchange! So this is a series of PR preceding it: #5785 makes Identify a source of PeerRecord, this PR allow us to store it and I'm planning on one to enable the use of it in gossipsub.
@jxs thoughts?

jxs

I forgot this very important reason why I want to support libp2p::core::PeerRecord(signed addresses) here in peer store: gossipsub PeerEXchange! So this is a series of PR preceding it: #5785 makes Identify a source of PeerRecord, this PR allow us to store it and I'm planning on one to enable the use of it in gossipsub.
@jxs thoughts?

Hi Dr Huang, can we do it on a subsequent PR?

misc/peer-store/src/memory_store.rs

jxs · 2025-02-19T12:31:48Z

misc/peer-store/src/memory_store.rs

+    }
+}
+
+#[cfg(feature = "serde")]


why do we require the Serde features?

We've discussed it in the last maintainers call. We would like to persist the data in the store in some way and serde is probably the easiest.

AFAIR, we discussed it for kad not for peer-store right?
What are the use cases for persisting the peer-store?
@rishiad as you were also in that conversation, do you have a specific need for serializing the peer-store
I also feel it's counter intuitive to have a MemoryStore that may then be a persistent store

It is called MemoryStore doesn't necessarily suggest it is meant to be temporary and volatile. If you don't want things like a database driver but also would like to persist data before shutdown, flip the switch, just like the generic parameter on it. It is not necessary to duplicate the code right?

but the generic parameter can be retrieved with record_iter and record_iter_mut right? And then serialized after for anyone who needs it

but the generic parameter can be retrieved with record_iter and record_iter_mut right? And then serialized after for anyone who needs it

I mean we don't really need that generic for PeerRecord but we decided to have it because it makes attaching additional data easier without re-implement MemoryStore, and I think the same can be said for serde support. Not that it is inconvenient when we want to persist the additional data though.

I mean we don't really need that generic for PeerRecord but we decided to have it because it makes attaching additional data easier without re-implement MemoryStore

we need it, you can see it being parametrized here I wouldn't have suggested it if there were no actual use cases for it.
Goes in line with what I wrote here and it's basically applying YAGNI. I.e do not limit hypothetical extensibility but also do not implement it if there are no actual use cases for it.

and I think the same can be said for serde support. Not that it is inconvenient when we want to persist the additional data though.

Yes but if no one requires it why should we maintain that code? What's the advantage?
The disadvantage is that it's more code to maintain, and may also be tampered by users not knowing what they may be doing.
When someone requires a specific feature, they will have the requirements to fit their purpose and will be a much better solution as it's adapted and tailored to their specific usecase, they will also have proper feedback with testing it in real case scenarios etc.

cc @dknopik

ping @drHuangMHT

ping @drHuangMHT

Not waiting for Daniel @dknopik to report back on this?

misc/peer-store/src/behaviour.rs

jxs

Ok almost ready to go Dr Huang, only missing removal of the serde code.
Thanks!

jxs · 2025-02-25T17:28:56Z

misc/peer-store/src/memory_store.rs

+    }
+}
+
+#[cfg(feature = "serde")]


ping @drHuangMHT

drHuangMHT added 5 commits December 6, 2024 14:48

template implementation

1affefe

implement in-memory store

8d4930a

manifest update

9cb59c3

formatting

fbc1344

docs

2bcfa7f

elenaf9 reviewed Dec 7, 2024

View reviewed changes

dariusc93 reviewed Dec 7, 2024

View reviewed changes

misc/peer-store/src/behaviour.rs Outdated Show resolved Hide resolved

misc/peer-store/src/store.rs Outdated Show resolved Hide resolved

misc/peer-store/src/store.rs Outdated Show resolved Hide resolved

misc/peer-store/src/behaviour.rs Outdated Show resolved Hide resolved

drHuangMHT added 5 commits December 8, 2024 11:14

return iterator of references instead of heap allocation

6df6ee5

move conencted_peers out of Store

99a6bfd

simplify Store trait, don't report conencted unless confirmed by swarm event

use capped LruCache instead of uncapped HashMap for address records

5c7ba32

update address book when a connection is established regardless of fu…

b6dcd59

…ture denial

provide address for dial

cbb1906

jxs reviewed Dec 11, 2024

View reviewed changes

apply suggestions

6270259

Store now handles FromSwarm directly; rename some on_* methods; allow removing address; update records on FromSwarm::ConnectionEstablished

drHuangMHT added 3 commits December 17, 2024 20:36

garbage collect records, test

cc91ab5

documentation and formatting

f9b040e

clippy lint

a9597da

drHuangMHT requested review from jxs, dariusc93 and elenaf9 December 18, 2024 02:30

drHuangMHT added 6 commits December 20, 2024 14:36

simplify Store trait

0e6b280

Merge branch 'master' into peer-store

fe14c42

manifest and changelog

92ac2fb

Merge branch 'peer-store' of https://github.com/drHuangMHT/rust-libp2p …

913ed1f

…into peer-store

export at libp2p crate root

b8a7114

changelog for libp2p

5f628fb

elenaf9 mentioned this pull request Dec 23, 2024

feat(request-response): Add support for custom dial options when making request to disconnected peer #5692

Open

4 tasks

diegomrsantos reviewed Jan 15, 2025

View reviewed changes

misc/peer-store/src/behaviour.rs Outdated Show resolved Hide resolved

don't store connected peers

c94f636

dknopik reviewed Jan 29, 2025

View reviewed changes

misc/peer-store/src/behaviour.rs Show resolved Hide resolved

misc/peer-store/src/behaviour.rs Show resolved Hide resolved

misc/peer-store/src/memory_store.rs Outdated Show resolved Hide resolved

misc/peer-store/src/memory_store.rs Outdated Show resolved Hide resolved

allow attaching custom data

a02f088

jxs force-pushed the peer-store branch from 26341b3 to a02f088 Compare January 31, 2025 16:44

make Behaviour composable

a909499

event hierarchy rework

7c4d9b4

jxs reviewed Feb 5, 2025

View reviewed changes

drHuangMHT added 5 commits February 11, 2025 10:44

simplify Store trait

5639ae5

doc test

a040e07

doc and manifests

7c847a6

Merge branch 'master' into peer-store

dee1897

add peer-store to full feature set

9af4c69

rishiad reviewed Feb 11, 2025

View reviewed changes

misc/peer-store/src/memory_store.rs Outdated Show resolved Hide resolved

drHuangMHT added 6 commits February 13, 2025 11:58

simplify MemoryStore, serde support

9cb909b

Merge branch 'master' into peer-store

9740566

tests with Swarm and Identify

3ea8439

docs

cdb9188

clippy lint

4a76192

doc link

082eb16

jxs reviewed Feb 19, 2025

View reviewed changes

drHuangMHT added 4 commits February 21, 2025 10:54

register waker for MemoryStore

7063769

fix imports in doc test

06ed691

Merge branch 'master' into peer-store

c9e2c0b

fmt

00d8d33

drHuangMHT commented Feb 25, 2025

View reviewed changes

misc/peer-store/src/behaviour.rs Show resolved Hide resolved

jxs reviewed Feb 25, 2025

View reviewed changes

misc/peer-store/src/memory_store.rs

}

}

#[cfg(feature = "serde")]

Copy link

Member

jxs Feb 25, 2025

Choose a reason for hiding this comment

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

ping @drHuangMHT

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(peer-store): introduce libp2p-peer-store #5724

feat(peer-store): introduce libp2p-peer-store #5724

drHuangMHT commented Dec 7, 2024

drHuangMHT commented Dec 7, 2024

elenaf9 left a comment •

edited

Loading

drHuangMHT commented Dec 9, 2024

jxs left a comment

drHuangMHT commented Dec 16, 2024 •

edited

Loading

dknopik left a comment

dknopik commented Feb 3, 2025

drHuangMHT commented Feb 4, 2025

jxs left a comment

jxs Feb 5, 2025

drHuangMHT Feb 17, 2025

drHuangMHT commented Feb 17, 2025

jxs left a comment

jxs Feb 19, 2025

drHuangMHT Feb 20, 2025

jxs Feb 20, 2025

drHuangMHT Feb 20, 2025

jxs Feb 21, 2025

drHuangMHT Feb 21, 2025

jxs Feb 21, 2025

drHuangMHT Feb 21, 2025

jxs Feb 25, 2025

drHuangMHT Feb 26, 2025

jxs left a comment

jxs Feb 25, 2025

feat(peer-store): introduce libp2p-peer-store #5724

Are you sure you want to change the base?

feat(peer-store): introduce libp2p-peer-store #5724

Conversation

drHuangMHT commented Dec 7, 2024

Description

Notes & open questions

Change checklist

drHuangMHT commented Dec 7, 2024

elenaf9 left a comment • edited Loading

Choose a reason for hiding this comment

drHuangMHT commented Dec 9, 2024

jxs left a comment

Choose a reason for hiding this comment

drHuangMHT commented Dec 16, 2024 • edited Loading

dknopik left a comment

Choose a reason for hiding this comment

dknopik commented Feb 3, 2025

drHuangMHT commented Feb 4, 2025

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drHuangMHT commented Feb 17, 2025

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 left a comment •

edited

Loading

drHuangMHT commented Dec 16, 2024 •

edited

Loading