-
Notifications
You must be signed in to change notification settings - Fork 130
zkevm: add ECRECOVER, SHA2-256, IDENTITY and RIPEMD precompiles #1524
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.
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
base: main
Are you sure you want to change the base?
Conversation
d53e674
to
bdfaa17
Compare
b8d50b4
to
99f249d
Compare
Signed-off-by: Ignacio Hagopian <[email protected]>
bdfaa17
to
d56f4ef
Compare
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
@@ -13,7 +13,7 @@ static: | |||
solc: 0.8.21 | |||
zkevm: | |||
evm-type: zkevm | |||
fill-params: --from=Cancun --until=Prague -m zkevm ./tests | |||
fill-params: --from=Cancun --until=Prague --block-gas-limit 36000000 -m zkevm ./tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gas_limit
in all tests is configured via Environment().gas_limit
. Since by default the testing framework sets as twice the current mainnet one, that looks unfair/unexpected for zkevm tests.
Thus, I think it's much better to define the expectation via --block-gas-limit
for the filling.
pytest.param(0x02, 60, 12, 64, id="SHA2-256"), | ||
pytest.param(0x03, 600, 120, 64, id="RIPEMD-160"), | ||
pytest.param(0x04, 15, 3, 1, id="IDENTITY"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes_per_unit_of_work
is mainly a concept of for how many bytes of input the precompile has to do "computational work". In the case of hashes, is related to the block size of the compression function. For identity is "1" since work is linear (i.e., not "chunked")
cc @kevaundray
pytest.param(0x02, 60, 12, 64, id="SHA2-256"), | ||
pytest.param(0x03, 600, 120, 64, id="RIPEMD-160"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find a way to pull the static and dynamic costs from precompiles. Is there a way to do this? Mostly asking for future-proofness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not zkevm specific but good general benchmarks for EVM. Can we make this framework for benchmarks a bit more generic? I'd like to contribute my existing benchmarks and more.
This PR:
zkevm
pytest markers since the testing framework now automatically does this.Environment()
configured gas limits, which are expected to be correctly set via the new--block-gas-limit
filling flag.Cycle counts in the new precompiles:
The last three are all part of a generic test since these precompiles have the same input parameter structure (i.e.,
data
). The generic test automatically discovers the optimal input length for each case to amortize the static cost as much as possible against the quadratic cost of memory expansions. ECRECOVER is a separate test since the parameter input structure is particular.