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.

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

consensus::State (de)serialization regressions #679

Closed
tarcieri opened this issue Nov 17, 2020 · 2 comments · Fixed by #680
Closed

consensus::State (de)serialization regressions #679

tarcieri opened this issue Nov 17, 2020 · 2 comments · Fixed by #680
Assignees

Comments

@tarcieri
Copy link
Contributor

tarcieri commented Nov 17, 2020

There are a couple problems with the serialization (or rather, deserialization) implemented in #676 which prevents existing consensus state JSON files from being read. (Note: we can support a new serialization, it's just the deserialization that needs to be backwards compatible)

null block_id field

tendermint_proto::serializers::optional does not appear to handle the field being explicitly null, e.g.:

{"height":"3820001","round":"0","step":2,"block_id":null}

This results in the following error:

invalid type: null, expected struct BlockId at line 1 column 50

total field of parts::Header

This was previously serialized as a string. It now appears to be serialized as an integer. This causes the following error when deserializing an existing file:

 invalid type: string "1", expected u32 at line 1 column 146
@greg-szabo
Copy link
Member

The total field was changed in Tendermint Go too. I can write a compatibility layer to be able to deserialize from string OR int, but we should keep in mind that serialization will follow the new normal.

This means that KMS can upgrade from an older version but cannot downgrade after a new version was executed.

@tarcieri
Copy link
Contributor Author

@greg-szabo that sounds fine. worst case, people can hand-edit the files or blow them away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants