Set osaka as the default EVM version
#16296
Conversation
matheusaaguiar
force-pushed
the
set-osaka-default
branch
from
6f51e68 to
ef438cd
Compare
nikola-matic
force-pushed
the
set-osaka-default
branch
3 times, most recently
from
5b9f3fe to
024b18c
Compare
There was a problem hiding this comment.
This was a bit annoying to figure out. Osaka introduced a transaction gas limit of 2^24 (16_777_216) gas limit; Hardhat team then initially implemented usage of this transaction gas limit in lieu of the block gas limit depending on the EVM version, which wouldn't have caused annoying failures in all of our external tests:
ProviderError: transaction gas limit (30000000) is greater than the cap (16777216)
However, they then decided to have this implemented in hardhat/EDR, which they did in fact do, but unfortunately, this has not been released yet, which means I had to explicitly set hardhat.blockGasLimit to 16_777_216 instead of the default (30_000_000). This now allows all tests to pass.
Here's a bit more context from the hardhat PR that introduced Osaka support.
| # Hardhat 3.0+ breaks the test suite | ||
| yarn add [email protected] | ||
| # v2.27.1 is the last v2 Hardhat (introduces Osaka support) | ||
| yarn add [email protected] |
There was a problem hiding this comment.
Self explanatory - Osaka was only introduced in 2.27.1.
cameel
left a comment
cameel
left a comment
There was a problem hiding this comment.
Approving, because it does the job, but could use a few minor tweaks if there's time to apply them.
| echo "module.exports.solidity = ${compiler_settings};" | ||
| echo "module.exports.networks.hardhat = module.exports.networks.hardhat || { hardfork: '${evm_version}' }" | ||
| echo "module.exports.networks.hardhat.hardfork = '${evm_version}'" | ||
| if [[ $evm_version == "osaka" ]]; then |
There was a problem hiding this comment.
This looks like a new place where we'll have to list all the newer EVM versions. Do we actually care about the exact value of the gas limit? Like, do any tests fail when it's too high because they specifically test for it or something.
It's important when developing dapps, but IMO for our testing it does not matter. We'd be better off disabling it, or at least set a very high value (same for all EVM versions) if that's not possible.
There was a problem hiding this comment.
The check is not strictly necessary, however, the gas value is (at least for Osaka) - in fact, setting it to as high a value as possible would be the opposite of what we want for Osaka; if it's set higher than 2^24, all of the external tests (hardhat based) will fail.
I believe that it won't be necessary after the next ERD release is out.
|
Oh, I see that we forgot to add CLZ to the list of EVM features under Target Options. We should add it now. The mention about EOF is outdated too. Perhaps we should remove it. Or at least remove it from this list and only mention it in the description of the |
nikola-matic
force-pushed
the
set-osaka-default
branch
2 times, most recently
from
327f71c to
849831e
Compare
test/externalTests/zeppelin.sh
Outdated
| # Fails under evmVersion=osaka, likely due to transaction gas limits introduced by EIP-7825 | ||
| sed -i "s|describe(\('RSA'\)|describe.skip(\1|g" test/utils/cryptography/RSA.test.js |
There was a problem hiding this comment.
Oh, so you think it's due to the gas limits and not EIP-7823: Set upper bounds for MODEXP?
I guess it could be true given that I checked the failing test and the all the parameters seemed in range. For the record, here's what I posted on the chat:
I checked their tests for
RSA.soland the data file they take the test parameters from (SigVer15_186-3.rsp), but I don't see any cases where any of those those parameters would exceed the limits. The limit is 1024 bytes while the cases go up to 512 bytes. Even accounting for some bug somewhere that would incorrectly count characters as bytes (values are hex-encoded so 2 chars encode one byte if you count correctly), we're still only at 1024, which is allowed.
Also EIP-7883: ModExp Gas Cost Increase would definitely contribute to increased gas use.
One thing that still does not add up for me is that this is a manually thrown Panic rather than an out-of-gas revert. tryModExp() forwards all gas to the staticcall so I'd expect the outer call to fail out of gas as well without being able to react. Or is the 1/64th of gas still enough for it to perform that manual revert?
There was a problem hiding this comment.
Ah, right, my brain is fried, it could also very well be due to https://eips.ethereum.org/EIPS/eip-7823 (in fact that's more likely).
I'm now not sure whether to add this to the comment as well, as it would risk another circleci re-run :/
There was a problem hiding this comment.
I mean, I dismissed this as less likely initially, but you may be on to something here :)
nikola-matic
force-pushed
the
set-osaka-default
branch
from
849831e to
32de249
Compare
matheusaaguiar
force-pushed
the
set-osaka-default
branch
from
32de249 to
5fc0f34
Compare
Previous PR for reference: #16030