This repository has been archived by the owner on Jul 5, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 272
Specs for TLOAD TSTORE #516
Merged
zemse
merged 5 commits into
privacy-scaling-explorations:master
from
zemse:511-tload-tstore
Mar 23, 2024
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# ErrorOutOfGasTloadTstore state for both TLOAD and TSTORE OOG errors | ||
|
||
## Procedure | ||
|
||
Handle the corresponding out of gas errors for both `TLOAD` and `TSTORE` opcodes. | ||
|
||
### EVM behavior | ||
|
||
The out of gas error may occur for `constant gas`. | ||
|
||
#### TLOAD gas cost | ||
|
||
For this gadget, TLOAD gas cost in EIP-1153 is specified as the cost of hot SLOAD, currently `100`. | ||
|
||
#### TSTORE gas cost | ||
|
||
For this gadget, TSTORE gas cost in EIP-1153 is specified as the cost of SSTORE on an already SSTOREd slot, currently `100`. | ||
|
||
### Constraints | ||
|
||
1. For TLOAD, constrain `gas_left < gas_cost`. | ||
2. For TSTORE, constrain `gas_left < gas_cost`. | ||
3. Only for TSTORE, constrain `is_static == false`. | ||
4. Current call must fail. | ||
5. If it's a root call, it transits to `EndTx`. | ||
6. If it isn't a root call, it restores caller's context by reading to `rw_table`, then does step state transition to it. | ||
7. Constrain `rw_counter_end_of_reversion = rw_counter_end_of_step + reversible_counter`. | ||
|
||
### Lookups | ||
|
||
7 bus-mapping lookups for TLOAD and 8 for TSTORE: | ||
|
||
1. 5 call context lookups for `tx_id`, `is_static`, `callee_address`, `is_success` and `rw_counter_end_of_reversion`. | ||
2. 1 stack read for `transient_storage_key`. | ||
3. Only for TSTORE, 1 stack read for `value_to_store`. | ||
4. Only for TSTORE, 1 account transient storage read. | ||
|
||
## Code | ||
|
||
> TODO |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# TLOAD & TSTORE opcodes | ||
|
||
## Variables definition | ||
|
||
| Name | Value | | ||
| - | - | | ||
| SLOAD_GAS | 100 | | ||
|
||
## Constraints | ||
|
||
1. opcodeId checks | ||
1. opId === OpcodeId(0x5c) for `TLOAD` | ||
2. opId === OpcodeId(0x5d) for `TSTORE` | ||
2. state transition: | ||
- gc | ||
- `TLOAD`: +7 | ||
- 4 call_context read | ||
- 2 stack operations | ||
- 1 transient storage reads | ||
- `TSTORE`: +8 | ||
- 5 call_context read | ||
- 2 stack operations | ||
- 1 transient storage reads/writes | ||
- stack_pointer | ||
- `TLOAD`: remains the same | ||
- `TSTORE`: -2 | ||
- pc + 1 | ||
- reversible_write_counter | ||
- `TLOAD`: +0 | ||
- `TSTORE`: +1 (for transient storage) | ||
- gas: | ||
- `TLOAD`: | ||
- gas + SLOAD_GAS | ||
- `SSTORE`: | ||
- gas + SLOAD_GAS | ||
3. lookups: | ||
- `TLOAD`: 8 busmapping lookups | ||
zemse marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- call_context: | ||
- `tx_id`: Read the `tx_id` for this tx. | ||
- `rw_counter_end_of_reversion`: Read the `rw_counter_end` if this tx get reverted. | ||
- `is_persistent`: Read if this tx will be reverted. | ||
- `callee_address`: Read the `callee_address` of this call. | ||
- stack: | ||
- `key` is popped off the top of the stack | ||
- `value` is pushed on top of the stack | ||
- transient storage: The 32 bytes of `value` are read from storage at `key` | ||
- `TSTORE`: 10 busmapping lookups | ||
zemse marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- call_context: | ||
- `tx_id`: Read the `tx_id` for this tx. | ||
- `is_static`: Read the call's property `is_static` | ||
- `rw_counter_end_of_reversion`: Read the `rw_counter_end` if this tx get reverted. | ||
- `is_persistent`: Read if this tx will be reverted. | ||
- `callee_address`: Read the `callee_address` of this call. | ||
- stack: | ||
- `key` is popped off the top of the stack | ||
- `value` is popped off the top of the stack | ||
- transient storage: | ||
- The 32 bytes of new `value` are written to transient storage at `key`, with the previous `value` and `committed_value` | ||
|
||
## Exceptions | ||
|
||
1. gas out: remaining gas is not enough | ||
2. stack underflow: | ||
- the stack is empty: `1024 == stack_pointer` | ||
- only for `TSTORE`: contains a single value: `1023 == stack_pointer` | ||
3. context error | ||
- only for `TSTORE`: the current execution context is from a `STATICCALL` (since Cancun fork). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it 6 instead of 7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 7 is correct because it is in 92TLOAD_93TSTORE.md too? Or can you please correct me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, 7 doesn't match the listed lookups below (5 call context lookups and 1 stack read). Note that for
TLOAD
we don't needis_static
. But on the other hand, one stack operation seems to be missing (value read), so this is the reason for the difference in failure and success cases. But I agree, I think these two numbers should be the same.Note, however, that the numbers in failure/success cases are different for
SLOAD/SSTORE
too. Is there a mistake?One additional thing that I find a bit confusing is that the failure case is using only
is_success
while the success case is using onlyis_persistent
, shouldn't be used both in both cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I saw SLOAD's OOG also having
is_static
in the specs. That needs to be fixed?It would be very helpful if @ChihChengLiang or @ed255 could chime in regarding this.
Yeah the same name should be used.
is_persistent
seems to convey the idea better thanis_success
. I can update that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have another look, but just before you do any modifications - note that
is_persistent
andis_success
are two different fields ofCall
struct which mean different things (is't not just the naming):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
is_success
andis_persistent
are ok as they are (is_success
needs to be false and is checked for each OOG error inCommonErrorGadget
).I think if you remove
is_static
lookup for from the error lookups forSLOAD
, it should be fine (that would sum up to 5SLOAD
lookups in the error case).SLOAD/SSTORE
docs are ok in my opinion, except that forSSLOAD
there is a redundantis_static
in the error case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that SLOAD OOG doesn't need to read
is_static
, and the same would happen for TLOAD OOG. On the other hand, by looking at the implementation I see thatis_static
value is ignored in the SLOAD OOG case:https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/4996eb6c11fb4eb4bcde7eff55827ecf6cfaaf03/zkevm-circuits/src/evm_circuit/execution/error_oog_sload_sstore.rs#L70-L71
So the implementation is technically correct, even though we could skip the read of
is_static
in the SLOAD case. I think this is a very minor detail; skipping the read means one less row in the RwTable, which is an optimization, but I think the effects are negligible.Do you mean that
SLOAD.rwc_delta == OOG_SLOAD.rwc_delta
andSSTORE.rwc_delta == OOG_SSTORE.rwc_delta
?If that's the case, notice that in a success SLOAD, there's the value pushed to the stack, but in OOG, the call will be aborted, so no one is going to read the result of SLOAD, which means there's no need to push a result value to the stack. So that's one RwTable lookup that appears in SLOAD but not in OOG_SLOAD. There are other differences like in SLOAD we update the access list, but in OOG_SLOAD we don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Edu! Yes, I think it's ok as it is (I realised later there's no additional push in the error case). Agree that the additional
is_static
call in TLOAD/SLOAD doesn't change things much, so I am approving the PR.