feat: address and proto books #590

vasco-santos · 2020-03-24T14:24:48Z

AddressBook and ProtoBook

This is the initial milestone regarding js-libp2p PeerStore improvements described on libp2p/js-libp2p#582

This PR restructures the initial version of peerStore, where we simply had a record of PeerInfos into data structures with different concerns and own logic.

We will start by introducing the AddressBook and ProtoBook since this was the content being used from the PeerInfos previously stored. In next PRs we will add other "books", namely the keybook and metadata book.

AddressBook implementation (initial version implementated)
ProtoBook implementation (initial version implementated)
PeerStore core
Change PeerStore usage to be compliant with the new implementation
Document PeerStore (initial version written)
Update docs/API.md
Remove peer-info from Ping (initial version implemented)

PeerStore usage

Change Peers data

identifyService
- pushToPeerStore - identify push (on libp2p.handle, libp2p.unhandle)
  - iterate peerStore peers, can iterate on ProtoBook instead?
- After identify, replace peer's data
- After handle push message, replace peer's data
Upgrader: onConnection handler
- Silent put to PeerStore
  - add to AddressBook
Discovery
- add Peer with multiaddr to peerStore
DHT
- Get/has operations
- Put operations when receiving peers from other peers
  - DHT Message contains addresses to peers

Get Peers data

Registrar
- topology needs to listen for protocol:change event and to iterate on all the known peers to check if they run the protocol
- topology iterates on all the peers. if they support the protocol, they add it to the topology
dialProtocol
- if received PeerInfo to dial (deprecate!)
Dialer
- Uses peerStore.multiaddrsForPeer to get the multiaddrs for a PeerInfo
- PeerInfo widely used internally, need to remove it and use peerId + addressBook content
getPeerInfo
- Change with PeerInfo refactor?
ping
- receives peerInfo and libp2p, but can simply receive peerId

Datastructure

The books datastructures where designed with consistency in mind among them and with their underlying data structure (Map). All should contain the main map methods set, get, has and delete, which should work in the same way as the map methods by default, but with some extra optional parameters to add additional super powers.

They were separated on their own instead of living on the top level peerStore for some reasons:

separation of concerns and logic
in the future some will need runtime pruning, such as multiaddr cleanning on ttls, scorings, ...

set(peerId, data, [options = { replace = true }]) : `Array<data>`

Set known data {Array<multiaddr>, Array<string>, ...} of a provided peer. By default, if previous data exists, it will replace it. If the replace options is explicitly defined to false, the data will be merged.

When the recorded data is inserted a new event should be triggered for the interested parties. If a record for the provided peer already exists and the new data is not relevant (i.e the final content will be the same) the operation just returns the final content, but no event is triggered for efficiency.

get(peerId) : `Array<data>`

Get known data of a provided peer. If the peer does not exist, it returns undefined.

has(peerId) : `boolean`

Do we have data for this peer?

delete(peerId, [data]) : `boolean`

Deletes the provided peer from the book. If data is provided, just remove the data

Returns true if the peer's data was fond and the data was deleted (or the partial data did not already exist). If data is modified or removed, the proper event change is triggered to the interest parties.

Concern: what if the array of data gets empty? should we remove the peer? I don't see any good reason to keep it!

General open questions

Who handles the emit peer?
- When we known an address, so it should be when added to the AddressBook!

vasco-santos · 2020-03-24T14:30:54Z

@jacobheun I made a first pass through the PeerStore improvements milestone 1.

This includes a brief explanation on the first post of the PR with PeerStore current usage and data structures decision. Moreover, it includes a README file with the PeerStore goals and a brief writing on its implementation and API.

I would say that for now, it would be great to have your feedback on the questions/decisions written on the PR and on the README (via PR review comments). You can also look into the structure and APIs for the AddressBook and ProtoBook but while we do not discuss what I previously mentioned things might need to be changed or completed.

src/peer-store/README.md

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

…dules

vasco-santos · 2020-03-31T13:42:26Z

@jacobheun this is now ready to review

It did not introduce any breaking change yet, as I left the older API of peerStore with adapters for the new addressBook and protoBook, since other modules (like DHT, ...) are still using them. Anyway, all the internal usage of the peerStore, including the tests now use the new API.

I will do a follow up PR with the breaking change for the peerStore API with the PRs of the modules using it. This will precede the breaking change PRs to remove peer-info usages from internal code and API

jacobheun

Havent gone through everything yet, but sharing the review early as there is enough to go on while I go through the rest.

doc/API.md

src/identify/index.js

src/index.js

src/peer-store/README.md

jacobheun · 2020-04-01T17:49:20Z

src/peer-store/README.md

I wouldn't mention the Map piece. It's not compliant with a Map and we can't iterate on them, so I would avoid comparing the two.

It should gather environment changes, be able to take decisions and notice interested parties of relevant changes.

Can you clarify this? It's sounds like the PeerStore tries to be smart ("decisions"), it should not be smart. It should be the single source of truth for all peer data.

If a subsystem learns about peer data it needs to go directly to the peer store.

If any system/application/person wants to know about peers or listen for updates, they need to go to the peerstore.

jacobheun · 2020-04-01T17:54:11Z

src/peer-store/README.md

+The PeerStore manages the high level operations on its inner books. Moreover, the peerStore should be responsible for notifying interested parties of relevant events, through its Event Emitter.
+
+(Future considerations: Peerstore should manage a job runner to trigger books runners for data trimming or computations)


This doesn't provide enough information for future devs (including us). We should make a dedicated Future considerations header and be a bit more descriptive about what the PeerStore might wish to do and why. What kind of data trimmings? What computations? These can be concise, but should be clear. For example:

## Future Considerations - If multiaddr TTLs are added, the PeerStore may schedule jobs to delete all addresses that exceed the TTL to prevent AddressBook bloating

jacobheun · 2020-04-02T11:51:52Z

doc/API.md

+
+Find the stored information of a given peer.
+
+`peerStore.find(peerId)`


Should we just call this peerStore.get(peerId)? I'm worried users might misinterpret this as a network search for the peer.

Yes, I agree! The question is, should I do it now, or in the next PR? (this will break the return value)

jacobheun

Some additional things.

I was thinking it might be helpful to add some tests that exercise some user scenarios. Such as:

I want to find all peers who support protocol x.
Given an address with no peer id, I want to find all peers that have this address.

This might help us catch any usability issues up front.

jacobheun · 2020-04-02T11:59:27Z

src/peer-store/book.js

+  get (peerId) {
+    if (!PeerId.isPeerId(peerId)) {
+      throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
+    }


I think we can skip the type checks of peerId in these methods. We're documenting everything, so I feel like these are unnecessary restrictions.

I personally don't like to see cannot access toString() of undefined as an error, as someone needs to go into the code/docs and see what is the issue. This way, I think the error is useful here. if we do not try to access properties of it, I would agree.

We can table this discussion for outside of this PR as we're already doing similar checks elsewhere. In short, I'd rather see us move to support static type checking for users that want it instead of doing runtime checks. While they're not big, doing these type checks everywhere slowly adds to bundle size as well.

Should we open a general issue to discuss this?

jacobheun · 2020-04-02T12:01:09Z

src/peer-store/book.js

+      throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
+    }
+
+    const rec = this.data.get(peerId.toString())


Use PeerId.toB58String everywhere, until we do a targeted switch to base32, otherwise we risk hitting unexpected behavior.

Suggested change

const rec = this.data.get(peerId.toString())

const rec = this.data.get(peerId.toB58String())

jacobheun · 2020-04-02T12:04:35Z

src/peer-store/address-book.js

+   * @property {number} validity NOT USED YET
+   * @property {number} confidence NOT USED YET


We should just remove these until they're in place.

Suggested change

* @property {number} validity NOT USED YET

* @property {number} confidence NOT USED YET

jacobheun · 2020-04-02T12:06:04Z

src/peer-store/address-book.js

+     * "peer" - emitted when a peer is discovered by the node.
+     * "change:multiaddrs" - emitted when the known multiaddrs of a peer change.
+     */
+    this._ps = peerStore


Should this just be set in the Book base class?

jacobheun · 2020-04-02T12:08:43Z

src/peer-store/address-book.js

+      throw errcode(new Error('addresses must be provided'), ERR_INVALID_PARAMETERS)
+    }
+
+    if (!Array.isArray(addresses)) {


Should we just simplify the API now and require set be provided an array? We could avoid these checks, and just let addresses.forEach throw the error if it's not an array.

jacobheun · 2020-04-02T12:45:11Z

src/peer-store/address-book.js

+    // Add recorded uniquely to the new array (Union)
+    rec && rec.forEach((mi) => {
+      if (!multiaddrInfos.find(r => r.multiaddr.equals(mi.multiaddr))) {
+        multiaddrInfos.push(mi)


We should probably avoid mutating multiaddrInfos and rec here. I'd suggest composing a new array for the union and setting that as the new record. This would help prevent accidental modification of the underlying records.

I am not sure how this was before I moving code around to get set and add. However, now the multiaddrInfos is created by this._toMultiaddrInfos() function and it is "pushed" with data from the current record, and then set as the new data and not returned.

So, the only possible scenario of second effects here that I can see is if someone did get before and then changes the data after the add. So, I would say that for being safe we only need to destruct the record m?

Other scenario is if someone changes the multiaddr provided! So yes, it might be better to create a new array

but if we just create a new array, I think we will not solve any of these scenarios

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

jacobheun

Just some minor things and API consistency changes. Also added some things to think about for followup PRs.

FYI: I updated the base branch of the PR to be 0.28.x instead of master

doc/API.md

src/peer-store/address-book.js

src/peer-store/proto-book.js

jacobheun · 2020-04-09T10:18:48Z

src/registrar.js

-    if (!PeerInfo.isPeerInfo(peerInfo)) {
-      throw errcode(new Error('peerInfo must be an instance of peer-info'), ERR_INVALID_PARAMETERS)
-    }
+    // TODO: This is not a `peer-info` instance anymore, but an object with the data.


Perhaps to avoid confusion, when we get rid of PeerInfo we should start calling variables peerData?

Yes, I was having a hard time with this naming confusion. Good suggestion! 👍

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

jacobheun

Looks good 👍

* feat: address and proto books * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <jacobheun@gmail.com> * chore: minor fixes and initial tests added * chore: integrate new peer-store with code using adapters for other modules * chore: do not use peerstore.put on get-peer-info * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <jacobheun@gmail.com> * chore: add new peer store tests * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <jacobheun@gmail.com> Co-authored-by: Jacob Heun <jacobheun@gmail.com>

vasco-santos force-pushed the feat/address-and-proto-books branch from 655fe5d to d118b4d Compare March 24, 2020 14:25

vasco-santos mentioned this pull request Mar 25, 2020

PeerStore improvements #582

Closed

vasco-santos force-pushed the feat/address-and-proto-books branch from d118b4d to de32f25 Compare March 25, 2020 12:44

jacobheun reviewed Mar 25, 2020

View reviewed changes

vasco-santos force-pushed the feat/address-and-proto-books branch 9 times, most recently from 9f5f43a to 84a4c8a Compare March 31, 2020 11:53

vasco-santos and others added 3 commits March 31, 2020 13:53

feat: address and proto books

a1617e6

chore: apply suggestions from code review

8dab477

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

chore: minor fixes and initial tests added

72dea96

vasco-santos force-pushed the feat/address-and-proto-books branch from 84a4c8a to 63583ab Compare March 31, 2020 11:55

chore: integrate new peer-store with code using adapters for other mo…

d3e4ad2

…dules

vasco-santos force-pushed the feat/address-and-proto-books branch from 63583ab to d3e4ad2 Compare March 31, 2020 13:36

vasco-santos marked this pull request as ready for review March 31, 2020 13:42

vasco-santos requested a review from jacobheun March 31, 2020 13:46

This was referenced Apr 1, 2020

chore: use new peer store api libp2p/js-libp2p-kad-dht#179

Merged

chore: deprecate old peer store api #598

Merged

vasco-santos force-pushed the feat/address-and-proto-books branch from b3df579 to c4df833 Compare April 1, 2020 09:21

chore: do not use peerstore.put on get-peer-info

eb27b8d

vasco-santos force-pushed the feat/address-and-proto-books branch from c4df833 to eb27b8d Compare April 1, 2020 12:46

This was referenced Apr 1, 2020

chore: update peer store api and libp2p api without peer-info libp2p/js-libp2p-daemon#38

Merged

chore: update libp2p daemon with peer-store libp2p/interop#36

Merged

jacobheun reviewed Apr 1, 2020

View reviewed changes

vasco-santos force-pushed the feat/address-and-proto-books branch 2 times, most recently from d645b63 to b06040b Compare April 2, 2020 09:08

jacobheun reviewed Apr 2, 2020

View reviewed changes

vasco-santos force-pushed the feat/address-and-proto-books branch 2 times, most recently from f521893 to 30b0ae9 Compare April 2, 2020 17:05

chore: apply suggestions from code review

f938535

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

vasco-santos force-pushed the feat/address-and-proto-books branch from 30b0ae9 to f938535 Compare April 2, 2020 17:47

chore: add new peer store tests

1662065

jacobheun changed the base branch from master to 0.28.x April 9, 2020 10:22

jacobheun suggested changes Apr 9, 2020

View reviewed changes

chore: apply suggestions from code review

7dcc78e

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

vasco-santos force-pushed the feat/address-and-proto-books branch from db2e890 to 7dcc78e Compare April 9, 2020 13:12

vasco-santos requested a review from jacobheun April 9, 2020 13:24

jacobheun approved these changes Apr 9, 2020

View reviewed changes

jacobheun merged commit c99a7a2 into 0.28.x Apr 9, 2020

jacobheun deleted the feat/address-and-proto-books branch April 9, 2020 14:07

vasco-santos restored the feat/address-and-proto-books branch April 9, 2020 18:36

vasco-santos mentioned this pull request Apr 10, 2020

chore: rewrite peer-store submit and retrieve docs #605

Merged

This was referenced Apr 16, 2020

chore: remove peer info usage #610

Merged

chore: remove peer-info from api libp2p/js-libp2p-daemon-client#25

Merged

vasco-santos mentioned this pull request Apr 27, 2020

feat: peerstore persistence #619

Merged

2 tasks

vasco-santos mentioned this pull request May 4, 2020

feat: keybook #626

Merged

1 task

achingbrain deleted the feat/address-and-proto-books branch May 18, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: address and proto books #590

feat: address and proto books #590

vasco-santos commented Mar 24, 2020 •

edited

vasco-santos commented Mar 24, 2020

vasco-santos commented Mar 31, 2020

jacobheun left a comment

jacobheun Apr 1, 2020

jacobheun Apr 1, 2020

jacobheun Apr 2, 2020

vasco-santos Apr 2, 2020

jacobheun left a comment

jacobheun Apr 2, 2020

vasco-santos Apr 2, 2020 •

edited

jacobheun Apr 3, 2020

vasco-santos Apr 6, 2020

jacobheun Apr 2, 2020

jacobheun Apr 2, 2020

jacobheun Apr 2, 2020

jacobheun Apr 2, 2020

jacobheun Apr 2, 2020

vasco-santos Apr 2, 2020

vasco-santos Apr 2, 2020

vasco-santos Apr 2, 2020

jacobheun left a comment

jacobheun Apr 9, 2020

vasco-santos Apr 9, 2020

jacobheun left a comment

		The PeerStore manages the high level operations on its inner books. Moreover, the peerStore should be responsible for notifying interested parties of relevant events, through its Event Emitter.

		(Future considerations: Peerstore should manage a job runner to trigger books runners for data trimming or computations)


		Find the stored information of a given peer.

		`peerStore.find(peerId)`

	const rec = this.data.get(peerId.toString())
	const rec = this.data.get(peerId.toB58String())

		* @property {number} validity NOT USED YET
		* @property {number} confidence NOT USED YET

feat: address and proto books #590

feat: address and proto books #590

Conversation

vasco-santos commented Mar 24, 2020 • edited

AddressBook and ProtoBook

PeerStore usage

Change Peers data

Get Peers data

Datastructure

set(peerId, data, [options = { replace = true }]) : Array<data>

get(peerId) : Array<data>

has(peerId) : boolean

delete(peerId, [data]) : boolean

General open questions

vasco-santos commented Mar 24, 2020

vasco-santos commented Mar 31, 2020

jacobheun 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

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Apr 2, 2020 • edited

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

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

vasco-santos commented Mar 24, 2020 •

edited

set(peerId, data, [options = { replace = true }]) : `Array<data>`

get(peerId) : `Array<data>`

has(peerId) : `boolean`

delete(peerId, [data]) : `boolean`

vasco-santos Apr 2, 2020 •

edited