search

ipld / specs

Content-addressed, authenticated, immutable data structures
Other
592 stars 108 forks source link

block-layer/codecs: dag-pb can't enforce PBNode ordering #360

Closed mvdan closed 3 years ago

mvdan commented 3 years ago

(see commit message)

willscott commented 3 years ago

i'm not sure i agree with the conclusion of not having a canonical form? just because we're willing to decode non-canonical stored representations doesn't mean there isn't a canonical form

mvdan commented 3 years ago

I guess that depends on whether you define encoders that use a different order as spec-compliant or not. This is why I said the encoding "should" use a specific order, instead of "must".

If we don't get a review from Rod by the middle of next week, I can always remove that sentence to unblock a merge.

rvagg commented 3 years ago

Yeah, I'm with @willscott on this one.

I think it should be "must be encoded ...", but "The decoder should accept either order", and also that there is a canonical form for given data. It's just that there exists some historical data which a decoder may encounter which is not in that canonical form. What we want here is for any new/current codec implementation to produce that canonical form, so round-trips of this historical data will end up as different bytes & with a different CID. We're not committing to preserving non-canonical forms in round-trips either (which is one way you could read this current wording I think).

mvdan commented 3 years ago

Fair enough, that's a good point. Your wording does imply that someone using a "field number order" protobuf encoder is actively breaking the spec, but I think we're fine with that for the sake of strongly encouraging canonical forms.

mvdan commented 3 years ago

Updated :) I'll prepare a branch for go-codec-dagpb on top of the performance PR, since otherwise I think I'll run into conflicts with myself.

mvdan commented 3 years ago

I've applied all three of Eric's nits. It seems like we all agree, so I'm moving ahead. Thanks!