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

Transient Storage Support #1797

Merged
merged 17 commits into from
Apr 23, 2024
Merged

Transient Storage Support #1797

merged 17 commits into from
Apr 23, 2024

Conversation

zemse
Copy link
Member

@zemse zemse commented Apr 2, 2024 •
edited
Loading

#1761

  • Geth utils update
  • TLOAD TSTORE in bus mapping
  • TLOAD TSTORE in zkevm circuits

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Apr 2, 2024
@ed255 ed255 linked an issue Apr 4, 2024 that may be closed by this pull request
Implement TLOAD and TSTORE instructions #1761
Closed
@zemse zemse force-pushed the 1761-tload-tstore branch 2 times, most recently from ad27230 to a295f0f Compare April 6, 2024 04:22
@zemse zemse marked this pull request as ready for review April 6, 2024 04:23
@miha-stopar miha-stopar self-requested a review April 10, 2024 06:36
@ChihChengLiang
Copy link
Collaborator

@zemse Any idea on why the main tests are not passing?

@zemse
Copy link
Member Author

zemse commented Apr 11, 2024

The geth-utils is giving a panic: runtime error: invalid memory address or nil pointer dereference. Have been trying to debug but no progress.

zemse
zemse commented Apr 11, 2024
View reviewed changes
zkevm-circuits/src/evm_circuit/param.rs
@@ -9,7 +9,7 @@ use halo2_proofs::{
use std::collections::HashMap;

// Step dimension
pub(crate) const STEP_WIDTH: usize = 139;
pub(crate) const STEP_WIDTH: usize = 140;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was required because this assert statement fails.

@zemse
patch for some breaking changes in geth
4b15ed1 
seems that the format of trace errors from geth is changes
miha-stopar
miha-stopar reviewed Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

First pass

bus-mapping/src/operation.rs Outdated Show resolved Hide resolved
bus-mapping/src/operation.rs Outdated Show resolved Hide resolved
bus-mapping/src/operation/container.rs Outdated Show resolved Hide resolved
eth-types/src/evm_types/transient_storage.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/tload.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/tstore.rs Outdated Show resolved Hide resolved
zemse and others added 2 commits April 12, 2024 17:43
@zemse @miha-stopar
nit pics
a58a645 
Co-authored-by: Miha Stopar <miha.stopar@xlab.si>
@zemse
remove reversion info from tload
2425cdb
@github-actions github-actions bot added the crate-external-tracer Issues related to the external-tracer workspace member label Apr 12, 2024
@zemse @miha-stopar
nit pic
b9f5be9 
Co-authored-by: Miha Stopar <miha.stopar@xlab.si>
miha-stopar
miha-stopar reviewed Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

I only have one additional nitpick. It looks ok to me, but I am not sure what could be the cause for the test failure. I will have a look into it.

zkevm-circuits/src/witness/rw.rs Outdated Show resolved Hide resolved
zemse and others added 2 commits April 15, 2024 16:11
@zemse
fix the panic error from geth-utils
2c582b5
@zemse @miha-stopar
nit pic
b309ed5 
Co-authored-by: Miha Stopar <miha.stopar@xlab.si>
ChihChengLiang
ChihChengLiang reviewed Apr 17, 2024
View reviewed changes
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.

The CI test failure remains to be fixed.
Otherwise, LGTM.
I added some nitpicks

bus-mapping/src/evm/opcodes/tload.rs Outdated Show resolved Hide resolved
geth-utils/gethutil/trace.go Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/sstore.rs Outdated Show resolved Hide resolved
zemse and others added 2 commits April 22, 2024 17:28
@zemse @ChihChengLiang
fix bug in new geth logic
1b1a8a8 
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
@zemse @ChihChengLiang
nit pics
3dd7bd4 
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
@zemse
Copy link
Member Author

zemse commented Apr 22, 2024

Tests are passing now! @miha-stopar @ChihChengLiang

ChihChengLiang
ChihChengLiang approved these changes Apr 22, 2024
View reviewed changes
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.

LGTM. Great fix!

miha-stopar
miha-stopar approved these changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

LGTM!

@zemse zemse added this pull request to the merge queue Apr 23, 2024
Merged via the queue into privacy-scaling-explorations:main with commit e7171f6 Apr 23, 2024
15 checks passed
@zemse zemse deleted the 1761-tload-tstore branch April 23, 2024 09:21
@lispc
Copy link
Collaborator

lispc commented Apr 26, 2024

@zemse i think we should also modify get_rev_op_by_ref in input_state_ref.rs?

lispc
lispc reviewed Apr 26, 2024
View reviewed changes
bus-mapping/src/operation/container.rs
self.transient_storage
.push(Operation::new(rwc, rwc_inner_chunk, rw, op));
OperationRef::from((Target::TransientStorage, self.transient_storage.len() - 1))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

since tstore can be reversible, we should also write like this?

if reversible {
                    Operation::new_reversible(rwc, rwc_inner_chunk, rw, op)
                } else {
                    Operation::new(rwc, rwc_inner_chunk, rw, op)
                }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lispc lispc lispc left review comments

@miha-stopar miha-stopar miha-stopar approved these changes

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-external-tracer Issues related to the external-tracer workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement TLOAD and TSTORE instructions
4 participants
@zemse @ChihChengLiang @lispc @miha-stopar