LT & GT opcode spec #53
Conversation
|
cc @ChihChengLiang @miha-stopar This PR is ready for review. |
|
|
||
| The `LT` and `GT` opcode compares the top two values on the stack, and push the | ||
| result (0 or 1) back to the stack. | ||
|
|
There was a problem hiding this comment.
I'd like to see a short description on
- How do we use
c8s === b8s - a8sto checkb8s > a8s - How do we use
swapto reuse the same gadget for GT and LT.
So that the reviewer or the auditor can have a rough idea before diving into the Python code.
src/zkevm_specs/opcode/lt_gt.py
Outdated
| swap: bool, | ||
| carry: bool, | ||
| ): | ||
| assert not swap |
There was a problem hiding this comment.
I would suggest renaming swap to something like is_gt so that it doesn't collide with the SWAPX opcode
There was a problem hiding this comment.
LGTM! However, would it make sense to include the EQ opcode as well if privacy-scaling-explorations/zkevm-circuits#120 is merged?
src/zkevm_specs/opcode/lt_gt.py
Outdated
| """ | ||
| Check if c8s equals to b8s - a8s. | ||
| When result is 1, the last carry of the addition of a8s and c8s should be 0, | ||
| and c8s should not equal to 0. |
There was a problem hiding this comment.
nitpick: "not be equal to 0."
There was a problem hiding this comment.
Fixed. Thank you.
src/zkevm_specs/opcode/lt_gt.py
Outdated
| ): | ||
| """ | ||
| Check if c8s equals to b8s - a8s. | ||
| When result is 1, the last carry of the addition of a8s and c8s should be 0, |
There was a problem hiding this comment.
Is this properly formulated? Don't we have only one carry?
There was a problem hiding this comment.
Update the doc to be more clear.
|
@ChihChengLiang I moved the test basic add and sub test from |
There was a problem hiding this comment.
LGTM
Waiting for a final look from @miha-stopar before merging
|
It seems cool to me as it reflects the current implementation. However, the implementation is now being changed here. There are some differences in the concept of both implementations and there is also EQ added in the new one. Would it be OK to reflect the new implementation before merging it? |
There was a problem hiding this comment.
Talked with @ChihChengLiang - it makes sense to merge it now and then reflect the implementation changes in a separate PR. Thanks for good work!
* add lt & gt spec Co-authored-by: gaswhat <hs@scroll.tech>
Add the python code and doc for LT and GT opcode. Also refactor the test code.