Implement final FilMultihash/FilCodec

[ribasushi]

( I do not have fil-project perms, hence PR from fork )

This rewrites the module rather radically, implementing the decision agreed by @rvagg, @jbenet, @hannahhoward and @Stebalien in multiformats/multicodec#161 (comment)

Focus on the function name changes and signature changes within the module: the tests are simply adjusted to match.

[rvagg]

Duplication is a thing I'm mostly interested and maybe we can reduce that a bit? Starting with the constants.

Sealed and unsealed codecs are defined here:

https://github.com/ipfs/go-cid/blob/85cd30874e614c61d6dd82d991878d84053fd579/cid.go#L78-L79

The two hash functions are defined here (I think only the sha256 one is used here at the moment):

https://github.com/multiformats/go-multihash/blob/6f1ea18f1da5f7735ea31b5e2011da61c409e37f/multihash.go#L71-L73

A lot of the code in this repo exists because go-cid and go-multihash didn't have values ready, so a further duplication could happen by pushing all of the multihash and cid creation back down to go-cid and go-multihash now that they have them. But I think that's lower priority since the main urgent thing is simply getting the right constants into play! Up to you how far down that rabbit hole you go I think, it can be deferred. For now I'd like to see the constants deduplicated and not redefined if that's not too hard. The "reserved" entries are probably in the way a bit, but I don't think they're used at all and I'm not really sure why they're there. The 0xf100 space is essentially "reserved" now for filecoin.

[ribasushi]

Implements consensus from multiformats/multicodec#161 (comment)
This is now open for final review by the original stakeholders: @rvagg, @jbenet, @hannahhoward @Stebalien and @mikeal. ( Can't assign reviewers due to lack of perms )

This will need to move relatively fast to make it into the feature freeze. I can help with updating lotus and friends, provided the changes are 100% correct / approved.

[rvagg]

iirc CommR uses sealed+Poseidon. CommD and CommP use unsealed + the special sha256. @porcuquine can you confirm please before we commit to these identifiers?

[porcuquine]

That's right. CommR is the output of a binary Poseidon hash and has no relationship to SHA256. Does that suffice to answer your question?

[rvagg]

@porcuquine yep, thanks!

@ribasushi so, better change this to multihash. POSEIDON_BLS12_381_A1_FC1 and the comment above it too

[ribasushi]

Offfff thanks for catching this folks! I misread an earlier comment to arrive at this 🤦
All fixed!

Also rewrote the validator from scratch to disallow incorrect codec/hash combinations. Namely:

go-fil-commcid/commcid.go

Lines 111 to 133 in 226ec33

	// ValidateFilecoinCidSegments returns an error if the provided CID parts
	// conflict with each other.
	func ValidateFilecoinCidSegments(mc FilMultiCodec, mh FilMultiHash, commitment []byte) error {
	switch mc {
	case cid.FilCommitmentUnsealed:
	if mh != multihash.SHA2_256_TRUNC254_PADDED {
	return ErrIncorrectHash
	}
	case cid.FilCommitmentSealed:
	if mh != multihash.POSEIDON_BLS12_381_A1_FC1 {
	return ErrIncorrectHash
	}
	default: // neigher of the codecs above: we are not in Fil teritory
	return ErrIncorrectCodec
	}
	if len(commitment) != 32 {
	return fmt.Errorf("commitments must be 32 bytes long")
	}
	return nil
	}

[rvagg]

It's certainly simplified after this! maybe this is a style issue - but would it be easier if this package re-exported those go-multihash and go-cid constants so callers don't need to reach into those packages to get them? The current usage of this package would be using constants from here but will not need to include this package, go-cid and go-multihash to get everything together. Maybe that's perfectly fine for Go and nobody will blink an eye at it, it just seems heavy-handed.

[rvagg]

Maybe DataCommitmentV1ToCID() and ReplicaCommitmentV1ToCID() are the main interfaces here, in which case my comment about constants may not be that relevant. The other two public methods exposed here require you supply the codec and multihash constants to use, so you need to get them from somewhere. Prior to this PR you'd just pull them in from this package. By removing them now and deferring to go-cid and go-multihash, if you call those methods then you'd also need to grab the relevant multihash and multicodecs from go-cid and go-multihash.

[ribasushi]

Maybe DataCommitmentV1ToCID() and ReplicaCommitmentV1ToCID() are the main interfaces here, in which case my comment about constants may not be that relevant.

@rvagg right, this is the idea, cemented further by the validator. I am inclined to leaving the "inconvenience" in place at present, so that folks do not reach out for these functions without thinking. Ideally none of the helpers should be exported, but I do not want to change the preexisting package's style too much.

[rvagg]

Goodo, thanks for doing this!

[ribasushi]

Last weeks update on this was:

we can consume this into our staging branch as soon as it’s merged (all consensus-critical stuff goes there).

@arajasek @whyrusleeping what needs to happen to make this... happen? :)

[rvagg]

@hannahhoward I think you're the closest to being an "owner" here, would you mind taking a look and merging?

[whyrusleeping]

This LGTM. A bit easier to follow and the checks feel safer too.

[mikeal]

@whyrusleeping does there need to be some kind of plan to roll this out to lotus? it’s sort of a breaking change, but you’d know better than me what scale of breakage it could potentially cause.