core/vm, core/state: return error if gas is insufficient for cost #540

rjl493456442 · 2025-04-11T09:41:36Z

In the current implementation, we attempt to compute the required gas cost for
each operation, which is a good approach. By checking against the available gas,
we can terminate the computation early if there’s enough gas to cover the cost.

However, this introduces several issues:

(a) During gas cost computation, the access list may be mutated. Some operations
may require access to multiple state entries. It’s possible that part of the access
succeeds and gets cached in the access list, while the remaining accesses fail
due to an out-of-gas (OOG) error.

The semantics around whether the successfully cached entries should be reverted
in such cases is unclear. Under the current implementation, these entries are not
reverted, which could lead to inconsistent behavior in subsequent gas cost computations.

(b) Technically, it’s unnecessary to compute the exact gas cost if the available
gas is already insufficient. A simpler alternative is to have the gas cost function
return an error when there’s not enough gas. This error can then be propagated up
the call stack, ultimately terminating EVM execution and consuming all remaining gas.

For reference, see the implementation in gasCallEIP2929:
https://github.com/ethereum/go-ethereum/blob/master/core/vm/operations_acl.go#L196

In the current implementation, the required gas cost is also ambiguous. e.g. if
two state entries are accessed in the operation, the real gas cost should be
cost_1 + cost_2, unfortunately in some places, only cost_1 is returned. I would
prefer to simply return an error.

rjl493456442 · 2025-04-11T10:02:24Z

core/state/access_events.go

Shouldn't it be expected1 + expected2?

not sure I understand the question, it is expected1+expected2

in your implementation, the required gas is expected1+expected2, but the returned value is params.WarmStorageReadCostEIP2929 which i believe is a mistake

right it's a rebase error, in the source branch it's the correct behavior

rjl493456442 · 2025-04-11T10:03:08Z

core/state/access_events.go

if consumed < expected1, then the availableGas-consumed will be incorrect

although the returned expected gas cost is correct...

makes sense: we are returning the expected gas and we are saving a confusing check. I can fix it if you don't like it.

come to think of if, it's really confusing. I'll fix it if we don't go your way.

i would prefer to using my approach to aviod the complexity in the first place

rjl493456442 · 2025-04-11T10:07:38Z

core/vm/eips.go

It's very weird.

Theoretically, if the available gas is not sufficient, we should terminate the execution and deduct all the remaining gas outside.

It's very weird to deduct the gas cost regardless of the available gas is sufficient or not...

it's not that weird, any error condition would just "fall through". It's used a lot in languages that support passing errors.

rjl493456442 · 2025-04-11T10:10:28Z

core/vm/evm.go

the len(ret) > 0 is unnecessary, as the gas cost for empty code is always zero.

this seems to come from a rebase, I agree this is now useless

rjl493456442 · 2025-04-11T10:16:10Z

core/vm/operations_verkle.go

we should use contract.Gas - gas

yeah that was introduced when I moved from op to gas. Makes sense that it works, but we need to update the spec tests to make sure that this could have been caught.

gballet

It makes sense and it seems to work with the execution spec tests. I am quite surprised that the contract creation passes, I would have to check with the testnet

gballet · 2025-04-11T13:00:44Z

core/state/access_events.go

not sure I understand the question, it is expected1+expected2

gballet · 2025-04-11T14:40:39Z

core/state/access_events.go

makes sense: we are returning the expected gas and we are saving a confusing check. I can fix it if you don't like it.

gballet · 2025-04-11T14:41:30Z

core/state/access_events.go

come to think of if, it's really confusing. I'll fix it if we don't go your way.

gballet · 2025-04-11T14:45:05Z

core/vm/operations_verkle.go

yeah that was introduced when I moved from op to gas. Makes sense that it works, but we need to update the spec tests to make sure that this could have been caught.

gballet · 2025-04-11T14:46:03Z

core/state/access_events_test.go


 	// Check warm read cost
-	gas = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
+	gas, _ = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)


if a new parameter is added that could indicate an issue, it should be checked in tests.

gballet · 2025-04-11T15:16:46Z

core/vm/eips.go

it's not that weird, any error condition would just "fall through". It's used a lot in languages that support passing errors.

gballet · 2025-04-11T16:35:26Z

(a) During gas cost computation, the access list may be mutated. Some operations may require access to multiple state entries. It’s possible that part of the access succeeds and gets cached in the access list, while the remaining accesses fail due to an out-of-gas (OOG) error.

The semantics around whether the successfully cached entries should be reverted in such cases is unclear. Under the current implementation, these entries are not reverted, which could lead to inconsistent behavior in subsequent gas cost computations.

That's not an issue: we are using this to build the witness, and the gas costs correspond to that use case. Once the witness has been paid for, the next access in the same transaction will be cheaper, we want it to be this way.

If the transaction reverts, then the next transaction will use a new access list, and so the witness will be cleared for future gas costs.

(b) Technically, it’s unnecessary to compute the exact gas cost if the available gas is already insufficient. A simpler alternative is to have the gas cost function return an error when there’s not enough gas. This error can then be propagated up the call stack, ultimately terminating EVM execution and consuming all remaining gas.

For reference, see the implementation in gasCallEIP2929: https://github.com/ethereum/go-ethereum/blob/master/core/vm/operations_acl.go#L196

In the current implementation, the required gas cost is also ambiguous. e.g. if two state entries are accessed in the operation, the real gas cost should be cost_1 + cost_2, unfortunately in some places, only cost_1 is returned. I would prefer to simply return an error.

That is indeed a very nice simplification, I am not very happy with the fact that "we don't care about the exact amount of gas being consumed". I agree that it doesn't seem to have any impact and so we don't really care since everything will be consumed. We just need to make sure that it won't create any corner case. I just want to test it very thoroughly before I give it a go.

gballet · 2025-04-11T20:09:49Z

as it were, I discovered that the execution spec tests were just skipping all the tests while saying 'passed'. Among other issues, it means that moving op -> gas broke but wasn't reported as broken and I have to run through a few hoops to see if this PR really works (I mean, if it's more broken than the base branch is)

rjl493456442 · 2025-04-12T00:46:07Z

That's not an issue: we are using this to build the witness, and the gas costs correspond to that use case. Once the witness has been paid for, the next access in the same transaction will be cheaper, we want it to be this way.

Let's say we want to access account basic data and account code hash in a single operation, and gas is only sufficient for the step 1. At that point the account basic data hasn't been resolved yet and shouldn't be included in the witness.

If the transaction reverts, then the next transaction will use a new access list, and so the witness will be cleared for future gas costs.

This can also occur in deeper call frames. For example, in opCall, several state entries may be cached in the access event before the operation reverts due to an out-of-gas error. In such cases, should we clear the cached entries? Or should we leave them as-is—knowing they can affect gas cost computation in subsequent operations within the same transaction, but from a different call frame?

gballet · 2025-04-14T07:24:46Z

That's not an issue: we are using this to build the witness, and the gas costs correspond to that use case. Once the witness has been paid for, the next access in the same transaction will be cheaper, we want it to be this way.

Let's say we want to access account basic data and account code hash in a single operation, and gas is only sufficient for the step 1. At that point the account basic data hasn't been resolved yet and shouldn't be included in the witness.

it should be, because otherwise the stateless client will not be able to read back the basic data when trying to verify the block. Now, I know you want to do everything atomic, but that's not the spec and the spec can change but it takes time and we're in agreement that the first step is to sync the verkle testnet. This is the behavior that is needed to sync the verkle testnet.

If the transaction reverts, then the next transaction will use a new access list, and so the witness will be cleared for future gas costs.

This can also occur in deeper call frames. For example, in opCall, several state entries may be cached in the access event before the operation reverts due to an out-of-gas error. In such cases, should we clear the cached entries?

no

Or should we leave them as-is—knowing they can affect gas cost computation in subsequent operations within the same transaction, but from a different call frame?

yes, we should do that. The gas costs are about paying for increasing the witness size. If an addition to the witness was paid for by a transaction, it's fair that any subsequent accesses for that transaction will not pay the full cost.

gballet · 2025-04-14T08:11:58Z

core/state/access_events.go

-func (ae *AccessEvents) ContractCreateInitGas(addr common.Address, availableGas uint64) (uint64, uint64) {
+// a contract creation. If the available gas is insufficient to cover the total
+// cost, a false flag is returned.
+func (ae *AccessEvents) ContractCreateInitGas(addr common.Address, availableGas uint64) (uint64, bool) {


This single change breaks 243 execution spec tests out of 532

move extcodesize consumption to gas method

abb0094

rjl493456442 requested a review from holiman as a code owner April 11, 2025 09:41

rjl493456442 commented Apr 11, 2025

View reviewed changes

core/vm, core/state: return error if gas is insufficient for cost

515e4b3

rjl493456442 force-pushed the kaustinen7-gas-costs branch from 95344c7 to 515e4b3 Compare April 11, 2025 10:16

gballet self-requested a review April 11, 2025 12:14

gballet reviewed Apr 11, 2025

View reviewed changes

gballet force-pushed the kaustinen7-gas-costs branch from abb0094 to 69068cd Compare April 14, 2025 07:46

gballet reviewed Apr 14, 2025

View reviewed changes

rjl493456442 force-pushed the kaustinen7-gas-costs branch from 69068cd to fcc690e Compare May 12, 2025 07:34

gballet force-pushed the kaustinen7-gas-costs branch from fcc690e to 69068cd Compare May 13, 2025 11:31

core/vm, core/state: return error if gas is insufficient for cost #540

Are you sure you want to change the base?

core/vm, core/state: return error if gas is insufficient for cost #540

Uh oh!

Conversation

rjl493456442 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gballet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gballet commented Apr 11, 2025

Uh oh!

gballet commented Apr 11, 2025

Uh oh!

rjl493456442 commented Apr 12, 2025

Uh oh!

gballet commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rjl493456442 commented Apr 11, 2025 •

edited

Loading

rjl493456442 Apr 11, 2025 •

edited

Loading