Signed Peer Records #630
Signed Peer Records #630
Conversation
There was a problem hiding this comment.
A good start, but there are no tests. In particular, it would be really nice to get some interoperability tests (e.g. with Testground) as a sanity check. Also, we don't do anything about signed peer records for handleGetProviders.
This PR also increases bandwidth usage by sending both signed and unsigned peer records so it would be nice to quantify that in some way. If it's substantial then perhaps it's worth waiting for a DHT version upgrade before doing this so that we don't have to duplicate messages.
We might be able to save bandwidth by only sending the signature + public key instead of the entire signed envelope. Unfortunately the signed peer records were setup as signatures over the protobufs of the PeerRecord struct and protobufs do not have guaranteed canonical serialization...
I'd like to better understand why we're doing this now (as opposed to after we get our network upgrading strategy set up) and how much it's going to cost the network in bandwidth.
| } | ||
| c := ConnectionType(n.Connectedness(peerrec.PeerID)) | ||
|
|
||
| bz, err := peerRecords[i].Marshal() |
| // github.com/libp2p/go-libp2p-core/peer/pb/peer_record.proto for message definitions. | ||
| bytes signedPeerRecord = 1; | ||
| // used to signal the sender's connection capabilities to the peer | ||
| ConnectionType connection = 2; |
There was a problem hiding this comment.
Do we really want to embed the connection here? What do we get out of duplicating this state from the regular peer records?
| continue | ||
| } | ||
|
|
||
| evs[peer.PeerID] = RawSignedPeerRecordConn{ev, peer.Addrs, Connectedness(pbsp[i].Connection)} |
There was a problem hiding this comment.
What do you think about adding in checks here to make sure we haven't been sent multiple records for the same peer?
| // return if we got a nil response | ||
| // This happens if the self peer decides to not run the query RPC on the remote peer for some reason | ||
| if newPeers == nil { | ||
| ch <- &queryUpdate{cause: p, heard: nil, queried: []peer.ID{p}, queryDuration: queryDuration} | ||
| return | ||
| } |
There was a problem hiding this comment.
Maybe I'm just missing it, but where do we get err != nil && newPeers == nil?
| // add any other know addresses for the candidate peer. | ||
| curInfo := q.dht.peerstore.PeerInfo(pid) |
There was a problem hiding this comment.
Why are we doing this? If the signed record on its own doesn't warrant being queried on its own then why should we help it by adding externally learned addresses?
| // process unsigned peers | ||
| for _, next := range newPeers.unsignedPeers { |
There was a problem hiding this comment.
We probably want to explicitly not check unsigned addresses for a peer for which we received signed addresses.
| resp.CloserPeers = pb.PeerInfosToPBPeers(dht.host.Network(), closerinfos) | ||
|
|
There was a problem hiding this comment.
What's the reason to send all unsigned addresses from our peerstore instead of just taking the signed addresses and extracting them?
| resp.CloserPeers = pb.PeerInfosToPBPeers(dht.host.Network(), withAddresses) | ||
|
|
|
I'm deferring to @aschmahmann for now. Please re-add me for review when done. |
aschmahmann
added
the
status/blocked
|
While I'd like to get signed peer records into the DHT there are a couple higher priority items, so I'm calling this blocked for now.
|
|
There have been a lot of changes since this was put up and this is a server side change which comes with some complexity. We'll have to revisit this when there's more time to resolve #558 |
For #558
Backward compatible for now.