From 9a25c844c1e3952f901627f43f81d077f5afbda5 Mon Sep 17 00:00:00 2001 From: Vasco Santos Date: Wed, 6 May 2020 16:55:43 +0200 Subject: [PATCH] chore: apply suggestions from code review Co-authored-by: Jacob Heun --- doc/API.md | 13 ++++---- src/peer-store/README.md | 14 +++++---- src/peer-store/address-book.js | 10 ++++++ src/peer-store/book.js | 5 --- src/peer-store/key-book.js | 24 ++++++++------- src/peer-store/persistent/index.js | 2 +- test/peer-store/key-book.spec.js | 20 ++---------- test/peer-store/peer-store.spec.js | 23 +++----------- test/peer-store/persisted-peer-store.spec.js | 32 ++++++++++---------- 9 files changed, 62 insertions(+), 81 deletions(-) diff --git a/doc/API.md b/doc/API.md index 04278fb475..883c67f5d5 100644 --- a/doc/API.md +++ b/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 \ No newline at end of file diff --git a/src/peer-store/README.md b/src/peer-store/README.md index be2dbb906d..3c1750571d 100644 --- a/src/peer-store/README.md +++ b/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` -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,11 +54,11 @@ 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>` -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 \ No newline at end of file +[peer-id]: https://github.com/libp2p/js-peer-id diff --git a/src/peer-store/address-book.js b/src/peer-store/address-book.js index 32d11fefc4..54b23013c4 100644 --- a/src/peer-store/address-book.js +++ b/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 } diff --git a/src/peer-store/book.js b/src/peer-store/book.js index f2e62eea72..34d77e1da6 100644 --- a/src/peer-store/book.js +++ b/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) } diff --git a/src/peer-store/key-book.js b/src/peer-store/key-book.js index 894b7b9989..a5b26edce3 100644 --- a/src/peer-store/key-book.js +++ b/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)) { diff --git a/src/peer-store/persistent/index.js b/src/peer-store/persistent/index.js index e8e6f9a604..8e7c459fb8 100644 --- a/src/peer-store/persistent/index.js +++ b/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 diff --git a/test/peer-store/key-book.spec.js b/test/peer-store/key-book.spec.js index 1179e1ec5f..df8507e3bf 100644 --- a/test/peer-store/key-book.spec.js +++ b/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') @@ -36,7 +35,7 @@ describe('keyBook', () => { 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 +46,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) - }) }) diff --git a/test/peer-store/peer-store.spec.js b/test/peer-store/peer-store.spec.js index 4f65eb1511..1884ea4dc6 100644 --- a/test/peer-store/peer-store.spec.js +++ b/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() }) }) diff --git a/test/peer-store/persisted-peer-store.spec.js b/test/peer-store/persisted-peer-store.spec.js index 0da71e15f8..da5d434f40 100644 --- a/test/peer-store/persisted-peer-store.spec.js +++ b/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) @@ -114,7 +114,7 @@ describe('Persisted PeerStore', () => { 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', 4) // 2 AddressBook + 2 ProtoBook expect(peerStore.peers.size).to.equal(2) await peerStore.stop() @@ -127,8 +127,8 @@ 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', 4) + expect(spyDs).to.have.property('callCount', 4) expect(peerStore.peers.size).to.equal(2) expect(peerStore.addressBook.data.size).to.equal(2) @@ -159,7 +159,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 +201,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,7 +215,7 @@ 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 @@ -223,7 +223,7 @@ describe('Persisted PeerStore', () => { 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 +240,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 +252,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 +261,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 +354,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)