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

Correct Serialize implementation of BinaryString and upgrade database #276

Merged
merged 5 commits into from
May 27, 2023

Conversation

Dekkonot
Copy link
Member

@Dekkonot Dekkonot commented May 27, 2023

After doing some more detailed investigation into the issue I found in PR #275, I've identified that the issue was with our implementation of Serializate in BinaryString.

We're currently using serialize_bytes to serialize the internal buffer of it, and using <Vec<u8>>::deserialize to deserialize it. This was fine for a while but after upgrading to rmp-serde version 1.1.1, it has broken. After giving it some thought I've realized that our implementation is outright wrong because we're assuming Vec<u8> and &[u8] serialize the same. A lot of the time this is true, but it won't always be, so we should stop making that assumption.

This PR does just that and instead makes it so we just use Vec<u8>'s serialize and deserialize implementations. It's easy to implement a Visitor and continue to use serialize_bytes but that ends up being a lot of code that we don't need to have. The cost to the database does not exist (I checked) so there's no downside.

I've also included a new and upgraded database so that nobody else has to go through the process of updating. This comes with a few snapshot changes but nothing surprising (I am horrified to learn we weren't tracking things like ChildData though).

Copy link
Member

@kennethloeffler kennethloeffler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, glad there was a solution

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 this pull request may close these issues.

2 participants