Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: certified addressbook #683

Merged
merged 9 commits into from
Jul 23, 2020
Merged

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jun 25, 2020

This PR adds the signed peer records in the AddressBook as part of #653

Remaining:

  • Identify service should use these new features to store self and fetch it

Needs:

The protobuf definition for persistence was inspired (copied) from go-libp2p, and the API to consume and get the records was inspired on go-libp2p CertifiedAddressBook interface.


The AddressBook was previously holding Map<string, Array<Address>>, which mapped a peer id string identifier to an Array of Address (object with multiaddr property). It was changed to Map<string, Array<Entry>>, where the Entry is an object containing { addresses: Array<Address>, record: { raw: Buffer, seqNumber: number} }. The Address now also includes a isCertified property that aims to allow subsystems to filter known multiaddrs of a peer that are certified without a need to unmarshall the record. The raw envelope is stored for being exchanged easily with other peers.

There are 3 remaining questions to be answered:

  1. When the AddressBook receives a new peer record, should it replace the previously stored multiaddrs (not certified) by the new ones, or should it merge? If it merges, when the AddressBook receives a new peer record with different addresses from a previously stored record, what happens to the previously addresses marked as certified?
  2. We emit change:multiaddrs when a peer has new multiaddrs. The question is, what if we already know a multiaddr but it is now certified? From the persistence layer standpoint, it is important to receive the event, but is this expected from a regular listener of the events?
  3. (this we talked before, and we should just delay a decision and add it to Future Work section): When loading from the datastore on restart, should we ignore the multiaddrs stored that are not in the signed peer record?

@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from bb915aa to 9ff5cad Compare June 25, 2020 12:40
@vasco-santos vasco-santos mentioned this pull request Jun 25, 2020
@jacobheun
Copy link
Contributor

  1. When the AddressBook receives a new peer record, should it replace the previously stored multiaddrs (not certified) by the new ones, or should it merge? If it merges, when the AddressBook receives a new peer record with different addresses from a previously stored record, what happens to the previously addresses marked as certified?

Non certified addresses should be expired by TTL. When we receive a new Peer Record, the record with the highest sequence wins, the other certified addresses should be removed.

  1. We emit change:multiaddrs when a peer has new multiaddrs. The question is, what if we already know a multiaddr but it is now certified? From the persistence layer standpoint, it is important to receive the event, but is this expected from a regular listener of the events?

We should trigger the change. While the underlying multiaddr didn't change, our state did. This is helpful for apps that may want to take action when addresses become certified.

  1. (this we talked before, and we should just delay a decision and add it to Future Work section): When loading from the datastore on restart, should we ignore the multiaddrs stored that are not in the signed peer record?

We should check the TTL on load for these and only keep them if it hasn't expired.

I haven't checked the address book code yet, let me know if you want me to give that a pass.

@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch 4 times, most recently from 56e4dd9 to 40f4217 Compare June 29, 2020 12:52
@vasco-santos
Copy link
Member Author

Since we now have self being stored in the AddressBook, should we filter out self in libp2p.peerStore.peers?
Note: the interop tests are breaking because the daemon is breaking because of this (trying to get a connection from self to self as a result of being present in the peerStore.peers)

@wemeetagain
Copy link
Member

Just noticed that gossipsub v1.1 will need to import from this repo at runtime to access Envelope.openAndCertify. Not just a devDependency, as before.
Is this intended?

@vasco-santos
Copy link
Member Author

Just noticed that gossipsub v1.1 will need to import from this repo at runtime to access Envelope.openAndCertify. Not just a devDependency, as before.
Is this intended?

Yes! I am not a fan of requiring stuff as libp2p/src/..., but the alternative is getting a mountain of repos. We are moving forward on having all modules tight with libp2p inside the libp2p repo. We can move it to its own repo/module in the future if we think it is valuable.

@jacobheun jacobheun force-pushed the 0.29.x branch 2 times, most recently from a947283 to 4792ebb Compare July 14, 2020 20:09
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from 40f4217 to 44aef5b Compare July 15, 2020 11:52
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch 6 times, most recently from 4976483 to adad74e Compare July 15, 2020 14:20
@vasco-santos vasco-santos marked this pull request as ready for review July 15, 2020 17:13
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from adad74e to 9ee0499 Compare July 15, 2020 17:22
@vasco-santos vasco-santos requested a review from jacobheun July 15, 2020 17:49
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from 9ee0499 to 864d153 Compare July 15, 2020 19:59
src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/address-book.js Outdated Show resolved Hide resolved
@@ -37,9 +37,11 @@ class PeerStore extends EventEmitter {
/**
* @constructor
*/
constructor () {
constructor ({ peerId } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this optional? I don't see a use case for it and it just adds unnecessary checks when filtering self out.

src/peer-store/persistent/index.js Show resolved Hide resolved
* @param {Buffer} data
* @return {Promise<Envelope>}
*/
Envelope.createFromProtobuf = unmarshalEnvelope
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just set the function here and avoid two names for the same method?

test/peer-store/address-book.spec.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from f812c05 to 90b8761 Compare July 17, 2020 12:35
@vasco-santos vasco-santos force-pushed the feat/certified-addressbook branch from 90b8761 to 5019a60 Compare July 17, 2020 13:02
@vasco-santos vasco-santos requested a review from jacobheun July 17, 2020 13:08
* @param {PeerId} peerId
* @return {Buffer}
*/
getRawEnvelope (peerId) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we add a convenience method for getRawSelfEnvelope?
We are using this providing own peerId in the identify protocol, and I am also using it now in rendezvous

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the need here? We should already have easy access to libp2p, libp2p.peerStore.addressBook.getRawEnvelope(libp2p.peerId), I'm not seeing what we'd gain from this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not needed, but could potentially help. I did not have access to the self peerId in the rendezvous server (in the previous PR state), and I needed to change everything to provide libp2p because of that. But yeah, it was mostly a question, we don't need it, it could just help in some cases

* @param {PeerId} peerId
* @return {Buffer}
*/
getRawEnvelope (peerId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the need here? We should already have easy access to libp2p, libp2p.peerStore.addressBook.getRawEnvelope(libp2p.peerId), I'm not seeing what we'd gain from this.

src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/address-book.js Show resolved Hide resolved
vasco-santos and others added 2 commits July 22, 2020 16:19
@vasco-santos vasco-santos requested a review from jacobheun July 22, 2020 14:39
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

LGTM, just 2 minor language suggestions.

src/peer-store/address-book.js Show resolved Hide resolved
doc/API.md Show resolved Hide resolved
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
@vasco-santos vasco-santos merged commit feed491 into 0.29.x Jul 23, 2020
@vasco-santos vasco-santos deleted the feat/certified-addressbook branch July 23, 2020 12:00
@vasco-santos vasco-santos restored the feat/certified-addressbook branch July 23, 2020 12:00
@vasco-santos vasco-santos deleted the feat/certified-addressbook branch July 23, 2020 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants