Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Spec for opcode SHL#225

Merged
lispc merged 31 commits into
privacy-ethereum:masterfrom
scroll-tech:feat/opcode-shl
Aug 19, 2022
Merged

Spec for opcode SHL#225
lispc merged 31 commits into
privacy-ethereum:masterfrom
scroll-tech:feat/opcode-shl

Conversation

@silathdiir
Copy link
Copy Markdown
Contributor

@silathdiir silathdiir commented Jun 19, 2022 •
edited
Loading

Circuit: privacy-ethereum/zkevm-circuits#578

Summary

  1. Fix to use MulAddWordsGadget to implement both SHL and SHR circuits.

  2. Merge new SHL spec into SHR.

silathdiir added 20 commits May 19, 2022 09:06
@silathdiir
Add test case for SHL.
9b1d145
@silathdiir
Implement opcode SHL.
35f66e0
@silathdiir
Fix to validate a64s_hi[idx] < p_hi, and add Markdown doc for SHL.
1939133
@silathdiir
Move some code from SHR to make it work with master branch.
897ad08
@silathdiir
Merge remote-tracking branch 'origin/master' into feat/opcode-shl
af1c403
@silathdiir
Replace key code_source with code_hash in test code.
5f01718
@silathdiir
Merge remote-tracking branch 'origin/master' into feat/opcode-shl
929610d
@silathdiir
Fix to use mul_add_words for SHL.
8633f59
@silathdiir
Merge previous SHR into SHL.
ad0ed96
@silathdiir
Merge both SHL and SHR into mul_div_mod_shl_shr.
1469896
@silathdiir
Delete separate Markdown doc of SHL and SHR.
b45da81
@silathdiir
Fix constraints.
3ad5b22
@silathdiir
Update Markdown doc for MUL, DIV, MOD, SHL and SHR.
9b95227
@silathdiir
Merge remote-tracking branch 'origin/master' into feat/opcode-shl
450db8c
@silathdiir
Fix the wrong argument sequence of SHL and SHR.
4598e5f
@silathdiir
Add function is_op_mul, is_op_div, is_op_mod, is_op_shl and
7a7f5b9 
`is_op_shr`.
@silathdiir
Delete ExecutionState::SHL_SHR and update Markdown doc according to
76f92ab 
code review.
@silathdiir
Update spec code according to circuit, and add Pow2 lookup for shift
358f20e 
value of opcode SHL and SHR.
@silathdiir
Fix Pow2 lookup table and update to use shf0 for lookup.
c57b4d0
@silathdiir
Merge remote-tracking branch 'upstream/master' into feat/opcode-shl
e39f590
This was referenced Jun 19, 2022
Merged
Closed
@adria0
Copy link
Copy Markdown
Contributor

adria0 commented Jun 30, 2022

IMO merging SHL, SHR with the previous MUL_DIV_MOD adds sobre extra complexity in reviews that we can avoid for the moment. Some kind of low-constraint optimizations could be done at the end. I'm more comfortable with ad0ed96

Anyway, just an opinion, more opinions are welcomed. :)

@silathdiir
Copy link
Copy Markdown
Contributor Author

silathdiir commented Jul 21, 2022

As @adria0 commented, will fix to merge SHL with SHR. Thanks.

@silathdiir silathdiir marked this pull request as draft July 21, 2022 02:18
@silathdiir silathdiir marked this pull request as ready for review August 1, 2022 01:17
@silathdiir
Copy link
Copy Markdown
Contributor Author

silathdiir commented Aug 2, 2022

Hi @adria0 , @lispc and @icemelon , I updated both spec and circuit PRs for only merging opcode SHL and SHR to use MulAddWordsGadget.
Please help review when you have time. Thanks.

adria0
adria0 reviewed Aug 9, 2022
View reviewed changes
Comment thread specs/opcode/1bSHL_1cSHR.md Outdated
adria0
adria0 reviewed Aug 9, 2022
View reviewed changes
Comment thread specs/opcode/1bSHL_1cSHR.md Outdated
adria0
adria0 reviewed Aug 9, 2022
View reviewed changes
Comment thread src/zkevm_specs/evm/execution/shl_shr.py
silathdiir and others added 3 commits August 10, 2022 10:45
@silathdiir @adria0
Update specs/opcode/1bSHL_1cSHR.md
90c9344 
Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>
@silathdiir @adria0
Update specs/opcode/1bSHL_1cSHR.md
413cb21 
Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>
@silathdiir
Fix to always constrain push to zero if divisor == 0 (shift > 255).
d53de49
@silathdiir silathdiir requested a review from adria0 August 10, 2022 02:57
adria0
adria0 approved these changes Aug 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@adria0 adria0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, an extra review will be nice, this math stuff is always tricky :)

@lispc
Copy link
Copy Markdown
Collaborator

lispc commented Aug 19, 2022

lgtm

@lispc lispc merged commit dd004ba into privacy-ethereum:master Aug 19, 2022
@lispc lispc deleted the feat/opcode-shl branch August 19, 2022 04:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@lispc lispc lispc approved these changes

@adria0 adria0 adria0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@silathdiir @adria0 @lispc