filecoin-project / go-amt-ipld

Implementation of an array mapped trie using go and ipld
Other
9 stars 15 forks source link

avoid altering amt when empty #22

Closed acruikshank closed 4 years ago

acruikshank commented 4 years ago

closes #18

Motivation

Actions like calling ForEach or adding and removing elements put an empty AMT into a state that causes it to be serialized differently even though it is still the empty array. This is because these actions cause expVals to be expanded in the node from a nil value to a slice with nil values. While expVals isn't itself serialized, The fact that it is non-nil causes the node's Bmap value to be initialized from a nil value to []byte{0} in Flush, and that does serialize differently.

Proposed Changes

  1. Add a test to demonstrate the problem.
  2. Initialize Node.Bmap lazily in Flush.

This is probably the simplest solution that fixes the problem. It leaves open a bigger question about whether empty nodes should be structurally different when serialized from nodes containing one or more elements. I find it a little surprising, but not really wrong.

whyrusleeping commented 4 years ago

I think I agree with @Stebalien , this value should always be set and never should be serialized as nil.

Stebalien commented 4 years ago

Fix for byte arrays: https://github.com/whyrusleeping/cbor-gen/pull/29

Stebalien commented 4 years ago

Fix merged.

Stebalien commented 4 years ago

Fix pushed. Could you take a look at this @whyrusleeping and @acruikshank?