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 all 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: 1 addition & 1 deletion main/constants.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ CONST %LOCAL_EXIT_ROOT_STORAGE_POS = 1
CONST %LAST_TX_STORAGE_POS = 0
CONST %STATE_ROOT_STORAGE_POS = 1
CONST %MAX_MEM_EXPANSION_BYTES = 0x3fffe0
CONST %FORK_ID = 4
CONST %FORK_ID = 5

; RLP
CONST %MIN_VALUE_SHORT = 128
Expand Down
2 changes: 1 addition & 1 deletion main/ecrecover/ecrecover.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ ecrecover_store_args:
0n => A
$ :EQ,JMPC(ecrecover_s_is_zero)

; r and s in [1, FNEC-1]
; compute r inverse
$ => A :MLOAD(ecrecover_r),CALL(invFnEc)
B :MSTORE(ecrecover_r_inv)

Expand Down
18 changes: 5 additions & 13 deletions main/load-tx-rlp.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ INCLUDE "load-tx-rlp-utils.zkasm"
loadTx_rlp:
; check one keccak is available to begin processing the RLP
$ => D :MLOAD(cntKeccakPreProcess)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - 1 - D :JMPN(handleOOCKatRLP)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - 1 - D :JMPN(outOfCountersKeccak)

; A new hash with position 0 is started
0 => HASHPOS
Expand Down Expand Up @@ -59,7 +59,7 @@ endList:
136 :MSTORE(arithB), CALL(divARITH); in: [arithA, arithB] out: [arithRes1: arithA/arithB, arithRes2: arithA%arithB]
$ => B :MLOAD(arithRes1)
$ => D :MLOAD(cntKeccakPreProcess)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - B - D - 1:JMPN(handleOOCKatRLP)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - B - D - 1:JMPN(outOfCountersKeccak)

;; Read RLP 'nonce'
; 64 bits max
Expand Down Expand Up @@ -290,30 +290,22 @@ vREADTx:
;;;;;;;;;
;; update bytes parsed
$ => A :MLOAD(batchL2DataParsed)
A + C => A :MSTORE(batchL2DataParsed)
A + C :MSTORE(batchL2DataParsed)
;; increase number of transaction to process
$ => A :MLOAD(pendingTxs)
A + 1 => A :MSTORE(pendingTxs)
A + 1 :MSTORE(pendingTxs)
;; compute signature
$ => A :HASHKDIGEST(E)
A :MSTORE(txHash), JMP(txLoopRLP)

;;;;;;;;;
;; E - Handler error RLP fields
;;;;;;;;;
handleOOCBatRLP:
$${eventLog(onError, OOCB)} :JMP(handleOOCatRLP)
handleOOCKatRLP:
$${eventLog(onError, OOCK)} :JMP(handleOOCatRLP)
handleOOCSatRLP:
$${eventLog(onError, OOCS)} :JMP(handleOOCatRLP)
handleOOCatRLP:
$ => SR :MLOAD(batchSR)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@krlosMata krlosMata May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$ => HASHPOS :MLOAD(batchHashPos)
$ => E :MLOAD(batchHashDataId)

Expand Down
9 changes: 7 additions & 2 deletions main/main.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ skipSetGlobalExitRoot:
$ => B :MLOAD(arithRes1)
; Compute minimum necessary keccaks to finish the batch
B + 1 + %MIN_CNT_KECCAK_BATCH => B :MSTORE(cntKeccakPreProcess)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - B :JMPN(handleOOCKatRLP)
%MAX_CNT_KECCAK_F - CNT_KECCAK_F - B :JMPN(outOfCountersKeccak)

;;;;;;;;;;;;;;;;;;
;; C - Loop parsing RLP transactions
Expand All @@ -102,6 +102,8 @@ skipSetGlobalExitRoot:
A :MSTORE(ctxTxToUse) ; Points at first context to be used when processing transactions

$${var p = 0}
; set flag isLoadingRLP to 1
1 :MSTORE(isLoadingRLP)

txLoopRLP:
$ => A :MLOAD(lastCtxUsed)
Expand All @@ -110,7 +112,10 @@ txLoopRLP:
$ => A :MLOAD(batchL2DataLength)
$ => C :MLOAD(batchL2DataParsed)
C - A :JMPN(loadTx_rlp, endCheckRLP)

endCheckRLP:
; set flag isLoadingRLP to 0
0 :MSTORE(isLoadingRLP)
:JMP(txLoop)

;;;;;;;;;;;;;;;;;;
Expand All @@ -120,7 +125,7 @@ endCheckRLP:

txLoop:
$ => A :MLOAD(pendingTxs)
A-1 => A :MSTORE(pendingTxs), JMPN(processTxsEnd)
A-1 :MSTORE(pendingTxs), JMPN(processTxsEnd)

$ => A :MLOAD(ctxTxToUse) ; Load first context used by transaction
A+1 => CTX :MSTORE(ctxTxToUse),JMP(processTx)
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)
; 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
8 changes: 5 additions & 3 deletions main/utils.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,7 @@ handleError:
handleBatchError:
; restore init state root and finish batch
$ => SR :MLOAD(batchSR)
$ :MLOAD(isLoadingRLP),JMPNZ(invalidTxRLP)
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$${eventLog(onFinishTx)}
$${eventLog(onFinishBatch)} :JMP(processTxsEnd)

Expand Down Expand Up @@ -1153,19 +1154,20 @@ finalPush:
VAR GLOBAL tmpVarDaddB
VAR GLOBAL tmpZkPCaddB
VAR GLOBAL auxBytes

;@info: adds data to batchHashdata byte by byte
;@in: A: bytes to add
;@in D: bytes length
addBatchHashByteByByte:
%MAX_CNT_STEPS - STEP - 10 :JMPN(handleOOCSatRLP)
%MAX_CNT_STEPS - STEP - 10 :JMPN(outOfCountersStep)
RR :MSTORE(tmpZkPCaddB)
A :MSTORE(auxBytes)
D :MSTORE(tmpVarDaddB)
1 => D

utilsAddBatchHashBytebyByteLoop:
%MAX_CNT_STEPS - STEP - 50 :JMPN(handleOOCSatRLP)
%MAX_CNT_BINARY - CNT_BINARY - 1 :JMPN(handleOOCBatRLP)
%MAX_CNT_STEPS - STEP - 50 :JMPN(outOfCountersStep)
%MAX_CNT_BINARY - CNT_BINARY - 1 :JMPN(outOfCountersBinary)
32 - D => D
$ => A :MLOAD(auxBytes), CALL(SHRarith); in: [A: value, D: #bytes to right shift] out: [A: shifted result]
; get last byte in A
Expand Down
1 change: 1 addition & 0 deletions main/vars.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ VAR GLOBAL SPw ; aux variable to store Stack pointer 'SP'
VAR GLOBAL auxSR ; auxiliary variable. Temporary state root
VAR GLOBAL txRLPLength ; transaction RLP list length
VAR GLOBAL txDataRead ; aux variable to check transaction 'data' left that needs to be read
VAR GLOBAL isLoadingRLP ; flag to determine if the function is called from RLP loop

VAR CTX txGasLimit ; transaction parameter: 'gas limit'
VAR CTX txDestAddr ; transaction parameter: 'to'
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
"yargs": "^17.5.1"
},
"devDependencies": {
"@0xpolygonhermez/zkevm-proverjs": "github:0xPolygonHermez/zkevm-proverjs#c9adbddba82cb1dcc3b03f3dcb0f317627ac05b8",
"@0xpolygonhermez/zkevm-testvectors": "github:0xPolygonHermez/zkevm-testvectors#v1.1.0-fork.4",
"@0xpolygonhermez/zkevm-commonjs": "github:0xPolygonHermez/zkevm-commonjs#v1.0.0",
"@0xpolygonhermez/zkevm-proverjs": "github:0xPolygonHermez/zkevm-proverjs#3bc0368dd67caf92d38e3cbebe7b030e78c54e35",
"@0xpolygonhermez/zkevm-testvectors": "github:0xPolygonHermez/zkevm-testvectors#v1.2.0-rc.1-fork.5",
"@0xpolygonhermez/zkevm-commonjs": "github:0xPolygonHermez/zkevm-commonjs#v1.0.1-rc.3",
"chai": "^4.3.6",
"chalk": "^3.0.0",
"eslint": "^8.25.0",
Expand Down