Specs for TLOAD TSTORE

[zemse]

• add markdown specs
• work on the python implementation

Closes #511

[miha-stopar]

Nice work! Just a couple of questions about numbers.

[miha-stopar]

Is it 6 instead of 7?

[zemse]

I think 7 is correct because it is in 92TLOAD_93TSTORE.md too? Or can you please correct me?

[miha-stopar]

Hm, 7 doesn't match the listed lookups below (5 call context lookups and 1 stack read). Note that for TLOAD we don't need is_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 only is_persistent, shouldn't be used both in both cases?

[zemse]

Note that for TLOAD we don't need is_static.

I agree. I saw SLOAD's OOG also having is_static in the specs. That needs to be fixed?

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?

It would be very helpful if @ChihChengLiang or @ed255 could chime in regarding this.

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 only is_persistent, shouldn't be used both in both cases?

Yeah the same name should be used. is_persistent seems to convey the idea better than is_success. I can update that.

[miha-stopar]

I will have another look, but just before you do any modifications - note that is_persistent and is_success are two different fields of Call struct which mean different things (is't not just the naming):

/// This call is persistent or call stack reverts at some point
pub is_persistent: bool,
/// This call ends successfully or not
pub is_success: bool,

[miha-stopar]

I think is_success and is_persistent are ok as they are (is_success needs to be false and is checked for each OOG error in CommonErrorGadget).

I think if you remove is_static lookup for from the error lookups for SLOAD, it should be fine (that would sum up to 5 SLOAD lookups in the error case).

SLOAD/SSTORE docs are ok in my opinion, except that for SSLOAD there is a redundant is_static in the error case.

[ed255]

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 that is_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.

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?

Do you mean that SLOAD.rwc_delta == OOG_SLOAD.rwc_delta and SSTORE.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.

[miha-stopar]

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.

[rrtoledo]

Only had doubts about the number of lookups but this was resolved in threads.

[ed255]

@zemse it seems this is ready to be merged :) Feel free to merge yourself!