Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/fix rlp push0 #286

Merged
merged 6 commits into from
May 11, 2023
Merged

Feature/fix rlp push0 #286

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d77f64a
add push0
laisolizq Apr 25, 2023
a273144
fix RLP data length
laisolizq May 3, 2023
04c8a1a
update package
laisolizq May 8, 2023
2c9cca7
review & update package
laisolizq May 8, 2023
a23dc63
update forkid 5
laisolizq May 10, 2023
b35e5a4
update comment ecrecover & packages
krlosMata May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions main/load-tx-rlp.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,8 @@ handleOOCatRLP:

invalidTxRLP:
;; Append all missing 'batchL2Data' to 'batchDataHash' bytes
$ => B :MLOAD(auxLengthBytes)
$${p = p - B}
$ => B :MLOAD(batchL2DataLength)
$ => C :MLOAD(batchHashPos)
$ => HASHPOS :MLOAD(batchHashPos)
Expand Down
4 changes: 4 additions & 0 deletions main/main.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,15 @@ skipSetGlobalExitRoot:
txLoopRLP:
$ => A :MLOAD(lastCtxUsed)
A+1 => CTX :MSTORE(lastCtxUsed)
; set flag isLoadRLP to 1
1 :MSTORE(isLoadRLP)

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

@ignasirv ignasirv May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

:JMP(txLoop)

;;;;;;;;;;;;;;;;;;
Expand Down
2 changes: 1 addition & 1 deletion main/map-opcodes.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ mapping_opcodes:
:JMP(opINVALID) ; 0x5C
:JMP(opINVALID) ; 0x5D
:JMP(opINVALID) ; 0x5E
:JMP(opINVALID) ; 0x5F
:JMP(opPUSH0) ; 0x5F
:JMP(opPUSH1) ; 0x60
:JMP(opPUSH2) ; 0x61
:JMP(opPUSH3) ; 0x62
Expand Down
9 changes: 9 additions & 0 deletions main/opcodes/stack-operations.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@
* - stack input: none
* - stack output: [pushed_value]
*/
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree !! We will add specific comment for PUSH0

Copy link

Choose a reason for hiding this comment

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

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


opPUSH0:
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we would add them

; check out-of-gas
GAS - %GAS_QUICK_STEP => GAS :JMPN(outOfGas)
Copy link
Contributor

Choose a reason for hiding this comment

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

lint: align with following instructions

; store stack output
0 :MSTORE(SP++); [0 => SP]
; check stack overflow
%CALLDATA_OFFSET - SP :JMPN(stackOverflow, readCode)

opPUSH1:
; number of bytes to push to D
1 => D
Expand Down
7 changes: 7 additions & 0 deletions main/utils.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,10 +872,13 @@ SHLarithfinal:

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

@ignasirv ignasirv May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$${eventLog(onError, OOCS)} :JMP(handleBatchError)
outOfCountersKeccak:
$ :MLOAD(isLoadRLP),JMPNZ(handleOOCKatRLP)
$${eventLog(onError, OOCK)} :JMP(handleBatchError)
outOfCountersBinary:
$ :MLOAD(isLoadRLP),JMPNZ(handleOOCBatRLP)
$${eventLog(onError, OOCB)} :JMP(handleBatchError)
outOfCountersMemalign:
$${eventLog(onError, OOCM)} :JMP(handleBatchError)
Expand Down Expand Up @@ -1153,6 +1156,7 @@ finalPush:
VAR GLOBAL tmpVarDaddB
VAR GLOBAL tmpZkPCaddB
VAR GLOBAL auxBytes
VAR GLOBAL auxLengthBytes
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Yes

;@info: adds data to batchHashdata byte by byte
;@in: A: bytes to add
;@in D: bytes length
Expand All @@ -1161,6 +1165,7 @@ addBatchHashByteByByte:
RR :MSTORE(tmpZkPCaddB)
A :MSTORE(auxBytes)
D :MSTORE(tmpVarDaddB)
D :MSTORE(auxLengthBytes)
1 => D

utilsAddBatchHashBytebyByteLoop:
Expand All @@ -1174,6 +1179,8 @@ utilsAddBatchHashBytebyByteLoop:
D => B
; add last byte to batchHashData
1 => D :CALL(addBatchHashData); in:[D: length of the hash]
$ => D :MLOAD(auxLengthBytes)
D - 1 :MSTORE(auxLengthBytes)
; check loop
B => D
; D + 1 => D, we set 33 instead of 32 to earn 1 step
Expand Down
3 changes: 2 additions & 1 deletion main/vars.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,5 @@ VAR CTX salt ; CREATE2 parameter 'salt' used to compute new contract address
VAR CTX gasCTX ; remaining gas in the origin CTX when a new context is created
VAR CTX dataStarts; hash position where de transaction 'data' starts in the batchHashData
VAR CTX isPreEIP155 ; flag to check if the current tx is legacy, previous to Spurious Dragon (EIP-155)
VAR CTX initTouchedSR ; touched root once a new context begins
VAR CTX initTouchedSR ; touched root once a new context begins
VAR CTX isLoadRLP ; flag to determine if the function is called from RLP loop
Copy link
Contributor

@ignasirv ignasirv May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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