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

feat: add sstore circuit#383

Merged
CPerezz merged 1 commit into
privacy-ethereum:mainfrom
scroll-tech:sstore
Mar 15, 2022
Merged

feat: add sstore circuit#383
CPerezz merged 1 commit into
privacy-ethereum:mainfrom
scroll-tech:sstore

Conversation

@0xmountaintop
Copy link
Copy Markdown
Collaborator

@0xmountaintop 0xmountaintop commented Mar 10, 2022

No description provided.

@github-actions github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue labels Mar 10, 2022
@0xmountaintop 0xmountaintop mentioned this pull request Mar 10, 2022
Closed
ed255
ed255 reviewed Mar 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Thanks for working on this.

Nevertheless I see how tricky it is to calculate gas cost and refund values for this opcode. I would be more confident reviewing this if the SSTORE was implemented in the bus-mapping (like I suggest in a comment). So that the external tracer with the circuit input builder can be used, and then we can test all the different scenarios with the gas cost and refund value set by geth to test the correctness of the implementation.

Let me know your plans on implementing the SSTORE in the bus-mapping :)

Comment thread bus-mapping/src/operation.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/sstore.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/sstore.rs Outdated
@0xmountaintop 0xmountaintop force-pushed the sstore branch 3 times, most recently from 9cf2af2 to fa427a6 Compare March 12, 2022 02:21
@0xmountaintop 0xmountaintop mentioned this pull request Mar 14, 2022
Closed
@0xmountaintop
Copy link
Copy Markdown
Collaborator Author

0xmountaintop commented Mar 14, 2022

Hi @ed255 !

  1. fix stack_pointer in: ff39793
  2. update comments: 456bd1a (Refund Value in units of gas seems fine here. No need for Refund Value in gwei).
  3. Yes, I do plan to implement the bus-mapping for sstore. And like feat: add sload op (bus-mapping version) #347, I believe we will need Modularize mock crate for easier and more customizable testing setup generation #349 gets merged first. Otherwise I cannot test is_persist and is_warm conveniently. So, should I turn this PR into draft, and then work on it after Modularize mock crate for easier and more customizable testing setup generation #349 being merged?

@ed255 ed255 mentioned this pull request Mar 14, 2022
Closed
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Mar 14, 2022

  1. Yes, I do plan to implement the bus-mapping for sstore. And like feat: add sload op (bus-mapping version) #347, I believe we will need [WIP] Modularize mock crate for easier and more customizable testing setup generation #349 gets merged first. Otherwise I cannot test is_persist and is_warm conveniently. So, should I turn this PR into draft, and then work on it after [WIP] Modularize mock crate for easier and more customizable testing setup generation #349 being merged?

Thanks for pointing out that the bus-mapping implementation is blocked by #349
In that case I think it's OK to proceed without the bus-mapping implementation in order to avoid blocking this PR :)
I've opened an issue to track the bus-mapping implementation at #388

Also let's wait for a second review before merging!

ed255
ed255 approved these changes Mar 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

CPerezz
CPerezz approved these changes Mar 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!! Nice job!!
Good catch with the stack positions too @ed255

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Mar 14, 2022

@HAOYUatHZ once the PR is rebased with main and the conflicts are resolved, we can merge this :)

@adria0 adria0 mentioned this pull request Mar 14, 2022
Closed
11 tasks
@0xmountaintop
feat: add sstore circuit
fae98b5 
rebase

update comments

fix reversion

fix stack_pointer
@0xmountaintop 0xmountaintop changed the title feat: add sstore circuit feat: add sstore circuit Mar 15, 2022
@CPerezz CPerezz merged commit bdcb0c3 into privacy-ethereum:main Mar 15, 2022
@0xmountaintop 0xmountaintop deleted the sstore branch March 15, 2022 09:42
@0xmountaintop 0xmountaintop mentioned this pull request Apr 1, 2022
Closed
zemse pushed a commit to zemse/zkevm-circuits that referenced this pull request Mar 22, 2023
@silathdiir
Replace WordByteRangeGadget with WordByteCapGadget in `ErrorInval…
9c8b997 
…idJumpGadget`. (privacy-ethereum#383)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ed255 ed255 ed255 approved these changes

@CPerezz CPerezz CPerezz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@0xmountaintop @ed255 @CPerezz