Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not return gas cost along with ExceptionalHaltReason in OperationResult unless OOG #7919

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lu-pinto
Copy link
Contributor

@lu-pinto lu-pinto commented Nov 25, 2024

PR description

Rationale

While working on something unrelated I discovered that EVM halt events also require to pass in a gas cost. The meaning of this gas cost is not totally clear across all halt event types (insufficient gas, stack underflow/overflow, invalid jump destination, illegal state change, ...), so this inconsistency makes it harder for devs building new EVM features, see example on code from initial verkle gas schedule efforts. In the Ethereum Yellow paper it is mentioned that at the start of a transaction an upfront gas cost is taken captive and, then, if code is executed correctly the remaining unused gas is refunded to the account. In the event of an EVM error, halting the code execution, no gas is refunded and so the entire upfront gas made captive is consumed. Therefore, clients do not need any cost information upon halting.

Tracer cases

There is the case of tracers that have access to OperationResult when calling tracePostExecution. IMO, the only case where returning a gas cost along with the halt condition might make sense is the insufficient gas case. This seems to show exactly in which amount of gas charging did the EVM halted on. It does not make any sense at all to report a gas cost when the reason for the halt was not related to gas. In fact, I would question if it's really useful to report the gas cost at all even in the insufficient gas case because this is not specific enough (was it the static gas or a dynamic part? which type of gas exactly?). Currently the default in clients is to compound all gas costs for an opcode execution in advance of any DB lookups or computation. But one could incrementally compute the cost before any computation and check for out of gas incrementally also. I believe this will happen with stateless clients. This is also used by tracing APIs (in particular trace_* calls) but the current spec does not mention any standards around EVM halting cases. Right now I'm told that Nethermind and Reth implement these but I haven't checked how they handle this case in particular. A quick scan on Reth's website didn't show many details on this edge case either.

Changes

This PR refactors and streamlines the OperationResult, the outcome of executing an opcode, to split up between halting and successful results. It also enforces the creation of new halt OperationResults through static factory methods for better encapsulation, that always create a new instance since, on a halt event, the execution context is not recoverable so the overhead is negligible.
I'm proposing that we get rid of this gas cost reporting in OperationResult when an ExceptionalHaltReason is used, to make it all consistent. I changed some tests to cope with this modification so please review carefully. On the other hand, if someone feels strongly we could very well keep reporting gas cost upon insufficient gas.

EDIT: Latest tip allows for gasCost reporting for insufficient gas only.

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@lu-pinto lu-pinto changed the title Split operation result constructors Do not return gas cost along with ExceptionalHaltReason in OperationResult Nov 25, 2024
@lu-pinto lu-pinto force-pushed the split-operation-result-constructors branch 2 times, most recently from 2f5956f to 073356f Compare November 27, 2024 14:25
@lu-pinto lu-pinto force-pushed the split-operation-result-constructors branch from 073356f to c62c68d Compare November 27, 2024 14:51
@lu-pinto lu-pinto marked this pull request as ready for review November 28, 2024 08:12
@lu-pinto lu-pinto changed the title Do not return gas cost along with ExceptionalHaltReason in OperationResult Do not return gas cost along with ExceptionalHaltReason in OperationResult unless OOG Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant