core: implement EIP-8037, state creation gas cost increase#33601
core: implement EIP-8037, state creation gas cost increase#33601MariusVanDerWijden wants to merge 5 commits into
Conversation
MariusVanDerWijden
force-pushed
the
eip-8037
branch
from
f130451 to
ac0edb7
Compare
MariusVanDerWijden
requested review from
fjl,
gballet,
lightclient,
rjl493456442,
s1na and
zsfelfoldi
as code owners
MariusVanDerWijden
force-pushed
the
eip-8037
branch
4 times, most recently
from
89179ad to
15b3ac5
Compare
MariusVanDerWijden
force-pushed
the
eip-8037
branch
from
9222e0a to
c570357
Compare
MariusVanDerWijden
force-pushed
the
eip-8037
branch
3 times, most recently
from
54243de to
fb6a02a
Compare
| return GasCosts{}, ErrGasUintOverflow | ||
| } | ||
| return totalCost, nil | ||
| return GasCosts{RegularGas: totalCost}, nil |
There was a problem hiding this comment.
We should return the stateGas explicitly in the return and add the pre-deducted value back?
There was a problem hiding this comment.
I think there was a reason why I did it this way. The ordering when we charge state gas and when we charge regular gas matters
| // the necessary steps to create accounts and reverses the state in case of an | ||
| // execution error or failed value transfer. | ||
| func (evm *EVM) Call(caller common.Address, addr common.Address, input []byte, gas uint64, value *uint256.Int) (ret []byte, leftOverGas uint64, err error) { | ||
| func (evm *EVM) Call(caller common.Address, addr common.Address, input []byte, gas GasCosts, value *uint256.Int) (ret []byte, leftOverGas GasCosts, gasUsed GasUsed, err error) { |
There was a problem hiding this comment.
Nitpick, It will be much more readable if:
(1) change gas to gasBudget
(2) change leftOverGas to gasBudget
and I am wondering if it's feasible/makesense to change to gasBudget as the pointer, so that the leftOverGas can be eliminated.
There was a problem hiding this comment.
I tried it, but its not great. Neither for readability nor useability
I would prefer not to do it
There was a problem hiding this comment.
Is the gasBudget idea bad for readability or the pointer idea is bad, or both of them :)
| return GasCosts{}, err | ||
| } | ||
| evm.callGasTemp, err = callGas(evm.chainRules.IsEIP150, contract.Gas.RegularGas, intrinsic.RegularGas, stack.Back(0)) | ||
| evm.callGasTemp, err = callGas(evm.chainRules.IsEIP150, contract.Gas.RegularGas, intrinsic, stack.Back(0)) |
There was a problem hiding this comment.
The semantics here is ambiguous. It should be
- intrinsicFunc should return a vector of gas cost, including both regular and state gas cost
- the remaining regular gas should be calculated after deducting the intrinsic cost
- the 63/64 rule should be applied on the regular gas only
- the regular gas budget for child frame should then be capped by the user-specified amount
| } | ||
|
|
||
| // Compute and charge state gas (new account creation) AFTER regular gas. | ||
| stateGas, err := stateGasFunc(evm, contract, stack, mem, memorySize) |
There was a problem hiding this comment.
I am not sure if the stateGasFunc is necessary. In the EIP-7702 check, the state access already happens evm.StateDB.GetCode(addr). The gas metering reordering becomes moot.
| // When the calldata floor exceeds actual gas used, any | ||
| // remaining state gas must also be consumed. | ||
| targetRemaining := (st.initialBudget.RegularGas + st.initialBudget.StateGas) - floorDataGas | ||
| st.gasRemaining.StateGas = 0 |
There was a problem hiding this comment.
It looks very weird to empties the state gas.
In theory, if there are some state gas left, we should deduct from the regular gas first for floor cost cap.
I guess in this case, we only care about the sum of leftover regular gas and state gas for ether refund (to sender).
There was a problem hiding this comment.
Needs to be checked against spec
| fee.Mul(fee, effectiveTipU256) | ||
|
|
||
| // always read the coinbase account to include it in the BAL (TODO check this is actually part of the spec) | ||
| st.state.GetBalance(st.evm.Context.Coinbase) |
There was a problem hiding this comment.
AddBalance will also touch the account regardless of the amount to be collected
MariusVanDerWijden
force-pushed
the
eip-8037
branch
from
e5c644d to
fc42a77
Compare
MariusVanDerWijden
force-pushed
the
eip-8037
branch
from
fc42a77 to
84cbc04
Compare
| if subGasAmount > cost.StateGas { | ||
| subGasAmount -= cost.StateGas | ||
| } else { | ||
| subGasAmount = 0 |
| // exceeded the intrinsic-charged state gas | ||
| txState := cost.StateGas | ||
| if execGasUsed.StateGas > 0 { | ||
| txState += uint64(execGasUsed.StateGas) |
There was a problem hiding this comment.
Can we go lower than intrinsic gas here? We should theoretically do that. Check spec
| if !contract.IsSystemCall { | ||
| stateGas = params.StorageCreationSize * evm.Context.CostPerGasByte | ||
| } |
MariusVanDerWijden
force-pushed
the
eip-8037
branch
from
84cbc04 to
4c3abb8
Compare
| if evm.chainRules.IsEIP150 { | ||
| gas = params.SelfdestructGasEIP150 | ||
| var address = common.Address(stack.back(0).Bytes20()) | ||
| if gas > contract.Gas.RegularGas { |
There was a problem hiding this comment.
@MariusVanDerWijden why do we need to abort the gas calculation with a nil error here?
4 participants
Implements https://eips.ethereum.org/EIPS/eip-8037
mainly done in order to judge the complexity of the EIP
and to act as a jumping off point, since the eip will likely
change.