core: fix wrong refundGas when gasUsed < floorDataGas #31682
Conversation
| @@ -515,6 +515,7 @@ func (st *stateTransition) execute() (*ExecutionResult, error) { | |||
| if rules.IsPrague { | |||
| // After EIP-7623: Data-heavy transactions pay the floor gas. | |||
| if st.gasUsed() < floorDataGas { | |||
| gasRefund = 0 | |||
| prev := st.gasRemaining | |||
| st.gasRemaining = st.initialGas - floorDataGas | |||
There was a problem hiding this comment.
Not directly related to this PR, but I'm trying to figure out why we don't also subtract the base cost (21000) here.
| @@ -515,6 +515,7 @@ func (st *stateTransition) execute() (*ExecutionResult, error) { | |||
| if rules.IsPrague { | |||
| // After EIP-7623: Data-heavy transactions pay the floor gas. | |||
| if st.gasUsed() < floorDataGas { | |||
| gasRefund = 0 | |||
There was a problem hiding this comment.
It's probably not correct to blindly reset the refund counter to zero.
e.g., the used gas is 100, refund is 10, floor is 95;
the used gas will be reset to 95.
If (usedGas:95, refund: 0) is returned to gas estimator, then the peak gas usage will be (95 + params.CallStipend) * 64 / 63 which is lower than (100 + params.CallStipend) * 64 / 63.
Without sufficient gas allowance, the evm execution will be aborted due to oog at some point. And the gas estimation might be affected
There was a problem hiding this comment.
about your example, it won't be triggered as 100 > 95. Say we have:
initialGas = 200gasRemaining = 20gasRefund = 10
As gasRefund is added before the gasUsed check, we have:
gasRemaining = 20 + 10 = 30gasUsed = initialGas - gasRemaining = 170
Say floor is 180, then 170 < 180 so no refund should be reported:
gasRemaining = initialGas - floor = 200 - 180 = 20gasRefund = 0gasUsed = 200 - 20 = 180 = floor
effectively paying for the floor cost as well as returning the remaining up to initialGas. If we don't reset refundGas to 0, then it will still be 10, which breaks the gas accounting, that is, 180 gas used + 20 gas remaining + 10 gas refund = 210 > 200 initial gas
There was a problem hiding this comment.
twist your example a bit,
initialGas = 200gasRemaining = 20gasRefund = 10
As gasRefund is added before the gasUsed check, we have:
gasRemaining = 20 + 10 = 30gasUsed = initialGas - gasRemaining = 170
Say floor is 175, then 170 < 175
gasRemaining = initialGas - floor = 200 - 175 = 25gasUsed = floor
if we return gasUsed: 175, refund: 0, then the peak gas usage is interpreted as 175, rather than 180.
This peak gas usage is essential to accurately estimate the gas usage, also the reason why we do the
binary search instead of running the eth_call for one time.
There was a problem hiding this comment.
Then, do we let it as is, even when other clients do it differently? I'm not quite sure what to do here, or what the possible fix would be.
This PR addresses an issue reported on the Cantina contest. In short, when the gas used by the execution of a transaction is less than the floor gas from EIP 7623, the refunded gas must be zero, as done by other clients like reth. However, Geth does not set
gasRefundto0if that condition is met, so the returned gas refund is wrong.This does not pose an issue to the protocol as, when creating the associated receipt with
MakeReceipt, the fieldGasRefundedis ignored.