Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Signed envelopes & routing records #73

Merged
merged 70 commits into from
Feb 10, 2020
Merged

Signed envelopes & routing records #73

merged 70 commits into from
Feb 10, 2020

Conversation

yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented Nov 8, 2019

This implements the signed envelopes and routing records described in libp2p/specs#217.

I've also updated the peerstore implementation to fit the new API & will open a PR there in a sec.

Travis correctly marks this as a breaking API change, since the new methods on the peerstore make existing implementations no longer fit the interface. I think this means I need to bump the minor version?

I still feel like a Go newbie, so don't be shy about review feedback :)

Some TODO items:

  • filter out local addresses when making a routing record for a Host
  • ensure envelope type hint has valid multicodec prefix (right now it accepts any byte string)
    • maybe? we'd have to keep this updated as new multicodecs are added

crypto/envelope.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I like it.

crypto/envelope.go Outdated Show resolved Hide resolved
crypto/envelope.go Outdated Show resolved Hide resolved
crypto/pb/crypto.proto Outdated Show resolved Hide resolved
routing/pb/routing_state.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

good work, mostly nits; the important one is that we use gogo protobuf instead of the golang stuff.

crypto/envelope.go Outdated Show resolved Hide resolved
crypto/envelope.go Outdated Show resolved Hide resolved
crypto/envelope.go Outdated Show resolved Hide resolved
crypto/pb/envelope.proto Outdated Show resolved Hide resolved
crypto/envelope.go Outdated Show resolved Hide resolved
routing/pb/routing_state.proto Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
@yusefnapora
Copy link
Contributor Author

I've been playing around with interfaces a bit, and there is a way to add this functionality without making a breaking API change.

Instead of adding the new methods to the peerstore.AddrBook interface, we can define a new CertifiedAddrBook interface that just contains the new methods (AddCertifiedAddrs, CertifiedAddrs and SignedRoutingState).

To avoid the breaking change, we can't embed the new interface into the main Peerstore interface though, so callers would have to type assert to check if the peerstore supports signed addresses, e.g.

p := myHost.Peerstore()
if cab, ok := p.(CertifiedAddrBook); ok {
  cab.AddCertifiedAddrs(signedRoutingState, ttl)
}

This is more awkward to use though, and I'm probably overthinking this...

@vyzo
Copy link
Contributor

vyzo commented Dec 3, 2019

Definitely overthinking this! I think it's ok to just extend the interface in this case.

host/helpers.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
host/helpers.go Outdated Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Some feedback. I'm gonna rejig a few things, and open a PR against your branch so we can discuss alternative options for some things.

crypto/envelope.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
routing/state.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member

raulk commented Dec 27, 2019

ensure envelope type hint has valid multicodec prefix (right now it accepts any byte string)

  • maybe? we'd have to keep this updated as new multicodecs are added

Yeah, I wouldn't hardcode an enumeration in the protobuf.

@raulk
Copy link
Member

raulk commented Dec 27, 2019

This is more awkward to use though, and I'm probably overthinking this...

Note that the using component would only do that once, and it would safe a reference to the CertifiedAddrBook.

@Stebalien Stebalien removed the request for review from bigs January 2, 2020 23:16
Copy link
Contributor Author

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback @raulk

I spent some time today pulling in your branch and am refactoring a few things that I can push tomorrow.

I also think I've found a way around the implementation problem I was having with a separate CertifiedAddrBook interface, so I can make that change here once I verify that it works in the peerstore implementation.

@yusefnapora
Copy link
Contributor Author

Hey @raulk, I pulled in your review branch and added a few commits based on your feedback.

The main changes are:

  • Defined a separate CertifiedAddrBook and a helper method GetCertifiedAddrBook(AddrBook) (CertifiedAddrBook, bool) to do the upcasting.
    • yay for API compatibility & the elusive green checkmark :)
  • Renamed the record from RoutingStateRecord to PeerRecord and moved to the peer package.
  • Removed the embedded reference to the SignedEnvelope from the PeerRecord. It's now just a representation of the envelope payload, with helpers to produce a SignedEnvelope and unmarshal a PeerRecord from a SignedEnvelope.
  • The AddCertifiedAddrs method on CertifiedAddrBook now takes a generic SignedEnvelope as an argument, and unpacks the PeerRecord payload. Likewise, CertifiedAddrBook.SignedPeerRecord(peerId) returns a SignedEnvelope.
  • Moved the Seq field from the record to the SignedEnvelope and added to the signature calculation
    • will need to update the spec

What do you think? I've updated my -peerstore and go-libp2p branches locally to reflect the changes & will push after some cleanup if you think we're on the right track here.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Initial feedback. Still going through the rest, but posting this quickly so we can discuss the proposed API changes and build up the rest of the review on top of that discussion ;-)

event/addrs.go Show resolved Hide resolved
host/helpers.go Outdated Show resolved Hide resolved
peer/pb/Makefile Outdated Show resolved Hide resolved
peer/pb/peer_record.proto Show resolved Hide resolved
peer/pb/peer_record.proto Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
peer/record.go Show resolved Hide resolved
peerstore/peerstore.go Outdated Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

@yusefnapora really, really great work here! I am especially enjoying the thoughtful structure and godocs ❤️

record/envelope.go Outdated Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
record/envelope.go Outdated Show resolved Hide resolved
record/envelope.go Outdated Show resolved Hide resolved
record/envelope.go Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

This is looking close to the finish line. Just a few comments from my first review that went unaddressed, and a proposal to further refine the Record interface and simplify the overall solution.

peer/record.go Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
record/envelope.go Outdated Show resolved Hide resolved
record/record.go Outdated Show resolved Hide resolved
peer/record.go Outdated Show resolved Hide resolved
peerstore/peerstore.go Outdated Show resolved Hide resolved
record/pb/envelope.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉🎉🎉🎉

So excited to merge this! Excellent work, @yusefnapora! 🥇

🎉🎉🎉🎉🎉🎉🎉🎉

// Seq value of zero may be ignored or rejected by other peers.
//
// PeerRecords are intended to be shared with other peers inside a signed
// routing.Envelope, and PeerRecord implements the routing.Record interface
Copy link
Contributor

Choose a reason for hiding this comment

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

record.Envelope*

PayloadType []byte

// The envelope payload.
RawPayload []byte
Copy link
Contributor

Choose a reason for hiding this comment

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

@yusefnapora Why do we use the RawPayload here instead of using a field of Record type since any payload that goes into the Envelope will always be a Record.

Removed []UpdatedAddress
}

// EvtLocalPeerRoutingStateUpdated should be emitted when a new signed PeerRecord
Copy link
Contributor

Choose a reason for hiding this comment

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

EvtLocalPeerRecordUpdated*

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P0 Critical: Tackled by core team ASAP topic/filecoin Topic filecoin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants