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

StepRws in EVM circuits #1616

Closed

Conversation

nullity00
Copy link

@nullity00 nullity00 commented Sep 19, 2023

Description

Similar to #1582 , We use the StepRws struct to automatically track read-write indices.

Issue Link

#1586

Type of change

  • New feature (non-breaking change which adds functionality)

Contents

The change has been made to the following files

  • - create.rs
  • - end_tx.rs
  • - error_oog_call.rs
  • - error_oog_memory_copy.rs
  • - error_oog_sload_sstore.rs
  • - error_return_data_oo_bound.rs
  • - error_write_protection.rs
  • - extcodecopy.rs
  • - extcodehash.rs
  • - extcodesize.rs
  • - logs.rs
  • - return_revert.rs
  • - returndatacopy.rs
  • - sha3.rs
  • - sload.rs
  • - sstore.rs

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Sep 19, 2023
@nullity00 nullity00 marked this pull request as ready for review September 19, 2023 16:08
@nullity00
Copy link
Author

@ChihChengLiang Hey, let me know what you think of this !

@ChihChengLiang ChihChengLiang self-requested a review September 20, 2023 10:41
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Hi @nullity00,
Thanks for taking this issue! I did the first found of review and left some comments. Please take a look.

Comment on lines 481 to 484
rws.next(); // 0 - TxId
rws.next(); // 1 - Depth
let rw_end_of_reversion = rws.next().call_context_value().as_usize(); // 2 - RwCounterEndOfReversion
let is_persistent = rws.next().call_context_value().as_usize() != 0; // 3 - IsPersistent
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great we write the intention of the read-write, e.g. TxId, Depth, RwCounterEndOfReversion, and IsPersistent.
But I think it defeats the purpose of using StepRws when adding counters in comments, e.g. 0, 1, 2, 3, or rw_off + x. We should remove the counters in all the changes in this PR.

One purpose of using StepRws is to add new features without changing all the magic numbers that aren't relevant to the feature. If we add the counters on comments, they have to be updated when the new feature is added. It is also to forget to update the comments.

Suggested change
rws.next(); // 0 - TxId
rws.next(); // 1 - Depth
let rw_end_of_reversion = rws.next().call_context_value().as_usize(); // 2 - RwCounterEndOfReversion
let is_persistent = rws.next().call_context_value().as_usize() != 0; // 3 - IsPersistent
rws.next(); // TxId
rws.next(); // Depth
let rw_end_of_reversion = rws.next().call_context_value().as_usize(); // RwCounterEndOfReversion
let is_persistent = rws.next().call_context_value().as_usize() != 0; // IsPersistent

zkevm-circuits/src/evm_circuit/execution/create.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/create.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/create.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CI Issues related to the Continuous Integration mechanisms of the repository. label Sep 21, 2023
@github-actions github-actions bot removed the CI Issues related to the Continuous Integration mechanisms of the repository. label Sep 21, 2023
@nullity00 nullity00 marked this pull request as draft September 21, 2023 16:42
@ChihChengLiang
Copy link
Collaborator

Hi @nullity00, do you see any blocker in this PR? Is there anything we can help?

Note that you can use block.debug_print_txs_steps_rw_ops(); to debug the read-writes, as #1586 describes.

@ed255
Copy link
Member

ed255 commented Nov 2, 2023

Closing due to inactivity. @nullity00 if you would like to retake this let us know and we can open the PR again :)

@ed255 ed255 closed this Nov 2, 2023
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
### Description

Continuing the work originally started in
#1616
this PR seeks to introduce `StepRws` across the evm circuits to reduce
the need for tracking rw indices through code.

### Issue Link
Closes
#1586

### Type of change
Refactor (no updates to logic)

### Contents

The change has been made to the following files

 - error_oog_call.rs
 - error_oog_memory_copy.rs
 - error_oog_sload_sstore.rs
 - error_return_data_oo_bound.rs
 - error_write_protection.rs
 - extcodecopy.rs
 - extcodehash.rs
 - extcodesize.rs
 - logs.rs
 - return_revert.rs
 - returndatacopy.rs
 - sha3.rs
 - sload.rs
 - sstore.rs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants