Conversation
| ExtCode, // 700, Extcode | ||
| Balance, // 400, Balance |
There was a problem hiding this comment.
I'm removing the ExtCode and Balance tiers because they seem obsolete. The instructions using them no longer have a static cost independent of the EVM version so I think they should be using the Special tier.
| Balance, // 400, Balance | ||
| Special, // multiparam or otherwise special | ||
| Invalid // Invalid. | ||
| // NOTE: Tiers should be ordered by cost, since we sometimes perform comparisons between them. |
There was a problem hiding this comment.
It's also the only place other than GasMeter::runGas() where we refer to these tiers directly. All other uses are through GasMeter so the removal of the obsolete tiers will have no real effect on compiler behavior.
There was a problem hiding this comment.
BTW, I didn't have time to dig into this, but I wonder if that logic in Metrics.cpp is even correct. Is Tier::BlockHash (formerly called Tier::Ext) really supposed add 49 to to cost even though BLOCKHASH costs 20 gas? Should TLOAD/TSTORE also get 49? And isn't this also overpricing some instructions that are in Special not due to dynamic cost but rather becuse their cost is EVM-version dependent?
There was a problem hiding this comment.
Maybe, when Tier::Ext was introduced, the logic was not updated accordingly and it went unnoticed...
I have no idea where the value 49 comes from, but it would make sense that Tier::BlockHash should get a value of 20 and then the upcoming Tier::WarmAccess for transient storage opcodes should get 100.
However, I also noticed that SSTORE is in Tier::Special, which would make it underpriced ?
| try | ||
| { | ||
| // Run optimiser and compile the contract. | ||
| compiler->compileContract(_contract, _otherCompilers, cborEncodedMetadata); | ||
| } | ||
| catch(evmasm::OptimizerException const&) | ||
| { | ||
| solAssert(false, "Optimizer exception during compilation"); | ||
| } | ||
| // Run optimiser and compile the contract. | ||
| compiler->compileContract(_contract, _otherCompilers, cborEncodedMetadata); |
There was a problem hiding this comment.
I ran into this when I forgot to update GasMeter::runGas() and got an ICE without the original message and pointing here rather than at the original assert in GasMeter::runGas() that failed. Since OptimizerException inherits from util::Exception and we have top-level handlers for that, I think it's better to just let it through.
| namespace GasCosts | ||
| { | ||
| /// NOTE: The GAS_... constants referenced by comments are defined for each EVM version in the Execution Specs: | ||
| /// https://ethereum.github.io/execution-specs/autoapi/ethereum/<evm version>/vm/gas/index.html |
There was a problem hiding this comment.
Cancun spec is still in a branch: https://github.com/ethereum/execution-specs/blob/forks/cancun/src/ethereum/cancun/vm/gas.py.
Also, the links for earlier versions (e.g. gas docs for shanghai) are broken temporarily. There was a PR merged an hour ago and something must have gone wrong. Fortunately it can also be viewed in the repo. E.g. gas docs for shanghai on master.
| Low, // 5, Fast | ||
| Mid, // 8, Mid | ||
| High, // 10, Slow | ||
| Ext, // 20, Ext |
There was a problem hiding this comment.
I'm not sure where Ext even comes from. It's for BLOCKHASH, which in Execution Specs is simply GAS_BLOCK_HASH, and there's no GAS_EXT there. From the name I at first assumed it could be for EXTCODEHASH and EXTCODESIZE, which in Yellow Paper is in a group called W_extaccount (along with BALANCE), but that's not it. And even in the Yellow Paper BLOCKHASH is in a group of its own.
Since all of this is unnecessarily confusing, I decided to rename this tier to BlockHash to match the Execution Specs.
cameel
force-pushed
the
clean-and-document-gas-tiers
branch
2 times, most recently
from
443fb95 to
0f5d940
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
r0qs
removed
the
stale
cameel
force-pushed
the
clean-and-document-gas-tiers
branch
2 times, most recently
from
06495c3 to
1355f52
Compare
cameel
force-pushed
the
clean-and-document-gas-tiers
branch
from
1355f52 to
ade8096
Compare
ekpyron
left a comment
ekpyron
left a comment
There was a problem hiding this comment.
I've only superficially looked through this so far: but just to confirm: there is no actual behavioural change involved in any of this here or did I miss something? If so, I'm fine with just merging.
|
Yeah, this is just a refactor+docs. No non-trivial behavior changes. Like, the removed |
- By catching and asserting it we hide the line number and message. - OptimizerException inherits from util::Exception so just letting it through will have the same effect as asserting.
- They're no longer used because the cost depends on the EVM version.
cameel
force-pushed
the
clean-and-document-gas-tiers
branch
from
ade8096 to
a973b4c
Compare
4 participants
Related to #14739 (in particular #14737 (comment) and my upcoming
MCOPYPR).I needed to understand how those gas tiers work to add a new one for
MCOPY. In the end it turned out that I don't even need to do it (tiers only represent the static cost and for the dynamic part I just need to addMCOPYnext to other...COPYopcodes inGasMeter::estimateMax()). Still, this was not obvious at first and I had to spend some effort to dig up that info, so I decided to add some comments explaining how to use it and how those tiers relate to constants from Execution Specs and opcode groups from Yellow Paper.The PR also removes some obsolete tiers and an unnecessary exception handler (see review comments).