-
Notifications
You must be signed in to change notification settings - Fork 140
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
Specify storage limits, enforce when encoding, fix bignum encoding #370
Conversation
…tead of registration
…ld version The encoding of negative bignums is specified in https://tools.ietf.org/html/rfc7049#section-2.4.2: "For tag value 3, the value of the bignum is -1 - n." Negative bignums were encoded incorrectly in version < 2, as just -n. Fix this by adjusting by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. added some comments.
var encodedDictionaryValueEntriesFieldKey = cborFieldKey( | ||
encodedDictionaryValue{}, | ||
"Entries", | ||
// NOTE: NEVER change, only add/increment; ensure uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NOTE: NEVER change, only add/increment; ensure uint64 | |
// NOTE: NEVER change, only add/increment; ensure uint64 | |
// otherwise, we will have problem decoding those values which | |
// were encoded as a different type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't about type confusion, but about confusion of the fields in a container
Entries: entries, | ||
return cbor.Tag{ | ||
Number: cborTagDictionaryValue, | ||
Content: cborMap{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. You already suggested extending the comment above, but it's not gone.
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Closes #351
The CBOR library's decoder has limits on the maximum number of items in an array, the maximum number of elements in a map, and the maximum nesting level of containers. Increase these limits, as they are a bit on the low side and users have run into this limit before.
Also explicitly pin / specify the limits explicitly instead of relying on the defaults of the library (which might change).
The CBOR library's encoder does not enforce those limits, they are only used when decoding. To ensure Cadence is not encoding data that can not be decoded due to limits, validate the encoding result. This required exporting the validation function in the CBOR library: I opened an issue and a PR. This requires temporarily using a fork which has this change. I hope to switch back to the original as soon as the patch has landed upstream.
I initially planned to validate the encoded data by traversing the prepared data structure which is encoded, by keeping a depth counter. For that reason I refactored the dedicated struct types to just use
cbor.Tag
. In addition, this made the key fields actual constants (instead of relying on reflection to read struct field tags).However, when implementing this, I found myself unsure if it would implement the same limit checks as the CBOR library's decoder, and eventually decided against this approach, and simply checking the encoded data for validity (which checks the decoding limits). I think this approach is simpler, as it doesn't complicate the encoding code, and doesn't potentially get out of sync with the CBOR library's decoding limit checks.
I think it's still worth keeping the
cbor.Tag
refactor, as I find it to be an improvement.Switching to the fork updated the library, which revealed a bug in the encoding of negative bignums.
The CBOR library originally didn't support encoding CBOR positive and negative bignums (tags 2 and 3), so I had implemented this according to the RFC.
The encoding of negative bignums is specified in http://tools.ietf.org/html/rfc7049#section-2.4.2:
"For tag value 3, the value of the bignum is -1 - n.".
However, I made a mistake and didn't adjust by one. This is really only a problem if the ledger data is read by another implementation which assumes the data is RFC-compliant CBOR. The data is correctly decoded though (as proven by tests), so this is not a problem for Flow / Cadence users.
Still, we should produce correct CBOR data. I therefore bumped the Cadence encoding version from 1 to 2 and added support for decoding data that was written in the old, incorrect format.