implementation plan for peer & content routing with signed address records about go-libp2p-core HOT 3 CLOSED

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 😜

from go-libp2p-core.

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 🤔

from go-libp2p-core.

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 :)

from go-libp2p-core.