fix(uints): constrain valueOf

[bernard-wagner]

Description

As stated in the TODO, uints8.ValueOf is not constrained. This commit constrains the output of ValueOf.

I am jumping in on the deep end with circuit development, so would appreciate as much feedback as possible. Especially would appreciate guidance on testing constraints for intermediate values, the overflow was the only technique I could think of that works in a unit test.

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

Added a unit test that if added to the current master branch will fail. The third test case should return an error as the input exceeds the output size.

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I did not modify files generated from templates
• golangci-lint does not output errors locally
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[ivokub]

Thanks for the PR. I'm trying to have a look at it this week to give feedback.

As of why we yet haven't added the constraining of the values always, is that in the context of some operations (XOR, AND), the uints.U8.Value are used as keys in looking up from the table and as such if the would not be in range, then the lookup would fail. So there is implicit constraining of the values to be 8-bit. But for example when we're doing some other operations which is not based on lookups, then it could lead to underconstrained circuits indeed.

I haven't looked at your implementation yet, but my general idea for building it better was to have an additional value or DB which says if U8 value is used in lookup table operations and is implicitly constrained.

[ivokub]

Alright - I finally got to reviewing it. Your initial PR was spot on, but it revealed another issue in Add method where we didn't check that the carry we omitted was correct. And fixing that made me realize that I could clean up some parts.

I pushed the changes directly to your branch, I hope you wont mind. Maybe you can have a look before I merge?

Otherwise, thanks for the contribution! Great work!