Ignore records with IDENTITY CID in IndexSorted

[masih]

Skip IDENTITY CIDs in IndexSorted, as required by CARv2
specification.

Re-generate sample-wrapped-v2.car testdata, since its previous index
contained CIDs with IDENTITY multihash.

Gracefully handle CIDs with IDENTITY multihash code in the
blockstore API. Since Get, GetSize and Has interfaces rely on
the index, decode given keys directly to satisfy calls for such CIDs.

See:

• https://ipld.io/specs/transport/car/carv2/#index-format

Fixes #215

[willscott]

I'm not totally opposed to this as a defensive mechanism.

in the past we've used the pattern of wrapping the blockstore in an idstore https://github.com/ipfs/go-ipfs-blockstore/blob/master/idstore.go when there is the potential for interaction with identity-hashed blocks.
I wonder if we're okay at this layer just panic-ing / being unhappy if they show up and expecting that wrapping layer to take care of them?

[masih]

I wonder if we're okay at this layer just panic-ing / being unhappy if they show up and expecting that wrapping layer to take care of them?

Yup that makes sense; will do 🍻

[rvagg]

I'm a soft +1 on all our layers behaving like this, but it's a bit tricky since if we don't do it completely then we set up a false expectation for users that they don't need to wrap or be careful with the layers they're interacting with. I don't think there's harm in doing it here, it's quite nice, especially if you just want to use a CARv2 as a raw blockstore, but the user expectation problem will be the thing that gets us, if anything.

[masih]

I'm a soft +1 on all our layers behaving like this, but it's a bit tricky since if we don't do it completely then we set up a false expectation for users that they don't need to wrap or be careful with the layers they're interacting with. I don't think there's harm in doing it here, it's quite nice, especially if you just want to use a CARv2 as a raw blockstore, but the user expectation problem will be the thing that gets us, if anything.

Having implemented an explicit error when given IDENTITY CIDs in blockstore here, I am leaning towards being more forgiving in the CARv2 blockstore APIs and "doing the right thing" when given such CIDs.

This is because, in order to explicitly error out the blockstore needs to decode the CID hash, and right there we have all the information to do what IdStore does. Sure; it would be duplicating the logic of IdStore in CARv2 library, but not doing it and forcing IdStore wrapper means:

1. potential code changes for dependant work streams if CAR files interacted with via the blockstore API often contain IDENTITY CIDs (cc @aarshkshah1992)
2. the CIDs will get decoded twice: once by the wrapping IdStore and once by the blockstore API here to check that they are indeed not IDENTITY CIDs.

@willscott @rvagg @aarshkshah1992 What are your thoughts?

[rvagg]

I think my weakly held position on this is that CARv2 is somewhat likely to be used as a stand-alone blockstore, for IPLD applications that exist outside of IPFS and want a simple immutable block store, so if the information is at hand already (i.e. it's cheap), I think it should transparently handle identity CIDs.

[masih]

After much discussion, this PR now gracefully handles IDENTITY CIDs, doing what IdStore wrapper does without the need for wrapping.

Thanks @willscott @rvagg

[mvdan]

Should this behavior be mentioned in the blockstore package godoc? It seems pretty relevant at a high level.