Feature/fix rlp push0 #286

laisolizq · 2023-05-05T16:05:34Z

Add push0
Fix RLP data length

ignasirv · 2023-05-08T08:03:01Z

main/utils.zkasm

@@ -872,10 +872,13 @@ SHLarithfinal:

 ; out of counters full tracer event trigger
 outOfCountersStep:
+    $                           :MLOAD(isLoadRLP),JMPNZ(handleOOCSatRLP)


What do you think adding this check only once at handleBatchError function?
$ :MLOAD(isLoadRLP),JMPNZ(invalidTxRLP)
And remove the handleOOCXatRLP from the code

and add $ => SR :MLOAD(batchSR) in invalidTxRLP

ignasirv · 2023-05-08T08:05:14Z

main/vars.zkasm

+VAR CTX initTouchedSR ; touched root once a new context begins
+VAR CTX isLoadRLP ; flag to determine if the function is called from RLP loop


Change to isLoadingRLP or isProcessingRLP.
Also, it should be Global. Now we are just setting the flag to 0 at the last context.

ignasirv · 2023-05-08T08:23:47Z

main/main.zkasm


        $ => A          :MLOAD(batchL2DataLength)
        $ => C          :MLOAD(batchL2DataParsed)
        C - A           :JMPN(loadTx_rlp, endCheckRLP)
 endCheckRLP:
+        ; set flag isLoadRLP to 0
+        0               :MSTORE(isLoadRLP)


The var now is of type CTX. We have to options:

Set the flag to 0 before calling loadTx_rlp: this way, all the CTX are set to 0

Make the var GLOBAL: in this case we should set it to 1 only once just before txLoopRLP

The current situation is that we are only setting the flag to 0 for the last processed context

krlosMata · 2023-05-08T08:40:26Z

main/opcodes/stack-operations.zkasm

+
+opPUSH0:
+    ; check out-of-gas
+    GAS - %GAS_QUICK_STEP => GAS  :JMPN(outOfGas)


lint: align with following instructions

krlosMata · 2023-05-08T08:48:01Z

main/utils.zkasm

@@ -872,10 +872,13 @@ SHLarithfinal:

 ; out of counters full tracer event trigger
 outOfCountersStep:
+    $                           :MLOAD(isLoadRLP),JMPNZ(handleOOCSatRLP)


and add $ => SR :MLOAD(batchSR) in invalidTxRLP

krlosMata · 2023-05-08T08:58:17Z

main/utils.zkasm

@@ -1153,6 +1156,7 @@ finalPush:
 VAR GLOBAL tmpVarDaddB
 VAR GLOBAL tmpZkPCaddB
 VAR GLOBAL auxBytes
+VAR GLOBAL auxLengthBytes


Not only used in this function, so this global variable should be in vars.zkasm

I think we can compute in invalidTxRLP:

batchHashPos - batchL2DataLength to check how many bytes are missing to fill in the the batchHashData

so we can save the auxLengthBytes variable

abdulmenan10 · 2023-05-10T16:13:24Z

Yes

ignasirv

💯

krlosMata

😸

gumb0 · 2023-05-29T07:53:34Z

main/opcodes/stack-operations.zkasm

@@ -7,6 +7,15 @@
 *  - stack input: none
 *  - stack output: [pushed_value]
 */
+
+opPUSH0:


Does it not consume any ZK counters? I would think that gas check at stack check do. Even POP requires 100 steps.

Yes, we would add them

gumb0 · 2023-05-29T07:58:46Z

main/opcodes/stack-operations.zkasm

@@ -7,6 +7,15 @@
 *  - stack input: none
 *  - stack output: [pushed_value]
 */


This comment applies to opPUSH1 and further, opPUSH0 should be above it and with its own comment.

Agree !! We will add specific comment for PUSH0

Also you need a newer link https://www.evm.codes/#5f?fork=shanghai

gumb0 · 2023-05-29T09:00:57Z

main/load-tx-rlp.zkasm

 invalidTxRLP:
 ;; Append all missing 'batchL2Data' to 'batchDataHash' bytes
        $ => B                          :MLOAD(batchL2DataLength)
        $ => C                          :MLOAD(batchHashPos)
+        $${p = C}


Why is this needed? Seems like it can change behaviour (of getTxs()?) significantly

p is the pointer to the read transactions bytes via getTxs().

While reading RLP, we add bytes to the batchHash and perform some check afterwards, but the bytes are already added. So, if anything goes wrong we jump to invalidTxRLP to load the remaining bytes. The remaining bytes where added starting at p, where the p was set during the RLP parsing. This was always true before, but if we perform the check before the bytes are added to the batchHash, then it will not match the batchHash.

The generic way to add the missing batchHash bytes was to simply set the pointer p to starting byte to add, which is stored in batchHashPos

So as I understand this error processing is now called from more places (utils generating out-of-counters errors), including from the points where p is not updated yet, so it is initialized explicitly.

The remaining bytes where added starting at p, where the p was set during the RLP parsing.

But for old cases, where this error processing is called after p is already set, wouldn't this code here add more bytes than needed?

So as I understand this error processing is now called from more places (utils generating out-of-counters errors), including from the points where p is not updated yet, so it is initialized explicitly.

exactly!

The remaining bytes where added starting at p, where the p was set during the RLP parsing.

But for old cases, where this error processing is called after p is already set, wouldn't this code here add more bytes than needed?

for old cases, new code will just only set the p where it was already read, so it will just only rewrite the p with same value

gumb0 · 2023-05-29T09:11:52Z

main/utils.zkasm

@@ -928,6 +928,7 @@ handleError:
 handleBatchError:
    ; restore init state root and finish batch
    $ => SR         :MLOAD(batchSR)
+    $               :MLOAD(isLoadingRLP),JMPNZ(invalidTxRLP)


Would be good to have a comment why RLP error processing is different

In case isLoadingRLP is 1 and you jump to invalidTxRLP (and later to processTxsEnd) the isLoadingRLP remains 1. May this cause any problems (e.g. infinite loops) or confusion?

This flag is not used outside the zkevm-rom so it could not cause any issue.
I would say the opposite, it could add information if the limit counters has been reached during the RLP

gumb0 · 2023-05-29T10:01:07Z

Some general thoughts about RLP fix, maybe missing the entire picture and all implications.

In general it feels like fixing the problem of managing one global thing (ZK counters, behaving differently in different contexts), with adding more global data. Overall this increases complexity and potential implicit interdependencies and difficulty of reasoning about code.

Ideally I think it would be better to make this dependency explicit by e.g. having an extra flag parameter to those utils that are called from RLP processing and should process counter errors differently.

I can see that this might be impractical if those utils are also called from many other places. Another less radical suggestion to consider could be: do the branching based on global isLoadingRLP only inside requried utils instead of lower-level handleBatchError. This way the fix would affect only some limited scope of functionality, comparing to affecting every batch error processing like now. And it would still be more explicit which utils depend on this global state.
(Downside of this approach could be if RLP processing changes to using some new utils, you have to remember to check isLoadingRLP in each one)

krlosMata · 2023-05-29T11:17:08Z

Some general thoughts about RLP fix, maybe missing the entire picture and all implications.

In general it feels like fixing the problem of managing one global thing (ZK counters, behaving differently in different contexts), with adding more global data. Overall this increases complexity and potential implicit interdependencies and difficulty of reasoning about code.

Ideally I think it would be better to make this dependency explicit by e.g. having an extra flag parameter to those utils that are called from RLP processing and should process counter errors differently.

I can see that this might be impractical if those utils are also called from many other places. Another less radical suggestion to consider could be: do the branching based on global isLoadingRLP only inside requried utils instead of lower-level handleBatchError. This way the fix would affect only some limited scope of functionality, comparing to affecting every batch error processing like now. And it would still be more explicit which utils depend on this global state. (Downside of this approach could be if RLP processing changes to using some new utils, you have to remember to check isLoadingRLP in each one)

We decided to go with the isLoadingRLP approach since it povides better security when adding new code to the RLP parsing since it is handled globally and it is not function specific. Also, RLP error where just removed and all zk-counters are handled via handleBatchError

laisolizq added 2 commits April 25, 2023 12:08

add push0

d77f64a

fix RLP data length

a273144

laisolizq requested review from krlosMata, invocamanman and ignasirv as code owners May 5, 2023 16:05

cla-bot bot added the cla-signed label May 5, 2023

laisolizq marked this pull request as draft May 5, 2023 16:05

ignasirv reviewed May 8, 2023

View reviewed changes

update package

04c8a1a

krlosMata requested changes May 8, 2023

View reviewed changes

laisolizq added 2 commits May 8, 2023 17:41

review & update package

2c9cca7

update forkid 5

a23dc63

abdulmenan10 approved these changes May 10, 2023

View reviewed changes

laisolizq requested review from krlosMata and ignasirv May 11, 2023 07:51

ignasirv approved these changes May 11, 2023

View reviewed changes

update comment ecrecover & packages

b35e5a4

krlosMata mentioned this pull request May 11, 2023

fix comment ecrecover #287

Closed

laisolizq marked this pull request as ready for review May 11, 2023 08:14

laisolizq requested a review from zkronos73 as a code owner May 11, 2023 08:14

invocamanman approved these changes May 11, 2023

View reviewed changes

krlosMata approved these changes May 11, 2023

View reviewed changes

krlosMata merged commit adc6ccb into develop May 11, 2023

gumb0 reviewed May 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fix rlp push0 #286

Feature/fix rlp push0 #286

laisolizq commented May 5, 2023

ignasirv May 8, 2023 •

edited

Loading

krlosMata May 8, 2023

ignasirv May 8, 2023 •

edited

Loading

ignasirv May 8, 2023 •

edited

Loading

krlosMata May 8, 2023

krlosMata May 8, 2023

krlosMata May 8, 2023

krlosMata May 8, 2023

abdulmenan10 May 10, 2023

abdulmenan10 commented May 10, 2023

ignasirv left a comment

krlosMata left a comment

gumb0 May 29, 2023

krlosMata May 29, 2023

gumb0 May 29, 2023

krlosMata May 29, 2023

chfast May 29, 2023

gumb0 May 29, 2023

krlosMata May 29, 2023 •

edited

Loading

gumb0 May 29, 2023

gumb0 May 29, 2023

krlosMata May 29, 2023

krlosMata May 29, 2023

gumb0 May 29, 2023

chfast May 30, 2023

krlosMata May 31, 2023

gumb0 commented May 29, 2023

krlosMata commented May 29, 2023

		VAR CTX initTouchedSR ; touched root once a new context begins
		VAR CTX isLoadRLP ; flag to determine if the function is called from RLP loop

Feature/fix rlp push0 #286

Feature/fix rlp push0 #286

Conversation

laisolizq commented May 5, 2023

ignasirv May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ignasirv May 8, 2023 • edited Loading

Choose a reason for hiding this comment

ignasirv May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulmenan10 commented May 10, 2023

ignasirv left a comment

Choose a reason for hiding this comment

krlosMata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krlosMata May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented May 29, 2023

krlosMata commented May 29, 2023

ignasirv May 8, 2023 •

edited

Loading

ignasirv May 8, 2023 •

edited

Loading

ignasirv May 8, 2023 •

edited

Loading

krlosMata May 29, 2023 •

edited

Loading