chore: apply suggestions from code review

Co-authored-by: Jacob Heun <jacobheun@gmail.com>

Author: vasco-santos

doc/API.md

@@ -814,8 +814,6 @@ Add known `protocols` of a given peer.
 peerStore.protoBook.add(peerId, protocols)
 ```
 
-* [`peerStore.keyBook.get`](#peerstorekeybookget)
-* [`peerStore.keyBook.set`](#peerstorekeybookset)
 
 ### peerStore.keyBook.delete
 
@@ -840,7 +838,7 @@ Delete the provided peer from the book.
 ```js
 peerStore.keyBook.delete(peerId)
 // false
-peerStore.keyBook.set(peerId)
+peerStore.keyBook.set(peerId, publicKey)
 peerStore.keyBook.delete(peerId)
 // true
 ```
@@ -868,7 +866,7 @@ Get the known `PublicKey` of a provided peer.
 ```js
 peerStore.keyBook.get(peerId)
 // undefined
-peerStore.keyBook.set(peerId) // with inline public key
+peerStore.keyBook.set(peerId, publicKey)
 peerStore.keyBook.get(peerId)
 // PublicKey
 ```
@@ -877,13 +875,14 @@ peerStore.keyBook.get(peerId)
 
 Set known `peerId`. This can include its Public Key.
 
-`peerStore.keyBook.set(peerId)`
+`peerStore.keyBook.set(peerId, publicKey)`
 
 #### Parameters
 
 | Name | Type | Description |
 |------|------|-------------|
 | peerId | [`PeerId`][peer-id] | peerId to set |
+| publicKey | [`RsaPublicKey|Ed25519PublicKey|Secp256k1PublicKey`][keys] | peer's public key |
 
 #### Returns
 
@@ -894,7 +893,8 @@ Set known `peerId`. This can include its Public Key.
 #### Example
 
 ```js
-peerStore.keyBook.set(peerId)
+const publicKey = peerId.pubKey
+peerStore.keyBook.set(peerId, publicKey)
 ```
 
 ### peerStore.protoBook.delete
@@ -1420,3 +1420,4 @@ This event will be triggered anytime we are disconnected from another peer, rega
 [connection]: https://github.com/libp2p/js-interfaces/tree/master/src/connection
 [multiaddr]: https://github.com/multiformats/js-multiaddr
 [peer-id]: https://github.com/libp2p/js-peer-id
+[keys]: https://github.com/libp2p/js-libp2p-crypto/tree/master/src/keys

src/peer-store/README.md

@@ -10,7 +10,9 @@ Several libp2p subsystems will perform operations, which will gather relevant in
 
 In a libp2p node's life, it will discover peers through its discovery protocols. In a typical discovery protocol, addresses of the peer are discovered along with its peer id. Once this happens, the PeerStore should collect this information for future (or immediate) usage by other subsystems. When the information is stored, the PeerStore should inform interested parties of the peer discovered (`peer` event).
 
-Taking into account a different scenario, a peer might perform/receive a dial request to/from a unkwown peer. In such a scenario, the PeerStore must store the peer's multiaddr once a connection is established. 
+Taking into account a different scenario, a peer might perform/receive a dial request to/from a unkwown peer. In such a scenario, the PeerStore must store the peer's multiaddr once a connection is established.
+
+When a connection is being upgraded, more precisely after its encryption, or even in a discovery protocol, a libp2p node can get to know other parties public keys. In this scenario, libp2p will add the peer's public key to its `KeyBook`.
 
 After a connection is established with a peer, the Identify protocol will run automatically. A stream is created and peers exchange their information (Multiaddrs, running protocols and their public key). Once this information is obtained, it should be added to the PeerStore. In this specific case, as we are speaking to the source of truth, we should ensure the PeerStore is prioritizing these records. If the recorded `multiaddrs` or `protocols` have changed, interested parties must be informed via the `change:multiaddrs` or `change:protocols` events respectively.
 
@@ -42,7 +44,7 @@ The `addressBook` keeps the known multiaddrs of a peer. The multiaddrs of each p
 
 `Map<string, Address>`
 
-A `peerId.toString()` identifier mapping to a `Address` object, which should have the following structure:
+A `peerId.toB58String()` identifier mapping to a `Address` object, which should have the following structure:
 
 ```js
 {
@@ -52,19 +54,19 @@ A `peerId.toString()` identifier mapping to a `Address` object, which should hav
 
 #### Key Book
 
-The `keyBook` tracks the publick keys of the peers by keeping their [`PeerId`][peer-id].
+The `keyBook` tracks the public keys of the peers by keeping their [`PeerId`][peer-id].
 
 `Map<string, PeerId`
 
-A `peerId.toString()` identifier mapping to a `PeerId` of the peer. This instance contains the peer public key.
+A `peerId.toB58String()` identifier mapping to a `PeerId` of the peer. This instance contains the peer public key.
 
 #### Protocol Book
 
 The `protoBook` holds the identifiers of the protocols supported by each peer. The protocols supported by each peer are dynamic and will change over time.
 
 `Map<string, Set<string>>`
 
-A `peerId.toString()` identifier mapping to a `Set` of protocol identifier strings.
+A `peerId.toB58String()` identifier mapping to a `Set` of protocol identifier strings.
 
 #### Metadata Book
 
@@ -129,4 +131,4 @@ Metadata is stored under the following key pattern:
 - When improving libp2p configuration for specific runtimes, we should take into account the PeerStore recommended datastore.
 - When improving libp2p configuration, we should think about a possible way of allowing the configuration of Bootstrap to be influenced by the persisted peers, as a way to decrease the load on Bootstrap nodes.
 
-[peer-id]: https://github.com/libp2p/js-peer-id
+[peer-id]: https://github.com/libp2p/js-peer-id

src/peer-store/address-book.js

@@ -86,6 +86,11 @@ class AddressBook extends Book {
     this._setData(peerId, addresses)
     log(`stored provided multiaddrs for ${id}`)
 
+    // Notify the existance of a new peer
+    if (!rec) {
+      this._ps.emit('peer', peerId)
+    }
+
     return this
   }
 
@@ -125,6 +130,11 @@ class AddressBook extends Book {
 
     log(`added provided multiaddrs for ${id}`)
 
+    // Notify the existance of a new peer
+    if (!rec) {
+      this._ps.emit('peer', peerId)
+    }
+
     return this
   }
 

src/peer-store/book.js

@@ -58,11 +58,6 @@ class Book {
     // Store data in memory
     this.data.set(b58key, data)
 
-    // Store PeerId
-    if (!PeerId.isPeerId(data)) {
-      this._ps.keyBook.set(peerId)
-    }
-
     // Emit event
     emit && this._emit(peerId, data)
   }

src/peer-store/key-book.js

@@ -24,7 +24,7 @@ class KeyBook extends Book {
   constructor (peerStore) {
     super({
       peerStore,
-      eventName: 'change:pubkey', // TODO: the name is not probably the best!?
+      eventName: 'change:pubkey',
       eventProperty: 'pubkey',
       eventTransformer: (data) => data.pubKey
     })
@@ -37,12 +37,13 @@ class KeyBook extends Book {
   }
 
   /**
-   * Set PeerId. If the peer was not known before, it will be added.
+   * Set the Peer public key.
    * @override
    * @param {PeerId} peerId
+   * @param {RsaPublicKey|Ed25519PublicKey|Secp256k1PublicKey} publicKey
    * @return {KeyBook}
- */
-  set (peerId) {
+  */
+  set (peerId, publicKey) {
     if (!PeerId.isPeerId(peerId)) {
       log.error('peerId must be an instance of peer-id to store data')
       throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
@@ -51,12 +52,13 @@ class KeyBook extends Book {
     const id = peerId.toB58String()
     const recPeerId = this.data.get(id)
 
-    !recPeerId && this._ps.emit('peer', peerId)
-    // If no record available, or it is incomplete
-    if (!recPeerId || (peerId.pubKey && !recPeerId.pubKey)) {
-      this._setData(peerId, peerId, {
-        emit: Boolean(peerId.pubKey) // No persistence if no public key
-      })
+    // If no record available, and this is valid
+    if (!recPeerId && publicKey) {
+      // This might be unecessary, but we want to store the PeerId
+      // to avoid an async operation when reconstructing the PeerId
+      peerId.pubKey = publicKey
+
+      this._setData(peerId, peerId)
       log(`stored provided public key for ${id}`)
     }
 
@@ -67,7 +69,7 @@ class KeyBook extends Book {
    * Get Public key of the given PeerId, if stored.
    * @override
    * @param {PeerId} peerId
-   * @return {PublicKey}
+   * @return {RsaPublicKey|Ed25519PublicKey|Secp256k1PublicKey}
    */
   get (peerId) {
     if (!PeerId.isPeerId(peerId)) {

src/peer-store/persistent/index.js

@@ -111,7 +111,7 @@ class PersistentPeerStore extends PeerStore {
     log('create batch commit')
     const batch = this._datastore.batch()
     for (const peerIdStr of commitPeers) {
-      // PeerId (replace by keyBook)
+      // PeerId
       const peerId = this.keyBook.data.get(peerIdStr) || PeerId.createFromB58String(peerIdStr)
 
       // Address Book

test/peer-store/key-book.spec.js

@@ -7,7 +7,6 @@ chai.use(require('chai-bytes'))
 const { expect } = chai
 const sinon = require('sinon')
 
-const PeerId = require('peer-id')
 const PeerStore = require('../../src/peer-store')
 
 const peerUtils = require('../utils/creators/peer')
@@ -24,7 +23,7 @@ describe('keyBook', () => {
     kb = peerStore.keyBook
   })
 
-  it('throwns invalid parameters error if invalid PeerId is provided', () => {
+  it('throwns invalid parameters error if invalid PeerId is provided in set', () => {
     try {
       kb.set('invalid peerId')
     } catch (err) {
@@ -34,9 +33,19 @@ describe('keyBook', () => {
     throw new Error('invalid peerId should throw error')
   })
 
+  it('throwns invalid parameters error if invalid PeerId is provided in get', () => {
+    try {
+      kb.get('invalid peerId')
+    } catch (err) {
+      expect(err.code).to.equal(ERR_INVALID_PARAMETERS)
+      return
+    }
+    throw new Error('invalid peerId should throw error')
+  })
+
   it('stores the peerId in the book and returns the public key', () => {
     // Set PeerId
-    kb.set(peerId)
+    kb.set(peerId, peerId.pubKey)
 
     // Get public key
     const pubKey = kb.get(peerId)
@@ -47,22 +56,9 @@ describe('keyBook', () => {
     const spy = sinon.spy(kb, '_setData')
 
     // Set PeerId
-    kb.set(peerId)
-    kb.set(peerId)
+    kb.set(peerId, peerId.pubKey)
+    kb.set(peerId, peerId.pubKey)
 
     expect(spy).to.have.property('callCount', 1)
   })
-
-  it('stores if already stored but there was no public key stored', () => {
-    const spy = sinon.spy(kb, '_setData')
-
-    // Set PeerId without public key
-    const p = PeerId.createFromB58String(peerId.toB58String())
-    kb.set(p)
-
-    // Set complete peerId
-    kb.set(peerId)
-
-    expect(spy).to.have.property('callCount', 2)
-  })
 })

test/peer-store/peer-store.node.js

@@ -0,0 +1,34 @@
+'use strict'
+/* eslint-env mocha */
+
+const chai = require('chai')
+chai.use(require('dirty-chai'))
+chai.use(require('chai-as-promised'))
+const { expect } = chai
+const sinon = require('sinon')
+
+const baseOptions = require('../utils/base-options')
+const peerUtils = require('../utils/creators/peer')
+
+describe('libp2p.peerStore', () => {
+  let libp2p, remoteLibp2p
+
+  beforeEach(async () => {
+    [libp2p, remoteLibp2p] = await peerUtils.createPeer({
+      number: 2,
+      populateAddressBooks: false,
+      config: {
+        ...baseOptions
+      }
+    })
+  })
+
+  it('adds peer address to AddressBook when establishing connection', async () => {
+    const spyAddressBook = sinon.spy(libp2p.peerStore.addressBook, 'add')
+    const remoteMultiaddr = `${remoteLibp2p.multiaddrs[0]}/p2p/${remoteLibp2p.peerId.toB58String()}`
+    const conn = await libp2p.dial(remoteMultiaddr)
+
+    expect(conn).to.exist()
+    expect(spyAddressBook).to.have.property('callCount', 1)
+  })
+})

test/peer-store/peer-store.spec.js

@@ -4,7 +4,6 @@
 const chai = require('chai')
 chai.use(require('dirty-chai'))
 const { expect } = chai
-const sinon = require('sinon')
 
 const PeerStore = require('../../src/peer-store')
 const multiaddr = require('multiaddr')
@@ -50,25 +49,11 @@ describe('peer-store', () => {
       expect(peer).to.not.exist()
     })
 
-    it('sets the peer to the KeyBook when added to the AddressBook', () => {
-      const spyPeerStore = sinon.spy(peerStore.keyBook, 'set')
+    it('sets the peer\'s public key to the KeyBook', () => {
+      peerStore.keyBook.set(peerIds[0], peerIds[0].pubKey)
 
-      peerStore.addressBook.set(peerIds[0], [addr1, addr2])
-      expect(spyPeerStore).to.have.property('callCount', 1)
-    })
-
-    it('sets the peer to the KeyBook when added to the ProtoBook', () => {
-      const spyPeerStore = sinon.spy(peerStore.keyBook, 'set')
-
-      peerStore.protoBook.set(peerIds[0], [proto1])
-      expect(spyPeerStore).to.have.property('callCount', 1)
-    })
-
-    it('does not re-set the to the KeyBook when directly added to it', () => {
-      const spyPeerStore = sinon.spy(peerStore.keyBook, 'set')
-
-      peerStore.keyBook.set(peerIds[0])
-      expect(spyPeerStore).to.have.property('callCount', 1)
+      const pubKey = peerStore.keyBook.get(peerIds[0])
+      expect(pubKey).to.exist()
     })
   })
 

test/peer-store/persisted-peer-store.spec.js

@@ -68,14 +68,14 @@ describe('Persisted PeerStore', () => {
       // AddressBook
       peerStore.addressBook.set(peer, multiaddrs)
 
-      expect(spyDirty).to.have.property('callCount', 2) // Address + PeerId
-      expect(spyDs).to.have.property('callCount', 2)
+      expect(spyDirty).to.have.property('callCount', 1) // Address
+      expect(spyDs).to.have.property('callCount', 1)
 
       // ProtoBook
       peerStore.protoBook.set(peer, protocols)
 
-      expect(spyDirty).to.have.property('callCount', 3) // Protocol
-      expect(spyDs).to.have.property('callCount', 3)
+      expect(spyDirty).to.have.property('callCount', 2) // Protocol
+      expect(spyDs).to.have.property('callCount', 2)
 
       // Should have three peer records stored in the datastore
       const queryParams = {
@@ -86,7 +86,7 @@ describe('Persisted PeerStore', () => {
       for await (const _ of datastore.query(queryParams)) { // eslint-disable-line
         count++
       }
-      expect(count).to.equal(3)
+      expect(count).to.equal(2)
 
       // Validate data
       const storedPeer = peerStore.get(peer)
@@ -110,11 +110,15 @@ describe('Persisted PeerStore', () => {
       peerStore.addressBook.set(peers[0], [multiaddrs[0]])
       peerStore.addressBook.set(peers[1], [multiaddrs[1]])
 
+      // KeyBook
+      peerStore.keyBook.set(peers[0], peers[0].pubKey)
+      peerStore.keyBook.set(peers[1], peers[1].pubKey)
+
       // ProtoBook
       peerStore.protoBook.set(peers[0], protocols)
       peerStore.protoBook.set(peers[1], protocols)
 
-      expect(spyDs).to.have.property('callCount', 6) // 2 AddressBook + 2 ProtoBook + 2 KeyBook
+      expect(spyDs).to.have.property('callCount', 6) // 2 AddressBook + 2 KeyBook + 2 ProtoBook
       expect(peerStore.peers.size).to.equal(2)
 
       await peerStore.stop()
@@ -127,11 +131,12 @@ describe('Persisted PeerStore', () => {
 
       await peerStore.start()
 
-      expect(spy).to.have.property('callCount', 6) // 6 datastore entries
-      expect(spyDs).to.have.property('callCount', 6) // 6 previous operations
+      expect(spy).to.have.property('callCount', 6)
+      expect(spyDs).to.have.property('callCount', 6)
 
       expect(peerStore.peers.size).to.equal(2)
       expect(peerStore.addressBook.data.size).to.equal(2)
+      expect(peerStore.keyBook.data.size).to.equal(2)
       expect(peerStore.protoBook.data.size).to.equal(2)
     })
 
@@ -159,7 +164,7 @@ describe('Persisted PeerStore', () => {
       expect(spyAddressBook).to.have.property('callCount', 1)
       expect(spyKeyBook).to.have.property('callCount', 1)
       expect(spyProtoBook).to.have.property('callCount', 1)
-      expect(spyDs).to.have.property('callCount', 3)
+      expect(spyDs).to.have.property('callCount', 2)
 
       // Should have zero peer records stored in the datastore
       const queryParams = {
@@ -201,7 +206,7 @@ describe('Persisted PeerStore', () => {
       // Remove data from the same Peer
       peerStore.addressBook.delete(peers[0])
 
-      expect(spyDirty).to.have.property('callCount', 4) // 2 AddrBook ops, 1 ProtoBook op, 1 KeyBook op
+      expect(spyDirty).to.have.property('callCount', 3) // 2 AddrBook ops, 1 ProtoBook op
       expect(peerStore._dirtyPeers.size).to.equal(1)
       expect(spyDs).to.have.property('callCount', 0)
 
@@ -215,15 +220,15 @@ describe('Persisted PeerStore', () => {
       // Add data for second book
       peerStore.addressBook.set(peers[1], multiaddrs)
 
-      expect(spyDirty).to.have.property('callCount', 6)
+      expect(spyDirty).to.have.property('callCount', 4)
       expect(spyDs).to.have.property('callCount', 1)
 
       // Should have two peer records stored in the datastore
       let count = 0
       for await (const _ of datastore.query(queryParams)) { // eslint-disable-line
         count++
       }
-      expect(count).to.equal(4)
+      expect(count).to.equal(2)
       expect(peerStore.peers.size).to.equal(2)
     })
 
@@ -240,7 +245,7 @@ describe('Persisted PeerStore', () => {
       peerStore.protoBook.set(peer, protocols)
 
       expect(spyDs).to.have.property('callCount', 0)
-      expect(spyDirty).to.have.property('callCount', 2) // ProtoBook + KeyBook
+      expect(spyDirty).to.have.property('callCount', 1) // ProtoBook
       expect(peerStore._dirtyPeers.size).to.equal(1)
 
       const queryParams = {
@@ -252,7 +257,7 @@ describe('Persisted PeerStore', () => {
 
       await peerStore.stop()
 
-      expect(spyDirty).to.have.property('callCount', 2)
+      expect(spyDirty).to.have.property('callCount', 1)
       expect(spyDs).to.have.property('callCount', 1)
       expect(peerStore._dirtyPeers.size).to.equal(0) // Reset
 
@@ -261,7 +266,7 @@ describe('Persisted PeerStore', () => {
       for await (const _ of datastore.query(queryParams)) { // eslint-disable-line
         count++
       }
-      expect(count).to.equal(2)
+      expect(count).to.equal(1)
       expect(peerStore.peers.size).to.equal(1)
     })
   })
@@ -354,7 +359,7 @@ describe('libp2p.peerStore (Persisted)', () => {
 
       await newNode.start()
 
-      expect(spy).to.have.property('callCount', 6) // 6 datastore entries
+      expect(spy).to.have.property('callCount', 4) // 4 datastore entries
 
       expect(newNode.peerStore.peers.size).to.equal(2)