implementation plan for peer & content routing with signed address records

[yusefnapora]

I've been trying to figure out how to integrate the signed routing records (or maybe signed address records? names are hard) into the DHT and other peer routing systems in go-libp2p. This issue is just a place to think out loud about it and get feedback from other people before I start diving in too deep.

In my opinion so far, it looks like the simplest way to go would be to add another field (containing the signed record) to the peer.AddrInfo struct, since it's used in all the places where we'd like to add signed addresses - mostly the PeerRouting and ContentRouting interfaces, but also in the kad-dht internals.

Extending the AddrInfo struct with a new field would mean that we wouldn't need to define new SignedPeerRouting and SignedContentRouting interfaces; we could just use the existing interfaces and have the implementations include the signed address records if they were able to find them, and leave the field nil if they can't. Callers who care about signed addresses could use the new field,
and callers who don't could ignore it.

This seems like it would make updating the DHT much simpler than defining new interfaces that are essentially copies of PeerRouting and ContentRouting but with a different return type. For example, this gnarly findProvidersAsync implementation could just be tweaked to include signed records in the results, rather than having to duplicate the whole method to return signed records instead of AddrInfos.

However, it turns out that adding a field to a struct is a breaking change, since people might be embedding your struct in their own types and you can't guarantee that the new field won't collide with one of theirs. Since we're making other breaking changes to the Peerstore interface (in #73) to add signed records in the first place, maybe this isn't a huge deal if we communicate it clearly.

The bigger question is whether we really do want separate interfaces for finding signed peer addresses for design / architecture reasons. Maybe conflating the two is a bad idea, and they are different enough to warrant separate interfaces. In my mind they seem really related - when I call FindPeer, I just want to know the best way to connect to the target peer, and it seems like that should include the signed addresses if possible.

One argument for making them separate is that I might only want signed addresses, and I don't want calls to FindProvidersAsync to give me a bunch of useless unsigned results.

Another reason is that peer.AddrInfo seems to be used in a lot of places, and shoving a field in there because it's convenient for this one use case might be rude 😄

So, if we're not extending peer.AddrInfo, it seems like we want to define something like this:

// SignedPeerRouting is for finding SignedRoutingState records for remote
// peers. SignedRoutingState records contain address information that has been
// issued by the peer itself and signed to authenticate and prevent tampering.
// SignedPeerRouting may be used instead of or in addition to PeerRouting,
// depending on the security requirements of the system.
type SignedPeerRouting interface {
	// FindPeerRoutingState searches for a peer with the given ID and
	// returns a SignedRoutingState record produced and signed by that peer.
	FindPeerRoutingState(context.Context, peer.ID) (*SignedRoutingState, error)
}

// SignedContentRouting is similar to ContentRouting, in that it finds information
// about who has what content. However, addresses for content providers are returned
// in the form of SignedRoutingState records, which are issued and signed by the
// providing peer themselves.
type SignedContentRouting interface {
	// Search for peers who are able to provide a given key. A SignedRoutingState
	// record will be sent over the returned channel for each discovered content provider.
	FindSignedProvidersAsync(context.Context, cid.Cid, int) <-chan SignedRoutingState
}

Where SignedRoutingState is the address record defined in #73, which contains the original signed envelope along with an "unpacked" address list.

Anyway, that's where my head is at. What do people think?

[aschmahmann]

Thanks @yusefnapora for the context dump, this is really helpful. Below are some of my thoughts on our use of Go interfaces generally and with the DHT/ContentRouting specifically that hopefully are useful (but idk you tell me 😄):

1. Given that everything is theoretically a breaking change, like that article states, we should probably define what "breaking" means to us (go-libp2p)
2. If we're going to change/augment the ContentRouting interface I would highly recommend combining it with the Discoverer interface

   • go-libp2p-core/discovery/discovery.go

     Lines 18 to 21 in 0723374

     	type Discoverer interface {
     	// FindPeers discovers peers providing a service
     	FindPeers(ctx context.Context, ns string, opts ...Option) (<-chan peer.AddrInfo, error)
     	}

   • This interface almost exactly matches the ContentRouting interface, with Discovery being the more general one. I even made some wrappers for converting between ContentRouting and Discovery at https://github.com/libp2p/go-libp2p-discovery/blob/be2ad5297c88bc8db2f98334dfd4f0ae392e47a0/routing.go
   • If the Discoverer interface is deemed insufficient we should toss it out and replace it with whatever works here since there are very few Discoverer implementations we'd have to update
3. I'm pretty sure the go-libp2p-kad-dht interfaces could use some quality rewriting anyway (i.e. why does it have functions with a dependency on CIDs when it's just a distributed KV store), so I wouldn't feel bad about breaking the interfaces. We can always put together a backwards compatibility wrapper if anyone really wants it.
4. Trying to write Java in Go
   • I'm sure there are a number of us that got our Java experience long before our Go experience. Combining this with our desire to make go-libp2p extremely pluggable leads us to spending a lot of time designing interfaces that are globally defined (e.g. this repo)
   • Go has a very opinionated view of how it thinks you should write code and that includes not using interfaces this way. On the Go Tour introduction it says:

   Interfaces are implemented implicitly

   A type implements an interface by implementing its methods. There is no explicit declaration of intent, no "implements" keyword.

   Implicit interfaces decouple the definition of an interface from its implementation, which could then appear in any package without prearrangement.

   • I suspect that if we actually took their advice and moved toward defining interfaces where they are used instead of globally that we would be able to "break" things with fewer repercussions.
5. If we're going to continue to use global interfaces
   • Perhaps we should try and make them more extensible/generic. For example, we could return some interface representing an signed peer address instead of the SignedRoutingState struct. Not convinced here, but just a thought 🤷‍♂
   • It might be helpful to use the option pattern in the interfaces so we break things less in the future
6. If modifying peer.AddrInfo is much easier than any of the more interface-oriented approaches then I don't see why we shouldn't do that, but if we have time to explore why everything is harder it might help us in planning how we should redesign things when we can start paying down some of our technical debt 😜

[yusefnapora]

These are great points, thanks @aschmahmann!

As someone that's coming more or less straight from Java to Go, the interface thing has definitely been tripping me up.

I think that defining and using an interface instead of the SignedRoutingState struct might also help me with testing. For example, in the identify PR I'd like to test how it behaves if signedRecord.Marshal() returns an error. In java I'd use something like Mockito, but the Go way seems to be to define an interface instead, and have the test code provide an implementation that returns an error.

I also found this blog post about go interfaces that's pretty good. In the Upgrading Interfaces section they recommend defining a new interface with methods you want to add instead of adding methods to an existing one. Then the caller has to do a type assertion to see if the new interface is supported. I could potentially make it so that the Peerstore changes in #73 are non-breaking changes by adopting that pattern.

I'm intrigued by the idea of moving go-libp2p away from "global" interfaces... one of the things that's been tricky for me as I'm learning how to navigate go-libp2p is that it's not obvious where the implementations for a given interface live and where all the consumers are. That's partly because things are spread out into a lot of repos, but moving towards defining interfaces at the point of use would probably help. Sounds like a pretty big refactor, but definitely worth thinking about 🤔

[yusefnapora]

just had a sync call with @raulk, where we talked over this issue and realized that we may not need interface changes here. Instead, we can isolate the signed vs unsigned record exchange within the DHT implementation, and callers will still use the existing PeerRouting and ContentRouting interfaces.

If a consumer of those interfaces really cares about only having signed addresses, they should be able to enable a "strict mode" flag, which will prevent the DHT from returning unsigned addrs.

I'm going to close this issue, since it looks like we won't need to change the -core interfaces or peer.AddrInfo. I'll open a new planning issue soon in the -kad-dht repo to track the actual implementation changes needed.

Thanks again for the input @aschmahmann :)