Coverage increased (+0.07%) to 57.215% when pulling 3388947 on Stebalien:feat/ipld-format-decoder into a8c7e8c on ipfs:master.

Coverage decreased (-0.05%) to 63.291% when pulling 4b04798 on Stebalien:feat/ipld-format-decoder into c13297e on ipfs:master.

Coverage decreased (-0.05%) to 63.291% when pulling e0e7215 on Stebalien:feat/ipld-format-decoder into c13297e on ipfs:master.

Coverage decreased (-0.05%) to 63.291% when pulling 9fd3ab9 on Stebalien:feat/ipld-format-decoder into c13297e on ipfs:master.

Ah, this breaks our tests that assert cbor values get normalized to their canonical values when you decode a block from them.

@Stebalien what do you think?

Here is a test case that reproduces this https://gist.github.com/magik6k/5b1b30087914ebaf0ccc7d0cb91c2d67

Based on https://github.com/ipfs/go-ipfs/blob/master/test/sharness/t0053-dag.sh#L115

IMO, it's more important to avoid changing the CID than to normalize. I'd remove the tests (but I'm probably missing prior discussion on this topic).

We can either normalize, or throw an error. Otherwise we can have two objects that are equivalent by value comparison, but have different 'hashes', which is an issue.

My primary concern is that block and DecodeBlock(block).(Block) should be interchangeable. Otherwise, I can ask a DAGService for a node with one CID and get back a node with a different CID (because the encoding was normalized under the hood).

However, looking back at this commit, I believe the problem is that Decode takes data that doesn't have a CID attached to it, produces a Block, and then turns that into a Node. Given that the data doesn't already have a CID, there's no problem in normalizing it first.

However, once something has a CID, I think we either need to keep it as-is or throw an error. I'm all for strict validation that fails early but I'm worried that if we always re-encode before generating the CID, we'll never be able to upgrade our CBOR library (also, efficiency).

As an aside, IMO, all comparison of blocks/nodes should compare CIDs and only CIDs.

However, once something has a CID, I think we either need to keep it as-is or throw an error.

Agreed with this, i think any place where we take in raw data (not a block) should do normalization validation.

Agreed with this, i think any place where we take in raw data (not a block) should do normalization validation.

Although remember, we may be taking in raw data through a user interface where the user will want us to normalize, not validate (e.g., JSON). Your comment on IRC about having a strict mode versus a non-strict mode is on-point.