Conversation
CodSpeed Performance ReportMerging #3132 will degrade performances by 3.96%Comparing Summary
Benchmarks breakdown
|
| table[TLOAD as usize] = Instruction::new(host::tload, 100); | ||
| table[TSTORE as usize] = Instruction::new(host::tstore, 100); | ||
| table[MCOPY as usize] = Instruction::new(memory::mcopy, 0); // static 2, mostly dynamic | ||
| table[MCOPY as usize] = Instruction::new(memory::mcopy, 3); // static 2, mostly dynamic |
There was a problem hiding this comment.
MCOPY has a static VERY_LOG gas
VERYLOW is 3gas.
revm/crates/interpreter/src/gas/calc.rs
Lines 110 to 111 in d0bb48e
There was a problem hiding this comment.
revm/crates/interpreter/src/gas/constants.rs
Lines 7 to 8 in fdf606d
| // Transfer value cost | ||
| if has_transfer { | ||
| gas += CALLVALUE; | ||
| } |
There was a problem hiding this comment.
| 5000 | ||
| } else { | ||
| 0 | ||
| } |
| const fn frontier_sstore_cost(vals: &SStoreResult) -> u64 { | ||
| if vals.is_present_zero() && !vals.is_new_zero() { | ||
| SSTORE_SET | ||
| } else { | ||
| SSTORE_RESET | ||
| } | ||
| } |
There was a problem hiding this comment.
Frontier part as it has special logic is moved here:
revm/crates/interpreter/src/gas/params.rs
Lines 271 to 281 in 403f28f
This is without load cost that is now part of static instruction gas
| let mut gas_cost = istanbul_sstore_cost::<WARM_STORAGE_READ_COST, WARM_SSTORE_RESET>(vals); | ||
|
|
||
| if is_cold { | ||
| gas_cost += COLD_SLOAD_COST; | ||
| } |
There was a problem hiding this comment.
is cold is now always added but cold cost is zero before berlin here:
revm/crates/interpreter/src/gas/params.rs
Lines 284 to 288 in 403f28f
| if vals.is_new_eq_present() { | ||
| SLOAD_GAS | ||
| } else if vals.is_original_eq_present() && vals.is_original_zero() { | ||
| SSTORE_SET | ||
| } else if vals.is_original_eq_present() { | ||
| SSTORE_RESET_GAS | ||
| } else { | ||
| SLOAD_GAS | ||
| } |
There was a problem hiding this comment.
This code is slighly changes as SLOAD_GAS is not part of the static instruction gas. So we have fewer branches
It is easy to follow commnets
revm/crates/interpreter/src/gas/params.rs
Lines 289 to 300 in 403f28f
| pub const fn static_sstore_cost(spec_id: SpecId) -> u64 { | ||
| if spec_id.is_enabled_in(SpecId::BERLIN) { | ||
| WARM_STORAGE_READ_COST | ||
| } else if spec_id.is_enabled_in(SpecId::ISTANBUL) { | ||
| ISTANBUL_SLOAD_GAS | ||
| } else { | ||
| SSTORE_RESET | ||
| } |
There was a problem hiding this comment.
This is moved to a variable inside GasParams.
revm/crates/interpreter/src/gas/params.rs
Line 157 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Line 149 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Line 127 in 403f28f
| pub const fn sload_cost(spec_id: SpecId, is_cold: bool) -> u64 { | ||
| if spec_id.is_enabled_in(SpecId::BERLIN) { | ||
| if is_cold { | ||
| COLD_SLOAD_COST | ||
| } else { | ||
| WARM_STORAGE_READ_COST | ||
| } | ||
| } else if spec_id.is_enabled_in(SpecId::ISTANBUL) { | ||
| // EIP-1884: Repricing for trie-size-dependent opcodes | ||
| ISTANBUL_SLOAD_GAS | ||
| } else if spec_id.is_enabled_in(SpecId::TANGERINE) { | ||
| // EIP-150: Gas cost changes for IO-heavy operations | ||
| 200 | ||
| } else { | ||
| 50 | ||
| } | ||
| } |
There was a problem hiding this comment.
Has become part of static instruction gas.
And additional gas is handled here:
revm/crates/interpreter/src/instructions/host.rs
Lines 193 to 210 in 403f28f
| if power.is_zero() { | ||
| Some(EXP) | ||
| } else { | ||
| // EIP-160: EXP cost increase | ||
| let gas_byte = U256::from(if spec_id.is_enabled_in(SpecId::SPURIOUS_DRAGON) { | ||
| 50 | ||
| } else { | ||
| 10 | ||
| }); | ||
| let gas = U256::from(EXP) | ||
| .checked_add(gas_byte.checked_mul(U256::from(log2floor(power) / 8 + 1))?)?; | ||
|
|
||
| u64::try_from(gas).ok() | ||
| } | ||
| } |
There was a problem hiding this comment.
Dynamic part is in GasParams here:
revm/crates/interpreter/src/gas/params.rs
Lines 193 to 201 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Lines 144 to 147 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Lines 110 to 111 in 403f28f
And static part (EXP) is now part of static instruction gas
| pub const fn create2_cost(len: usize) -> Option<u64> { | ||
| CREATE.checked_add(tri!(cost_per_word(len, KECCAK256WORD))) | ||
| } |
There was a problem hiding this comment.
Fully moved to:
revm/crates/interpreter/src/gas/params.rs
Lines 399 to 405 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Line 120 in 403f28f
revm/crates/interpreter/src/gas/params.rs
Line 116 in 403f28f
| if !vals.is_present_zero() && vals.is_new_zero() { | ||
| REFUND_SSTORE_CLEARS | ||
| } else { | ||
| 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
before instanbul is moved here:
revm/crates/interpreter/src/gas/params.rs
Lines 305 to 316 in 403f28f
| if vals.is_new_eq_present() { | ||
| 0 | ||
| } else { |
There was a problem hiding this comment.
part where we return zero is now here
revm/crates/interpreter/src/gas/params.rs
Lines 317 to 321 in 403f28f
| if vals.is_original_eq_present() && vals.is_new_zero() { | ||
| sstore_clears_schedule | ||
| } else { |
There was a problem hiding this comment.
This part is not refactored here
revm/crates/interpreter/src/gas/params.rs
Lines 322 to 327 in 403f28f
| if !vals.is_original_zero() { | ||
| if vals.is_present_zero() { | ||
| refund -= sstore_clears_schedule; | ||
| } else if vals.is_new_zero() { | ||
| refund += sstore_clears_schedule; | ||
| } | ||
| } | ||
|
|
||
| if vals.is_original_eq_new() { | ||
| let (gas_sstore_reset, gas_sload) = if spec_id.is_enabled_in(SpecId::BERLIN) { | ||
| (SSTORE_RESET - COLD_SLOAD_COST, WARM_STORAGE_READ_COST) | ||
| } else { | ||
| (SSTORE_RESET, sload_cost(spec_id, false)) | ||
| }; | ||
| if vals.is_original_zero() { | ||
| refund += (SSTORE_SET - gas_sload) as i64; | ||
| } else { | ||
| refund += (gas_sstore_reset - gas_sload) as i64; | ||
| } | ||
| } | ||
|
|
||
| refund |
There was a problem hiding this comment.
And last part is now here. Added additional comments from EIP
revm/crates/interpreter/src/gas/params.rs
Lines 328 to 355 in 403f28f
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a configurable gas parameters system (GasParams) for the EVM interpreter, enabling easy modification of dynamic gas costs for opcodes. The implementation replaces hardcoded gas calculations with a flexible, spec-aware table-based approach.
Key Changes:
- Introduced
GasParamsstruct with gas cost table that adapts based on EVM spec version - Refactored all instruction implementations to use
GasParamsinstead of direct gas constant references - Updated memory resize operations to accept and use
GasParams - Added gas parameter configuration in handler initialization
- Enhanced test script with
--keep-goingflag support
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/run-tests.sh |
Added --keep-going flag support for continuing tests after failures |
crates/interpreter/src/gas/params.rs |
New core gas parameters implementation with configurable gas table |
crates/interpreter/src/interpreter.rs |
Integrated GasParams into interpreter struct and initialization |
crates/interpreter/src/interpreter/shared_memory.rs |
Updated memory resize to use GasParams and return Result |
crates/interpreter/src/instructions/*.rs |
Refactored all instruction gas calculations to use GasParams |
crates/interpreter/src/gas/*.rs |
Removed deprecated gas calculation functions, kept core utilities |
crates/handler/src/*.rs |
Updated handler to configure gas parameters based on spec |
crates/context/src/journal/inner.rs |
Minor type alias cleanup |
crates/context/interface/src/context.rs |
Added helper methods to SStoreResult |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !is_spurious_dragon || transfers_value { | ||
| return NEWACCOUNT; | ||
| } | ||
| 0 |
There was a problem hiding this comment.
Moved to:
revm/crates/interpreter/src/gas/params.rs
Lines 442 to 453 in 0ee23df
| &mut self, | ||
| evm: &mut Self::Evm, | ||
| ) -> Result<ExecutionResult<Self::HaltReason>, Self::Error> { | ||
| self.configure(evm); |
There was a problem hiding this comment.
will this not work properly now for any impl overriding this trait? i don't think this is acceptable and we need to either hide this deeper or make it so this change is obvious from compilation issues after the bump
| ctx, | ||
| precompiles, | ||
| frame_input, | ||
| self.instruction.gas_params(), |
There was a problem hiding this comment.
can gas_params not be part of the Context?
this should allow us to avoid any changes to InstructionProvider and to the frame init logic which now propagate the GasParams quite deeply just to set the interpreter field while this is technically an externally provided context
it would also only retain the actual dynamic runtime data in Interpreter type
There was a problem hiding this comment.
this also makes me wonder if GasParams could be entirely hidden inside of the InstructionProvider? so that I just do something like EthInstructions::new(gas_params) and it produces an instruction table that uses my gas params?
A way to configure all gas-related parameters in EVM so we can easily change dynamic part of gas of opcode.