Skip to content

internal/ethapi: return code 3 from call/estimateGas even if a revert reason was not returned #31456

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

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Mar 22, 2025

Currently we only return a revertError from BlockchainAPI.(DoCall,EstimateGas) when execution reverts with non-empty revert reason.

If the revert data is empty, the RPC response is constructed with a generic errCodeDefault code here.

By altering the behavior to return a revertError from DoCall/DoEstimateGas upon revert regardless of whether there was a reason supplied, we consistently return the same revert error code whenever a revert occurs from eth_estimateGas/eth_call.

Steps to Verify this PR is Correct

Using the genesis files I have attached in this gist:

Spin up a dev-mode instance of Geth. The genesis contains an account 0x1000000000000000000000000000000000000001 which when called will revert with no data.

./build/bin/geth --datadir ./data init genesis-empty-revert.json
./build/bin/geth --dev --datadir ./data --http --http.api eth

In a separate terminal:

> curl http://localhost:8545 \
  -X POST \
  -H "Content-Type: application/json" \
  --data '{"jsonrpc":"2.0","method":"eth_call","params":[{"to": "0x1000000000000000000000000000000000000001"}],"id":1}'

which on master outputs:

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"execution reverted"}}

with this PR:

{"jsonrpc":"2.0","id":1,"error":{"code":3,"message":"execution reverted","data":"0x"}}

Now repeat the process again, except this time instantiating the datadir with genesis-nonempty-revert.json: The 0x1000000000000000000000000000000000000001 account now contains code that reverts with a single byte of revert data.

On master, and this PR:

{"jsonrpc":"2.0","id":1,"error":{"code":3,"message":"execution reverted","data":"0x00"}}

The same instructions can be repeated except with eth_call instead of eth_estimateGas and produce the same results as above.

@jwasinger
Copy link
Contributor Author

jwasinger commented Mar 22, 2025

I will add some additional test coverage with this PR if we agree that the changes here are ok.

@DenisNysheta
Copy link

l

@gballet gballet requested a review from Copilot March 23, 2025 14:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the error handling in the ethapi package so that both DoCall and DoEstimateGas consistently return a revertError when a revert occurs, regardless of whether revert data is provided.

  • Updates condition checks in Call and DoEstimateGas to use errors.Is(err, vm.ErrExecutionReverted) instead of checking the length of the revert data.
  • Ensures uniform error reporting across eth_call and eth_estimateGas methods.

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but as you said, tests are needed. Also, refer to the PR that prompted you to work on this.

@jwasinger
Copy link
Contributor Author

This PR was prompted by #31444 . I think there was some confusion with the other PR because currently we define the default rpc error code and a constant for the revert error code to have the same value of -32000.

So right now, when receiving an rpc error (of a generic type) from a revert w/o revert data, where the error contains said error code and the message "execution reverted", it could lead to the assumption that the error is a revert error type.

@fjl fjl changed the title internal/ethapi: return ethapi.revertError from DoCall/DoEstimateGas even if a revert reason was not supplied internal/ethapi: return code 3 from call/estimateGas even if a revert reason was not returned Mar 23, 2025
@fjl fjl merged commit b0b2b76 into ethereum:master Mar 23, 2025
3 of 4 checks passed
@fjl fjl added this to the 1.15.6 milestone Mar 23, 2025
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Mar 26, 2025
sivaratrisrinivas pushed a commit to sivaratrisrinivas/go-ethereum that referenced this pull request Apr 21, 2025
Rampex1 pushed a commit to streamingfast/go-ethereum that referenced this pull request May 15, 2025
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request May 22, 2025
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.

4 participants