Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

State exports can be deserialized in multiple ways #1547

Closed
hsanjuan opened this issue Jan 18, 2022 · 1 comment · Fixed by #1664
Closed

State exports can be deserialized in multiple ways #1547

hsanjuan opened this issue Jan 18, 2022 · 1 comment · Fixed by #1664
Labels
kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Milestone
Release v1.0.1

Comments

@hsanjuan
Copy link
Collaborator

A state export file contain pins. Each pin object may potentially have a user-defined metadata map.

These maps can potentially be serialized in any order. When the pin is stored as part of a delta, the binary representation of it may differ randomnly. This causes the created CRDT deltas to have different CIDs.

This means that multiple peers do not obtain a consistent state when the same state file is imported.

The serialization of a Pin to protobuf should be reproducible and only occur in one way for equivalent pin objects.
@hsanjuan hsanjuan added the kind/bug A bug in existing code (including security flaws) label Jan 18, 2022
@hsanjuan hsanjuan added the P1 High: Likely tackled by core team if no one steps up label Mar 1, 2022
@ipfs-cluster ipfs-cluster deleted a comment from welcome bot Mar 1, 2022
@hsanjuan
Copy link
Collaborator Author

It seems it was a really bad choice to put a map in the Pin protobuf, as per https://developers.google.com/protocol-buffers/docs/proto3#maps there is no guarantees on wire order or anything.

Thinking on approaches around this, which might involve deprecation of the protobuf field and using an array instead.

hsanjuan added a commit that referenced this issue May 5, 2022
@hsanjuan
Fix #1547: Store pin metadata in a sorted array
1f1f05a 
This deprecates the Metadata protobuf map and starts serializing metadata as an
array that is always sorted by the metadata key.

This should resolve the issue that an state export file can be imported
resulting in two different CRDT dags because the binary representation of the
pin can arbitrarily change depending on how the keys in the map are listed,
and thus the CID of a delta is impacted.

So with this, a state export should result in exactly the same DAG regardless
of where it is imported.
hsanjuan added a commit that referenced this issue May 5, 2022
@hsanjuan
Fix #1547: Store pin metadata in a sorted array
5586133 
This deprecates the Metadata protobuf map and starts serializing metadata as an
array that is always sorted by the metadata key.

This should resolve the issue that an state export file can be imported
resulting in two different CRDT dags because the binary representation of the
pin can arbitrarily change depending on how the keys in the map are listed,
and thus the CID of a delta is impacted.

So with this, a state export should result in exactly the same DAG regardless
of where it is imported.
@hsanjuan hsanjuan linked a pull request May 5, 2022 that will close this issue
Fix #1547: Store pin metadata in a sorted array #1664
Merged
hsanjuan added a commit that referenced this issue May 5, 2022
@hsanjuan
Merge pull request #1664 from ipfs/fix/1547-metadata-serialization
72a7c37 
Fix #1547: Store pin metadata in a sorted array
hsanjuan added a commit that referenced this issue Jun 16, 2022
@hsanjuan
Fix #1547: Store pin metadata in a sorted array
7a05eee 
This deprecates the Metadata protobuf map and starts serializing metadata as an
array that is always sorted by the metadata key.

This should resolve the issue that an state export file can be imported
resulting in two different CRDT dags because the binary representation of the
pin can arbitrarily change depending on how the keys in the map are listed,
and thus the CID of a delta is impacted.

So with this, a state export should result in exactly the same DAG regardless
of where it is imported.
@hsanjuan hsanjuan added this to the Release v1.0.1 milestone Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Milestone
Release v1.0.1
Development

Successfully merging a pull request may close this issue.

Fix #1547: Store pin metadata in a sorted array ipfs-cluster/ipfs-cluster
1 participant
@hsanjuan