-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement remaining canonicity checks #31
base: master
Are you sure you want to change the base?
Conversation
gen/unmarshal.go
Outdated
u.p.printf("\nreturn") | ||
u.p.print("\n}") | ||
u.ctx.PushString(s.Fields[i].FieldName) | ||
next(u, s.Fields[i].FieldElem) | ||
u.ctx.Pop() | ||
if ize := s.Fields[i].FieldElem.IfZeroExpr(); ize != "" && isFieldOmitEmpty(s.Fields[i], s) { |
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 is not robust. If IfZeroExpr()
returns empty string it will validate unconditionally
msgp/read_bytes.go
Outdated
} | ||
i = int64(getMint8(b)) | ||
o = b[2:] | ||
if (i << 7) == 0 { |
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.
I think this should be i << 5
, because msgpack supports only 5-bit negative fixint.
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.
Actually, I think there might be two other issues here. First, it seems like you should be shifting right (i.e., i >> 5
). Second, for signed values, I think you should check for == -1
rather than == 0
.
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.
Thanks for the catch, hopefully would have caught it soon in testing. JW how much of a performance vs readability tradeoff would it be to compare against int64 constants in this case rather than doing the bitshift comparisons?
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.
I doubt there's a measurable performance difference.
Just out of curiosity, I tested go 1.21.0, and the compiler emits two instructions (CMPQ
and SETCS
) for x < 256
, and emits three instructions (SHRQ
, TESTQ
, SETEQ
) for (x >> 8) == 0
. I didn't bother benchmarking them; we should go for what's clearer.
I don't have a strong opinion about whether bit-shifts or comparisons are clearer here.
…ordering of checks for negative values
Summary
This PR implements missing canonical checks for Marshall validate defined here:
https://github.com/algorandfoundation/specs/blob/master/dev/crypto.md
Specifically it implements conditions: 2-5 listed here:
TODO
Testing
Currently, the generated code compiles but isn't yet tested for correctness hence draft status.