feat: keybook #626

vasco-santos · 2020-05-04T07:41:45Z

This PR is part of the PeerStore improvements EPIC and tackles part of the milestone 4 - KeyBook.

As a follow up of the work in #590, #598, #610 and #619 , this PR adds they KeyBook and integrates it with the PeerStorePersistence.

Needs:

feat: peerstore persistence #619

doc/API.md

jacobheun · 2020-05-06T13:28:08Z

doc/API.md

+
+Set known `peerId`. This can include its Public Key.
+
+`peerStore.keyBook.set(peerId)`


What happens if there is no public key on the PeerId instance?

Just keeps the PeerId in it, and might update in the future. I would see the peer-id existing when we discover a new peer

src/peer-store/README.md

jacobheun · 2020-05-06T13:39:31Z

src/peer-store/key-book.js

+    const id = peerId.toB58String()
+    const recPeerId = this.data.get(id)
+
+    !recPeerId && this._ps.emit('peer', peerId)


We should avoid other things triggering events on the peer store and instead rely on the peer store to determine when it should be emitting things. If we don't isolate we'll run the risk of having what we used to have, where several things are emitting peer events which can result in a lot of unnecessary events.

It would probably be good to take a step back and think about the flow of the peers data into the peer store. I dont think that updating the keybook anytime we learn about the peer is the intuitive action. Updating the keybook after crypto negotiation makes sense.

It might be good to revisit the peer store readme and make sure that is up to date and makes sense with these flows.

We should avoid other things triggering events on the peer store and instead rely on the peer store to determine when it should be emitting things. If we don't isolate we'll run the risk of having what we used to have, where several things are emitting peer events which can result in a lot of unnecessary events.

I did not understand your point here. Currently, the Book.js base class is emitting the events for changed peers, while this is the previous event emitted when the peer was discovered (in the addressBook), but now that we have the keyBook keeping the peerIds, it feels that this is the place this should be.

It would probably be good to take a step back and think about the flow of the peers data into the peer store. I dont think that updating the keybook anytime we learn about the peer is the intuitive action. Updating the keybook after crypto negotiation makes sense.

It might be good to revisit the peer store readme and make sure that is up to date and makes sense with these flows.

I added it to update the keyBook because I did not want to keep track of the peerIds in two different data structures. If we move into only storing the peerId in the keybook if we have a public key for it, we will need to go back with the peer event into the addressBook.

Updating the keybook after crypto negotiation makes sense, and I forgot that flow. However, subsystems like the dht will also need to be able to set the public key for a peer.

All things considered, should we go into only storing the PeerId when it has a public key (we can reconstruct it into the getters with createFromCID if we do not store it? With that, we do not update the keybook anymore on the other books set and we emit the peer event from the addressBook? This way, the upgrader and the dht would call the keyBook.set()...

If we go this way, should we throw an error if there is no public key provided in the peerId to set? Probably we should change the API into keyBook.set(peerId, publicKey). While the parameter is redundant, it will sign in an easier way to the user that it is expected

src/peer-store/key-book.js

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos · 2020-05-06T19:01:57Z

src/peer-store/README.md

-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`.


@jacobheun from what we discussed, I could only not do this by now.

I think that it makes sense, but afaik noise uses static public keys, which I would not expect to have here. In addition, we will need to provide libp2p to the crypto module for adding the key to the keyBook or change the crypto interface to also require the public key, so that we can update this in the upgrader.js.

We should probably give some thought on this and follow up with a good solution.

Noise has its own keycache and I dont think we need to worry about persistence of the crypto transport keys, just the libp2p id keys. I think this section is achieved by updating the peer after we've established a connection, because the crypto handshake will result in libp2p public key exchange and verification.

For the inbound connection, we do have the public key after the connection being established. The same is not true for the outbound connection. Both for libp2p-secio and libp2p-noise

I went deeper on secio to figure out why this is happening and I found this: https://github.com/libp2p/js-libp2p-secio/blob/master/src/handshake/crypto.js#L66-L73

When an inbound connection, we go to the else statement while in an outbound connection we go to the if side of things. In the if part we basically do not add the public key. If I just add state.id.remote = remoteId inside the if and after the validation condition, it works. Is this the expected?

In noise, I could not follow the flow so easily yet...

Meanwhile, I already added the code for this here and a test. The validation of the outbound connection key being exchanged is currently commented as a result of what I described above

Note: this is fixed in secio now, I'll look into it in noise as part of my work there.

what do you recommend here? change the test configuration to use secio for now and create an issue to track this?

Leave it for now, I'll work on getting a patch for noise to correct this. We can add a TODO item to the peer store epic to track getting this in.

jacobheun

Overall this looks good, just some minor fixes/suggestions.

The PeerId instead of publickey storage in the keybook is a bit odd, although I understand the reasoning. The API here makes it easy to change in the future if we find a better approach, so this is not a blocker for moving forward with this imo.

src/peer-store/persistent/index.js

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

jacobheun · 2020-05-07T13:33:09Z

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

+      expect(err.code).to.equal(ERR_INVALID_PARAMETERS)
+      return
+    }
+    throw new Error('invalid peerId should throw error')


No need to change but you could instead do something like:

expect(() => kb.set('invalid peerId')).to.throw() .that.has.property('code', ERR_INVALID_PARAMETERS)

which avoids potentially forgetting the return/throw statements when a failure occurs.

I tried this out and I got:

AssertionError: expected [Function] to have property 'code'

I will skip this for now, we can revisit later

jacobheun · 2020-05-07T13:34:48Z

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

+
+    const localPeers = libp2p.peerStore.peers
+    expect(localPeers.size).to.equal(1)
+    // const publicKeyInLocalPeer = localPeers.get(remoteIdStr).id.pubKey


Is there an issue to track this? Otherwise just add a TODO in the comments here.

Adding a TODO with an issue in libp2p-noise

Co-authored-by: Jacob Heun <[email protected]>

feat: keybook

6610ef3

vasco-santos force-pushed the feat/keybook branch from bf3d550 to 6610ef3 Compare May 5, 2020 14:10

vasco-santos requested a review from jacobheun May 5, 2020 14:38

vasco-santos marked this pull request as ready for review May 5, 2020 14:38

jacobheun reviewed May 6, 2020

View reviewed changes

vasco-santos force-pushed the feat/keybook branch 9 times, most recently from dd94b20 to 9a25c84 Compare May 6, 2020 18:33

chore: apply suggestions from code review

1c2c432

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/keybook branch from 9a25c84 to 1c2c432 Compare May 6, 2020 18:47

vasco-santos requested a review from jacobheun May 6, 2020 18:58

vasco-santos commented May 6, 2020

View reviewed changes

vasco-santos mentioned this pull request May 7, 2020

chore: use keybook libp2p/js-libp2p-kad-dht#188

Merged

1 task

chore: add keys to keybook on connection upgraded

7969151

vasco-santos mentioned this pull request May 7, 2020

Feature Request: Out-of band cryptographic signatures libp2p/js-peer-id#123

Closed

jacobheun approved these changes May 7, 2020

View reviewed changes

vasco-santos mentioned this pull request May 7, 2020

Populate the remote public key consistently ChainSafe/js-libp2p-noise#58

Closed

chore: apply suggestions from code review

14bd946

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/keybook branch from bea0045 to 14bd946 Compare May 7, 2020 13:58

vasco-santos merged commit 9eaed58 into 0.28.x May 7, 2020

vasco-santos mentioned this pull request May 14, 2020

chore: uncomment local peer public key after connect test #636

Merged

vasco-santos mentioned this pull request Jun 2, 2020

PeerStore Persistence #591

Closed

achingbrain deleted the feat/keybook branch May 18, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: keybook #626

feat: keybook #626

vasco-santos commented May 4, 2020 •

edited

Loading

jacobheun May 6, 2020

vasco-santos May 6, 2020

jacobheun May 6, 2020

jacobheun May 6, 2020

vasco-santos May 6, 2020

vasco-santos May 6, 2020

vasco-santos May 6, 2020 •

edited

Loading

jacobheun May 6, 2020

vasco-santos May 7, 2020 •

edited

Loading

vasco-santos May 7, 2020 •

edited

Loading

jacobheun May 7, 2020

vasco-santos May 7, 2020

jacobheun May 7, 2020

jacobheun left a comment

jacobheun May 7, 2020

vasco-santos May 7, 2020

jacobheun May 7, 2020

vasco-santos May 7, 2020


		Set known `peerId`. This can include its Public Key.

		`peerStore.keyBook.set(peerId)`

feat: keybook #626

feat: keybook #626

Conversation

vasco-santos commented May 4, 2020 • edited Loading

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

vasco-santos May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos May 7, 2020 • edited Loading

Choose a reason for hiding this comment

vasco-santos May 7, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented May 4, 2020 •

edited

Loading

vasco-santos May 6, 2020 •

edited

Loading

vasco-santos May 7, 2020 •

edited

Loading

vasco-santos May 7, 2020 •

edited

Loading