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

Jump to bottom

Effective gas implementation #290

Merged
merged 6 commits into from
May 31, 2023
Merged

Effective gas implementation #290

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42dd1a4
Effective gas implementation
ignasirv May 24, 2023
fbf5a82
PR review
ignasirv May 26, 2023
d09f1c5
Add arith checker at beginning of processTx
ignasirv May 29, 2023
7d966f8
review changes
krlosMata May 29, 2023
ac51215
Add arith checker at beginning of processTx
ignasirv May 29, 2023
fcb8126
Merge pull request #291 from 0xPolygonHermez/feature/PR-push0-rlp-review
krlosMata May 29, 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
5 changes: 5 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 @@ -285,6 +285,11 @@ vREADTx:
A :MSTORE(txV)
C + D => C :CALL(addBatchHashData)

;; read effective percentage
effectivePercentageTx:
Copy link

Choose a reason for hiding this comment

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

This label is never called. Is this just a documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it is there for clarity

1 => D :CALL(getTxBytes)
Copy link

Choose a reason for hiding this comment

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

The description of getTxBytes does not say what C is used for.

Copy link
Contributor

Choose a reason for hiding this comment

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

C description is at the very beginning of the rlp-processing: https://github.com/0xPolygonHermez/zkevm-rom/blob/main/main/load-tx-rlp.zkasm#L28

A :MSTORE(effectivePercentageRLP)
C + D => C :CALL(addBatchHashData)
;;;;;;;;;
;; D - Finish RLP parsing
;;;;;;;;;
Expand Down
29 changes: 27 additions & 2 deletions main/process-tx.zkasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,34 @@ endCheckChainId:
:CALL(isColdAddress) ; add tx.origin to touched addresses
0 :MSTORE(depth) ; Initial depth is 0

;; Set gasPrice global var
;; Set gasPrice global var depending on effectivePercentage [0-255] -> txGasPrice = Floor((gasPrice * (effectivePercentage + 1)) / 256)
; _effGasPriceShifted = gasPrice * (effectivePercentage + 1)
Copy link

Choose a reason for hiding this comment

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

Cosmetic: this comment is duplicated on line 83, maybe it's not needed here

; A => gasPrice
Copy link

Choose a reason for hiding this comment

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

Cosmetic: notation with => is confusing here, because meaning is opposite of zkasm syntax

Suggested change
; A => gasPrice
; A = gasPrice

$ => A :MLOAD(txGasPriceRLP)
A :MSTORE(txGasPrice)
; B => effectivePercentage
Copy link

Choose a reason for hiding this comment

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

Same as above

Suggested change
; B => effectivePercentage
; B = effectivePercentage

$ => B :MLOAD(effectivePercentageRLP)
; B => [0, 256]
ignasirv marked this conversation as resolved.
Show resolved Hide resolved
B + 1 => B
; A*B + C = D * 2**256 + op(E)
0 => C
; _effGasPriceShifted = gasPrice * (effectivePercentage + 1)
$${var _effGasPriceShifted = A * B}
; get value above 256 bits
${_effGasPriceShifted >> 256} => D
; compute ARITH
${_effGasPriceShifted} => E :ARITH
Copy link

Choose a reason for hiding this comment

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

Should we have out of arith counters check for this code?
(or any other counters (steps, binary?) given that this adds quite a bit of computation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree we should add an arith checker at beginning of process-tx.zkasm


; txGasPrice = _effGasPriceShifted / 256
256 => B
; (_effGasPriceShifted / 256)(A) * 256(B) + (_effGasPriceShifted % 256)(C) = D * 2**256 + op(E)
${_effGasPriceShifted / 256} => A :MSTORE(txGasPrice)
${_effGasPriceShifted % 256} => C
; compute ARITH
E :ARITH
Copy link

Choose a reason for hiding this comment

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

Maybe I still don't quite understand how this works, so correct where if I'm wrong.

I believe the purpose of this ARITH is to contstrain the free calculations above (_effGasPriceShifted / 256 and _effGasPriceShifted % 256)

This ARITH line calculates AB + C and updates E and D.

What would stop malicious prover to return arbitrary value into A instead of _effGasPriceShifted / 256? This ARITH doesn't seem to constrain neither of registers values (unlike the first ARITH above?)
(for C it is more clear, it is at least checked for <256 afterwards)

Copy link

Choose a reason for hiding this comment

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

Actually I think I understand now: it takes into account previous values of E and D and checks result of ARITH against them, because this expression doesn't have an assignment to register.

; check divisor > remainder
C => A
Copy link

Choose a reason for hiding this comment

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

To confirm: LT treats arguments as unsigned. I.e. this will work in case C is "negative".

Copy link

Choose a reason for hiding this comment

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

Is the usage of LT covered by CNT_BINARY - 100?

Copy link
Contributor Author

@ignasirv ignasirv May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

LT uses 1 binary, so yes.
LT treats arguments as unsigned, it compares two 256 bit scalars. To treat them as signed, we also have the registry SLT.
In case C is "negative" it will be treated as a Scalar

1 :LT

;;;;;;;;;;;;;;;;;;
;; D - Verify and increase nonce
;;;;;;;;;;;;;;;;;;
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 @@ -56,6 +56,7 @@ VAR CTX storageAddr ; address which the storage will be modified
VAR CTX txValue ; transaction parameter: 'value'
VAR CTX txNonce ; transaction parameter: nonce
VAR CTX txGasPriceRLP ; transaction parameter: 'gasPrice' decoded from the RLP
VAR CTX effectivePercentageRLP ; transaction parameter: 'effectivePercentage' decoded from the RLP
VAR CTX txChainId ; transaction parameter: 'chainId'
VAR CTX txS ; transaction parameter: ecdsa signature S
VAR CTX txR ; transaction parameter: ecdsa signature R
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
},
"devDependencies": {
"@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",
"@0xpolygonhermez/zkevm-testvectors": "github:0xPolygonHermez/zkevm-testvectors#feature/effective-gas-price",
"@0xpolygonhermez/zkevm-commonjs": "github:0xPolygonHermez/zkevm-commonjs#feature/effective-gas-price",
"chai": "^4.3.6",
"chalk": "^3.0.0",
"eslint": "^8.25.0",
Expand Down