Provider records use multihashes instead of CIDs

[aschmahmann]

This makes it so that provider records use multihashes instead of CIDs, solving the most important part of libp2p/go-libp2p#755. This is effectively a wire protocol fix and does not change any of the external facing DHT APIs.

Note: This change will have a network breaking effect for users advertising provider records for CIDv1 CIDs, but should not effect anyone using CIDv0 CIDs.

[aschmahmann]

@raulk we'd like to switch from provider records using CIDs to Multihashes, to support finding peers with CIDv0 and CIDv1 of the same multihash. While this will break behavior for current users of CIDv1, since peers with an updated DHT will look for and advertise to the multihash instead of the CID, (CIDv0 users are protected since CIDv0 = Multihash SHA256), it will help the network longer term.

On the "server" end we previously checked that the key that we were adding providers for was a CID (the code change above). Initially I started just switching all the things associated with Providers to use Multihashes instead of CIDs. However, this does mean that a new server will reject advertisements from old users of CIDv1.

I was talking about this a bit with @Stebalien and we were wondering how would you feel about switching the provider records internals (e.g. ProviderManager) to work on []byte instead of on Multihashes or CIDs? We could then have the server check if the key is less than 80 bytes (640 bits) for DoS purposes, but otherwise not care about the content type.

[aschmahmann]

I changed this to use []byte instead of Multihashes and to limit the []byte size to 80 bytes. Can revert back if we want.

[aschmahmann]

Before we do a release with these changes I'd like to run a Testground test that uses both new and old versions to test compatibility (without using go mod major versions >1 it's very difficult to test this with unit tests)

[vyzo]

Shouldn't we just make it arbitrary strings instead of multihashes though?

[Stebalien]

Shouldn't we just make it arbitrary strings instead of multihashes though?

#422 (comment)

The remaining part is to change the local DHT (go) API but that issue is less pressing than changing the network.

[aschmahmann]

@vyzo we actually do allow arbitrary strings instead of multihashes internally right now (as long as they're less than 80 bytes). However, in order to access that functionality we'd need to fix the external APIs. So one at a time 😄

[aschmahmann]

It looks like our logging for the DHT assumes keys are of the form /namespace/CID (whether for DHT Puts/Gets or Provides/FindPeers).

go-libp2p-kad-dht/lookup.go

Line 36 in dd3d8fb

c, err := cid.Cast([]byte(cstr))

go-libp2p-kad-dht/lookup_test.go

Lines 9 to 10 in dd3d8fb

	func TestLoggableKey(t *testing.T) {
	c, err := cid.Decode("QmfUvYQhL2GinafMbPDYz7VFoZv4iiuLuR33aRsPurXGag")

Do we want to impose any key restrictions at all for what we log? What about assuming the for is /namespace/X where X is a CID, Multihash or an arbitrary string/byte array? This would allow us to print in all cases, but not have to deal with non-base encoded CID/Multihashes.

[Stebalien]

I've gotten a signoff from @raulk and the IPFS team so we should be good to go.

@aschmahmann WRT logging, we just need some reasonable key. We should just do the best we can but otherwise just base encode it.

[Stebalien]

(or we can just base32 encode the entire key literally and call it a day)

[Stebalien]

(lgtm modulo logging and merge conflict)

[aschmahmann]

This is uppercase, but when we encode CIDs using b32 we use lowercase. Should I lowercase this?

[aschmahmann]

Did some manual testing to confirm CIDv0 provider record compatibility (and CIDv1 record incompatibility) with current master. Would be nice to make this automated before a go-ipfs release, but this seems fine for now.

@Stebalien LMK if there's anything else blocking the merge (just noticed that dependencies were updated, will rebase on master and try again)

[lidel]

cc https://github.com/ipfs/ipfs/issues/337