Effective gas implementation #290

ignasirv · 2023-05-24T12:25:07Z

Effective gas price implementation.
In order to be more precise with gasPrice and low tx fees, the sequencer is able to propose a reduction of the signed gasPrice. This param, effectiveGasPrice, is passed at the last bytes of the encoded tx and present in the batchL2Data. The final effectiveGasPrice is compute as:

effectiveGasPrice = floor((gasPrice * (effectivePercentage + 1)) / 256)

txGasPrice is replaced by effectiveGasPrice among tx processing

krlosMata

😸

main/process-tx.zkasm

invocamanman · 2023-05-26T11:00:17Z

nice ^^ :D

gumb0 · 2023-05-29T10:51:06Z

main/process-tx.zkasm

+        ; get value above 256 bits
+        ${_effGasPriceShifted >> 256} => D
+        ; compute ARITH
+        ${_effGasPriceShifted} => E              :ARITH


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)

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

gumb0 · 2023-05-29T11:49:50Z

main/process-tx.zkasm

+        ${_effGasPriceShifted / 256} => A        :MSTORE(txGasPrice)
+        ${_effGasPriceShifted % 256} => C
+        ; compute ARITH
+        E                                       :ARITH


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)

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.

gumb0 · 2023-05-29T11:51:46Z

main/process-tx.zkasm

@@ -68,9 +70,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)


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

gumb0 · 2023-05-29T11:52:41Z

main/process-tx.zkasm

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


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

Suggested change

; A => gasPrice

; A = gasPrice

gumb0 · 2023-05-29T11:52:58Z

main/process-tx.zkasm

        $ => A                          :MLOAD(txGasPriceRLP)
-        A                               :MSTORE(txGasPrice)
+        ; B => effectivePercentage


Same as above

Suggested change

; B => effectivePercentage

; B = effectivePercentage

gumb0 · 2023-05-29T11:54:11Z

main/process-tx.zkasm

-        A                               :MSTORE(txGasPrice)
+        ; B => effectivePercentage
+        $ => B                          :MLOAD(effectivePercentageRLP)
+        ; B => [0, 255]


Cosmetic: if the comment supposed to explain the effect of the following line

Suggested change

; B => [0, 255]

; B = [1, 256]

review changes

chfast · 2023-05-30T09:26:27Z

main/load-tx-rlp.zkasm

@@ -285,6 +285,11 @@ vREADTx:
        A                               :MSTORE(txV)
        C + D => C                      :CALL(addBatchHashData)

+;; read effective percentage
+effectivePercentageTx:


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

yes, it is there for clarity

chfast · 2023-05-30T09:28:18Z

main/load-tx-rlp.zkasm

@@ -285,6 +285,11 @@ vREADTx:
        A                               :MSTORE(txV)
        C + D => C                      :CALL(addBatchHashData)

+;; read effective percentage
+effectivePercentageTx:
+        1 => D                          :CALL(getTxBytes)


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

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

chfast · 2023-05-30T10:23:53Z

main/process-tx.zkasm

+        ; compute ARITH
+        E                                       :ARITH
+        ; check divisor > remainder
+        C => A


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

Is the usage of LT covered by CNT_BINARY - 100?

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

Effective gas implementation

42dd1a4

ignasirv requested review from krlosMata, laisolizq and invocamanman as code owners May 24, 2023 12:25

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

ignasirv requested review from krlosMata, invocamanman, laisolizq, fractasy and zkronos73 and removed request for invocamanman, laisolizq, krlosMata and fractasy May 24, 2023 12:25

krlosMata approved these changes May 26, 2023

View reviewed changes

main/process-tx.zkasm Outdated Show resolved Hide resolved

PR review

fbf5a82

invocamanman approved these changes May 26, 2023

View reviewed changes

gumb0 reviewed May 29, 2023

View reviewed changes

ignasirv and others added 2 commits May 29, 2023 13:09

Add arith checker at beginning of processTx

d09f1c5

review changes

7d966f8

gumb0 reviewed May 29, 2023

View reviewed changes

ignasirv force-pushed the feature/effective-gas-price branch from d09f1c5 to 33034b9 Compare May 29, 2023 12:47

Add arith checker at beginning of processTx

ac51215

ignasirv force-pushed the feature/effective-gas-price branch from 33034b9 to ac51215 Compare May 29, 2023 12:49

Merge pull request #291 from 0xPolygonHermez/feature/PR-push0-rlp-review

fcb8126

review changes

chfast reviewed May 30, 2023

View reviewed changes

krlosMata merged commit eb376cc into develop May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Effective gas implementation #290

Effective gas implementation #290

ignasirv commented May 24, 2023 •

edited by krlosMata

Loading

krlosMata left a comment

invocamanman commented May 26, 2023

gumb0 May 29, 2023

ignasirv May 29, 2023

gumb0 May 29, 2023

gumb0 May 29, 2023

gumb0 May 29, 2023

gumb0 May 29, 2023

gumb0 May 29, 2023

gumb0 May 29, 2023

chfast May 30, 2023

krlosMata May 31, 2023

chfast May 30, 2023

krlosMata May 31, 2023

chfast May 30, 2023

chfast May 30, 2023

ignasirv May 30, 2023 •

edited

Loading

Effective gas implementation #290

Effective gas implementation #290

Conversation

ignasirv commented May 24, 2023 • edited by krlosMata Loading

krlosMata left a comment

Choose a reason for hiding this comment

invocamanman commented May 26, 2023

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

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

ignasirv May 30, 2023 • edited Loading

Choose a reason for hiding this comment

ignasirv commented May 24, 2023 •

edited by krlosMata

Loading

ignasirv May 30, 2023 •

edited

Loading