Feat/peerstore impl #505

jm-clius · 2021-01-15T09:39:15Z

This PR closes #504.

It adds a KeyBook, ProtoBook and MetadataBook to the PoC Peer Store implementation.

Miscellaneous changes

Extracted handlers from an encapsulating EventListener object (as per this conversation)

Suggested next steps:

Coordinate on best way to provide rudimentary peer management using the Peer Store. WakuRelay will specifically benefit from something like a dialer interface from where dialed peers can be added to a store, reconnected when connection dropped, etc. Parts of this may overlap with efforts elsewhere in nim-libp2p, so proper coordination is necessary. I will therefore propose any next steps as PoC features in the libp2p repo to gain consensus before proceeding.

dryajov · 2021-01-18T18:47:53Z

libp2p/peerstore.nim

+    protoBook*: ProtoBook
+    keyBook*: KeyBook
+    metadataBook*: MetadataBook
+    addrChangeHandlers*: seq[AddrChangeHandler]


Hmm, wouldn't it make more sense to move the change handlers into the specific *Book types? For example, I'm not sure what the functionality of KeyBook.keyChange is and how it relates to keyChangeHandlers? Wouldn't it make more sense to move keyChangeHandlers under KeyBook and get rid of keyChange, or am I missing something?

I agree. :) Improvement in ff7d081

The idea was to create some level of indirection between the multiple public handlers that different clients can register on the Peer Store and the single, private handler that the peer store itself registers per *Book when it initialises those books. Each book then only notifies its encompassing Peer Store of changes and the Peer Store in turn notifies its multiple registered clients (perhaps enforcing optional policy). Currently this serves no more than a conceptual purpose, so it will be cleaner to just move the handlers to the individual books. Thanks!

Yeah, I'm not sure if the "proxy" handler makes sense there - but I understand the idea behind it now.

My perspective on the PeerStore is a wrapper that exposes specific register*Handler methods to each underlying *Book. I think it's cleaner and simpler to reason about, but I'm willing to reconsider it when we might require this functionality.

libp2p/peerstore.nim

dryajov · 2021-01-20T18:54:19Z

@sinkingsugar would love your input on this.

oskarth · 2021-01-25T03:49:11Z

libp2p/peerstore.nim

+    changeHandlers: seq[KeyChangeHandler]
+
+  MetadataBook* = object
+    book*: Table[PeerID, Table[string, seq[byte]]]


Is this completely arbitrary? What do we imagine storing here? (Generally seems useful, mostly curious)

Yes, it's supposed to be a general purpose key-value metadata store per peer. In my mind we can use it to keep performance related metrics, such as peer age, throughput, bandwidth, etc. In the go- and js implementations it's also used to store peer location, libp2p agent version and other peer-related metadata determined during the identify procedure (see e.g. libp2p/js-libp2p#800).

I'm not 100% sure about seq[byte] as it can just hold pure data, no references etc.. but for now it's just fine, it's something that can be fixed when we figure out better.

I noticed no raises which is good and so we can probably add on top {.push raises: [Defect].}

Ah, great catch, lets get that added right at the top of the file @jm-clius!

In fact, to clarify - there rule of thumb is that we use this style everywhere possible, the only place/reason it can't be used is in the presence of async, non-async apis should default to this style.

Ok, great suggestions and I especially like the consistency of the second suggestion. To be clear, do you then suggest that specialized entry objects be defined for AddressEntry, ProtoEntry and KeyEntry, but that the MetadataBook maintains the generic PeerBookEntry[T] so peer book clients can define their own entry types?

I like the generic entry yes!, maybe codegen will be bloaty but that's not our fault (nim things)

Ok, great suggestions and I especially like the consistency of the second suggestion. To be clear, do you then suggest that specialized entry objects be defined for AddressEntry, ProtoEntry and KeyEntry, but that the MetadataBook maintains the generic PeerBookEntry[T] so peer book clients can define their own entry types?

Yep, precisely. In fact, we can make MetadataBook be the base for the more specialised book types as well. The only thing I'm not content with is the implicit entry*: T in PeerBookEntry, I wish Nim had a more concise syntax for that sort of thing.

Yup, this is a very reasonable suggestion, since addresses, keys and protos can be seen as specialised types of metadata in any case.

My only concern would be that the API should still make explicit the subtle differences between the books. For example, an AddressBook maps a peerId to a collection of addrs (and therefore should allow clients to add individual addrs to each collection), whereas a KeyBook only allows a single PublicKey entry per peerId (and therefore only allows setting a singular entry).

I've attempted to do the above in 12e0716. This significantly changes (and simplifies) the PeerStore by making extensive use of generics. Each PeerStore still consists of a specialised addressBook, keyBook and protoBook, but now also contains one "any-purpose" metadataBook of generic type PeerBook[T] (this can easily be changed to a seq of such any-purpose books if there is a need for extensibility). Of course, this does not really allow for run-time object variance in the metadataBook (which I don't think generics are suitable for, in any case?), but does allow clients to specify their own metadata type when instantiating a new PeerStore. I'm not exactly happy with the PeerStore itself being genericised, but since it requires a concrete type when constructing the metadataBook I don't really see a way around this. I'll be glad for any suggestions to make this even more flexible. cc @sinkingsugar @dryajov

Honestly, the concept of a free form metadata store doesn't make much sense in this context and I would just leave it out, allowing anyone who needs additional information to just extend the PeerBook and PeerStores accordingly.

oskarth · 2021-01-25T03:51:26Z

libp2p/peerstore.nim

+  protoBook.book.getOrDefault(peerId,
+                              initHashSet[string]())
+
+proc add*(protoBook: var ProtoBook,


At what point will this be called? Who is responsible for calling it and at what point in the lifecycle?

Since a Peer Store provides a "standalone" API it can theoretically be instantiated and added to (at any time) by any module seeking to provide some level of peer management.
Eventually I envision it to be integrated in such a way that there is a single libp2p instance, with the following integration points (based on js-implementation):

a Dialer (TBD) adds "bootstrap" or statically defined peers when these are dialed. This is most likely the first point of integration for WakuRelay as well. The Dialer would also be responsible for maintaining connections and redialing peers when dropped.

the identify protocol adds information about identified peers.

Discovery/DHT mechanisms adds information about discovered peers.

oskarth · 2021-01-25T03:52:23Z

libp2p/peerstore.nim

+    keyBook.book.del(peerId)
+    return true
+
+proc set*(keyBook: var KeyBook,


Ditto above, where is this called and who is responsible for it? Upon initial connection in nim-libp2p or somewhere else?

oskarth · 2021-01-25T03:55:03Z

libp2p/peerstore.nim

-    # Listener object with client-defined handlers for peer store events
-    addrChange*: AddrChangeHandler
-    # @TODO add handlers for other event types
+  ProtoChangeHandler* = proc(peerId: PeerID, protos: HashSet[string])


Is FooChangeHandler the common name here? Nitpick, but something like FooUpdateHandler might be a tiny bit more descriptive, "Change" to me suggests something could be breaking.

I agree. change was selected to stay as close as possible to similar events and naming in the js-implementation, but I am happy to update naming to update :)

I think the naming convention makes sense here.

sinkingsugar · 2021-01-25T12:34:38Z

@sinkingsugar would love your input on this.

LGTM, Very good style!
I noticed no raises which is good and so we can probably add on top {.push raises: [Defect].}

* Address Book POC implementation (#499) * Address Book POC implementation * Feat/peerstore impl (#505) Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

jm-clius added 2 commits January 15, 2021 11:07

Added missing books

c506272

Extract handlers from the encapsulating listener object.

48be0a7

jm-clius requested review from dryajov and oskarth January 15, 2021 09:39

dryajov reviewed Jan 18, 2021

View reviewed changes

Improvement - move handlers to books

ff7d081

jm-clius requested a review from dryajov January 19, 2021 09:55

dryajov reviewed Jan 19, 2021

View reviewed changes

libp2p/peerstore.nim Outdated Show resolved Hide resolved

Cleanup unnecessary newSeq initialisations

926d340

dryajov requested a review from sinkingsugar January 20, 2021 18:53

oskarth reviewed Jan 25, 2021

View reviewed changes

sinkingsugar approved these changes Jan 25, 2021

View reviewed changes

Genericise PeerStore

12e0716

oskarth approved these changes Jan 26, 2021

View reviewed changes

jm-clius requested a review from dryajov January 27, 2021 13:34

Removed metadataBook (for now)

218ca64

dryajov approved these changes Jan 28, 2021

View reviewed changes

jm-clius merged commit 090ea91 into unstable Jan 29, 2021

jm-clius deleted the feat/peerstore-impl branch January 29, 2021 08:43

dryajov added a commit that referenced this pull request Feb 8, 2021

Merge unstable (#518)

2658181

* Address Book POC implementation (#499) * Address Book POC implementation * Feat/peerstore impl (#505) Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/peerstore impl #505

Feat/peerstore impl #505

jm-clius commented Jan 15, 2021

dryajov Jan 18, 2021

jm-clius Jan 19, 2021

dryajov Jan 19, 2021

dryajov commented Jan 20, 2021

oskarth Jan 25, 2021

jm-clius Jan 25, 2021

sinkingsugar Jan 25, 2021

dryajov Jan 25, 2021

dryajov Jan 25, 2021

jm-clius Jan 26, 2021

sinkingsugar Jan 26, 2021

dryajov Jan 26, 2021

jm-clius Jan 27, 2021

dryajov Jan 27, 2021

oskarth Jan 25, 2021

jm-clius Jan 25, 2021

oskarth Jan 25, 2021

oskarth Jan 25, 2021

jm-clius Jan 25, 2021

dryajov Jan 25, 2021

sinkingsugar commented Jan 25, 2021

Feat/peerstore impl #505

Feat/peerstore impl #505

Conversation

jm-clius commented Jan 15, 2021

Miscellaneous changes

Suggested next steps:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov commented Jan 20, 2021

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

sinkingsugar commented Jan 25, 2021