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.

elenaf9 · 2024-12-07T15:41:06Z

misc/peer-store/src/store.rs

+    /// Peers that are currently connected.
+    connected_peers: HashSet<PeerId>,


Is there a case where using a HashSet<PeerId> to track connected peers is unsuitable for a specific use case? If now, how about moving this into the Behavior, so that the Store only concerns the "address-book" part of this behavior?

The idea of Store trait is to allow on-disk storage, now I think about it, this info will be changing in real time so it should be kept in memory anyway. Will move it into the behaviour itself.

can we have MemoryStore<T=()> so that we are able to store data for peers (like scoring etc):

can we have MemoryStore<T=()> so that we are able to store data for peers (like scoring etc):

MemoryStore is more of a reference implementation, I don't think it is necessary to include a generic parameter for customization since we are maintaining its internals.

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

elenaf9 · 2024-12-07T16:30:13Z

misc/peer-store/src/store.rs

+    /// Update an address record.  
+    /// Return `true` when the address is new.  
+    fn on_address_update(&mut self, peer: &PeerId, address: &Multiaddr) -> bool;


Nit: update gives the impression that a peer just has one address, and that this address gets updated here.
Wdyt of instead of calling it instead on_new_address or something like that?

Because for now I only see one address pop up at a time. I was planning to use a boxed slice but you know there would be a heap allocation, which isn't necessary for only one element.

Also we can't do a batch update so there will be a iterator anyway.

Sorry, maybe I wasn't clear. I was just nitpicking on the name of the function, not the address: &Multiaddr parameter :)

Um, I'm not so sure about the naming, because the address isn't necessarily a new address. If the address is not new, it is updated due to LRU rules, like touch, so I can't quite make the decision.

Wdyt of on_address_discovered then?

Wdyt of on_address_discovered then?

discovered also kind of suggests the address is new? I'm not really convinced.

dariusc93 · 2024-12-07T18:03:40Z

misc/peer-store/src/behaviour.rs

+        _role_override: libp2p_core::Endpoint,
+        _port_use: libp2p_core::transport::PortUse,
+    ) -> Result<libp2p_swarm::THandler<Self>, libp2p_swarm::ConnectionDenied> {
+        self.store.on_peer_connect(&peer);


It might make more sense to remove this for FromSwarm events since a connection could be denied later on (ie connection limits, banned peer, etc.), so that way the store isnt exactly cluttered.

That makes sense. Will favor the swarm event.

can we instead define Store::handle_* methods that are called here and in the other NetworkBehaviour::handle_*
so that it allows us to further manage our peers?

can we instead define Store::handle_* methods that are called here and in the other NetworkBehaviour::handle_* so that it allows us to further manage our peers?

What are those specifically? The store no longer record connected peers.

dariusc93 · 2024-12-07T18:04:52Z

misc/peer-store/src/store.rs

+/// - contains all observed addresses of peers;
+pub trait Store {
+    /// Called when a peer connects.
+    fn on_peer_connect(&mut self, peer: &PeerId);


If we are tracking peer connections too, should we also keep tabs on the ConnectionId?

We can, but how to store it? I think there can be multiple connections from a single peer, no?

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

jxs · 2024-12-11T16:00:28Z

misc/peer-store/src/store.rs

+    /// Peers that are currently connected.
+    connected_peers: HashSet<PeerId>,


can we have MemoryStore<T=()> so that we are able to store data for peers (like scoring etc):

jxs · 2024-12-11T16:02:30Z

misc/peer-store/src/behaviour.rs

+        _role_override: libp2p_core::Endpoint,
+        _port_use: libp2p_core::transport::PortUse,
+    ) -> Result<libp2p_swarm::THandler<Self>, libp2p_swarm::ConnectionDenied> {
+        self.store.on_peer_connect(&peer);


can we instead define Store::handle_* methods that are called here and in the other NetworkBehaviour::handle_*
so that it allows us to further manage our peers?

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

elenaf9 · 2024-12-31T07:09:53Z

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.

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 libp2p-peer-store for (e.g. when also combining it with other behaviors). Sorry if that wasn't clear!

drHuangMHT · 2024-12-31T07:30:16Z

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 libp2p-peer-store for (e.g. when also combining it with other behaviors). Sorry if that wasn't clear!

NP, PeerRecord is also what we need for gossipsub Peer eXchange feature that is currently missing. Conversations like this can be put into the documentation to give users some inspiration, and MemoryStore can be a good example as well.

elenaf9

Thank you for the follow ups!
Couple more comments.

Cargo.toml

elenaf9 · 2024-12-31T07:14:57Z

misc/peer-store/src/store.rs

+    /// Update an address record.  
+    /// Return `true` when the address is new.  
+    fn on_address_update(&mut self, peer: &PeerId, address: &Multiaddr) -> bool;


Sorry, maybe I wasn't clear. I was just nitpicking on the name of the function, not the address: &Multiaddr parameter :)

elenaf9 · 2024-12-31T07:22:26Z

misc/peer-store/src/memory_store.rs

+    pub fn last_seen_since(&self, now: Instant) -> Duration {
+        now.duration_since(*self.last_seen)
+    }
+    /// How much time has passed since the address is last reported wrt. current time.
+    pub fn last_seen(&self) -> Duration {
+        let now = Instant::now();
+        now.duration_since(*self.last_seen)
+    }


Why not just make last_seen public like the rest?

idk, they will have more semantic meanings this way I guess.

misc/peer-store/src/memory_store.rs

misc/peer-store/src/store.rs

misc/peer-store/src/behaviour.rs

only provide signed address when strict_mode is set to true

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!

jxs · 2025-01-23T00:08:30Z

Cargo.toml

@@ -100,7 +102,7 @@ libp2p-request-response = { version = "0.28.0", path = "protocols/request-respon
 libp2p-server = { version = "0.12.8", path = "misc/server" }
 libp2p-stream = { version = "0.2.0-alpha.1", path = "protocols/stream" }
 libp2p-swarm = { version = "0.45.2", path = "swarm" }
-libp2p-swarm-derive = { version = "=0.35.0", path = "swarm-derive" } # `libp2p-swarm-derive` may not be compatible with different `libp2p-swarm` non-breaking releases. E.g. `libp2p-swarm` might introduce a new enum variant `FromSwarm` (which is `#[non-exhaustive]`) in a non-breaking release. Older versions of `libp2p-swarm-derive` would not forward this enum variant within the `NetworkBehaviour` hierarchy. Thus the version pinning is required.
+libp2p-swarm-derive = { version = "=0.35.0", path = "swarm-derive" }                            # `libp2p-swarm-derive` may not be compatible with different `libp2p-swarm` non-breaking releases. E.g. `libp2p-swarm` might introduce a new enum variant `FromSwarm` (which is `#[non-exhaustive]`) in a non-breaking release. Older versions of `libp2p-swarm-derive` would not forward this enum variant within the `NetworkBehaviour` hierarchy. Thus the version pinning is required.


we don't need this white space right?

jxs · 2025-01-23T00:10:02Z

misc/peer-store/Cargo.toml

+[dependencies]
+libp2p-core = { workspace = true }
+libp2p-swarm = { workspace = true }
+lru = "*"


we need to define proper versions of these dependencies

jxs · 2025-02-04T10:33:47Z

libp2p/Cargo.toml

@@ -68,6 +68,7 @@ mdns = ["dep:libp2p-mdns"]
 memory-connection-limits = ["dep:libp2p-memory-connection-limits"]
 metrics = ["dep:libp2p-metrics"]
 noise = ["dep:libp2p-noise"]
+# peer-store = ["dep:libp2p-peer-store"]


to be uncommented right?

jxs · 2025-02-04T10:36:08Z

libp2p/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.55.0(unreleased)


probably will be addressed when solving the conflicts, but this doesn't need to be a minor, we can add the peer store as a patch release to the main libp2p

jxs · 2025-02-04T14:05:39Z

misc/peer-store/src/behaviour.rs

+
+    /// Manually update a record.  
+    /// This will always emit an `Event::RecordUpdated`.
+    pub fn update_address(&mut self, peer: &PeerId, address: &Multiaddr) {


having this duplication where the Behaviour and the Store share the same method is prone to confusion to the end user. I suggest we have a thin Behaviour delegating the on_swarm_event to the Store

This is for users who might want to update the cache from outside of the behavior. However if we only expect updates through swarm event, it can be removed.

jxs · 2025-02-04T14:06:49Z

misc/peer-store/src/behaviour.rs

+    RecordUpdated {
+        peer: PeerId,
+    },
+    Store(T),


we should also comment this variant

jxs · 2025-02-04T14:07:38Z

misc/peer-store/src/store.rs

+/// A store that
+/// - contains all observed addresses of peers;


this can probably be just a one liner comment right?

jxs · 2025-02-05T11:27:08Z

misc/peer-store/src/memory_store.rs

+    pub(crate) struct PeerRecord<T> {
+        /// A LRU(Least Recently Used) cache for addresses.  
+        /// Will delete the least-recently-used record when full.
+        addresses: LruCache<Multiaddr, AddressRecord>,


Suggested change

addresses: LruCache<Multiaddr, AddressRecord>,

addresses: LruCache<Multiaddr, ()>,

and can we then remove AddressRecord and go simple?

I would like to keep the signed address part at least. But if we need garbage collection we also need to keep track of when it was last seen.

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?

jxs · 2025-02-05T12:38:45Z

misc/peer-store/src/store.rs

+
+/// A store that
+/// - contains all observed addresses of peers;
+pub trait Store {


The Store should also only have the methods:

poll

addresses_or_peer

on_swarm_event

on_swarm_event

IIRC we also agreed that we shouldn't forward the whole SwarmEvent, but instead disassemble and match only the relevant variants each into a separate method, namely:

on_connection_established(peer_id: PeerId, address: Multiaddr)

on_dial_error(peer_id: PeerId, address: Multiaddr)

on_new_addr_of_peer(peer_id: PeerId, address: Multiaddr)

(The concrete methods and parameters are up for discussion.)

I know that this is reverting again what was proposed in #5724 (comment). But it would prevent the user from having to think about all the SwarmEvent::Listener* variants etc.

I don't feel strongly about this particular aspect, so I'd be also okay with sticking with on_swarm_event. @jxs do you agree with the proposed change? Or did I misunderstand you and you still prefer a single on_swarm_event?

no strong opinion Elena, as I lack usage on this.
So let's follow your suggestion and then in the future we can change it if needed

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

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

introduce PeerRecord

121e91b

elenaf9 reviewed Dec 31, 2024

View reviewed changes

drHuangMHT added 5 commits December 31, 2024 19:47

remove borrowed type and unused associated type

47048ec

move garbage collection to Store level

9da96ff

delay cloning of PeerRecord

9945fd0

MemoryStore strict mode

cb294c6

only provide signed address when strict_mode is set to true

allow Store to report to Swarm

a9dc9aa

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

jxs reviewed Feb 5, 2025

View reviewed changes

		/// Peers that are currently connected.
		connected_peers: HashSet<PeerId>,

		/// A store that
		/// - contains all observed addresses of peers;

	addresses: LruCache<Multiaddr, AddressRecord>,
	addresses: LruCache<Multiaddr, ()>,

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

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

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

drHuangMHT commented Dec 9, 2024

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 Dec 16, 2024 • edited Loading

elenaf9 commented Dec 31, 2024

drHuangMHT commented Dec 31, 2024

elenaf9 left a comment • edited Loading

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

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

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

Choose a reason for hiding this comment

elenaf9 left a comment •

edited

Loading

drHuangMHT commented Dec 16, 2024 •

edited

Loading

elenaf9 left a comment •

edited

Loading