Peer Store

[libp2p]

Peer Store

[folsen]

@Vurich @rphmeier could you provide some feedback on the interface of the Peer Store as defined in https://github.com/paritytech/libp2p-rs/blob/fh-add-peer/libp2p-peerstore/src/peerstore.rs

The implementation (memory_peerstore.rs) is sort of retarded, and probably not even correct, I just put stuff in to satisfy the tests and make sure that the implementation "feels" alright.

[eira-fransham]

I feel like you should have a get(&self) -> &Peer and get_mut(&mut self) -> &mut Peer (or if you need more complex logic you can use some PeerMut gatekeeper type). I don't like the duplication of get_* since they all do the getting logic, so it wastes cycles and keystrokes if you want to edit multiple things at once.

[rphmeier]

clear_addrs should return Vec<Multiaddr>.

Isn't PeerId a really light type? seems weird to return the Vec<&PeerId> when getting known peers. In general, what about returning/accepting iterators instead of Vec for arguments? I guess it doesn't play as well with the Box<PeerStore> situation.

[eira-fransham]

Ideally we would prevent any use of unavoidable allocations in a library. My recommendation is an associated type that implements Iterator<Item = Multiaddr> and defaults to Vec.

[folsen]

One question I have myself is if PeerId and PeerInfo should somehow actually be the same type, but then I'm not sure how you'd efficiently do lookups, right now there's HashMap<PeerId, PeerInfo>.

But PeerId is really just a multihash, and PeerInfo is responsible for a few things, most importantly managing Multiaddrs. The set of Multiaddr needs to be mutated either implictly or explicitly. The Go implementation mutates implicitly, when adding or removing a Multiaddr it goes through and removes ones with expired TTLs. I'd probably be in favor of an implicit thing as well, when you call get only returning ones with non-expired TTLs and when calling add/set you actually remove the expired ones to "clean up".

Ideally I would've wanted Multiaddr to be a HashSet, not a Vec, because that's what it is. But it might even be a HashMap<Multiaddr,TTL>, the problem right now is that Multiaddr doesn't implement Hash.

seems weird to return the Vec<&PeerId> when getting known peers.

Yeah, i actually forgot this in there. My idea was to return an iterator, but the problem is that the set of Peers can be modified after getting it, so the lifetime of the iterator isn't long enough, though I think there's a way around this. This particular case was a hack to get around it :P

so it wastes cycles and keystrokes if you want to edit multiple things at once

You would never edit a PeerId at all, you'd edit it's associated PeerInfo, and then really only through the Peerstore, this is exactly why I feel the two structures need to be more strongly linked, i'm just not exactly sure how.

My recommendation is an associated type that implements Iterator<Item = Multiaddr> and defaults to Vec.

I'm not sure that Iterator would be enough. PeerInfo needs to own the datastructure completely because it actually needs to do some stuff like deduplicating it (or updating the TTL) if another Multiaddr is added, and remove stuff if the TTL expires.

[eira-fransham]

So hopefully Multiaddr will implement Hash soon enough. There are a bunch of problems I have with that API and internal representation but I guess it's good enough for now. If the PeerId is immutable and the PeerInfo is mutable then can we just expose something similar to the entry API and keep them seperate?

[tomaka]

#38 is an initial implementation, but chances are that it will require some modifications in the future.

[folsen]

Closing this in favor of more specific issues needed for potential modifications in the future.