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

worrisome behavior when upgrading refmt #66

Open
tobowers opened this issue Sep 26, 2019 · 2 comments
Open

worrisome behavior when upgrading refmt #66

tobowers opened this issue Sep 26, 2019 · 2 comments
Assignees

Comments

@tobowers
Copy link
Contributor

Just wanted to note something we ran into in our codebase. We recently upgraded our refmt because it has a lot of performance improvements. However, we use uint64s in our objects. When you call https://github.com/ipfs/go-ipld-cbor/blob/master/node.go#L95 (Decode) here, those uint64s get unmarshaled into overflowed int and then re-encoded back... so the bytes and CID are different.

I realize the comment says that might happen, but this isn't "canonical cbor" we're talking about... this is a change from a uint64 into an overflowed Int that then gets reencoded.

The version currently in this library works fine... just wanted to give a "heads up" when you're thinking about upgrading.

@hsanjuan
Copy link
Contributor

hsanjuan commented Mar 6, 2020

Thanks for reporting @tobowers

@warpfork is there anything actionable here? Shall we add a test?

@warpfork
Copy link
Member

warpfork commented Mar 7, 2020

I made a commit about this way back in Sep 2nd of last year, and thought I pushed a link to that into chats for early review... but I guess that didn't get anywhere.

I raised it as a proper PR now (admittedly, as I probably should have in the first place): polydawn/refmt#52

Tests would be great; PR's welcome. I think they may need to have architecture flags: unless I've misunderstood something about golang's numbers, int is in fact a 64-bit sized thing on 64-bit builds, which means the described overflow wouldn't actually be seen on 64-bit builds.

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

No branches or pull requests

3 participants