Return Revert Data

[Confucian-e]

fix: #26823

The return value would be like this:

execution reverted: 0x96c6fd1e000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

which is error ERC20InvalidSender(address sender); in solidity

Summary (Generated by copilot):

This pull request includes a small but significant change to the error handling in the rpc/json.go file. The change enhances the error message formatting for specific conditions.

Error handling improvement:

• rpc/json.go: Modified the Error method of the jsonError struct to include additional details in the error message when the message is "execution reverted" and there is data available.

[namiloh]

The change is a bit awkward. The existing error-string method is this:

func (err *jsonError) Error() string {
	if err.Message == "" {
		return fmt.Sprintf("json-rpc error %d", err.Code)
	}
	return err.Message

The change in this PR just blindly assumes that the error was execution reverted -- couldn't it be 100 other things, e.g. connection torn down while making the request?

The idea might be good, but I think the change will have to be a bit more involved, and most likely be implemented closer to the evm, not this far out. Not sure

[Confucian-e]

The change is a bit awkward. The existing error-string method is this:

func (err *jsonError) Error() string {
	if err.Message == "" {
		return fmt.Sprintf("json-rpc error %d", err.Code)
	}
	return err.Message

The change in this PR just blindly assumes that the error was execution reverted -- couldn't it be 100 other things, e.g. connection torn down while making the request?

The idea might be good, but I think the change will have to be a bit more involved, and most likely be implemented closer to the evm, not this far out. Not sure

Sorry my mistake. I have modified the code.

[rjl493456442]

Couldn't you extract the data from the returned error?

[Confucian-e]

Couldn't you extract the data from the returned error?

Yes, for now. We can't get revert data when using EstimateGas() or CallContract() because CallContext() only return Error. So what we get is only "execution reverted".

go-ethereum/rpc/client.go

Lines 367 to 368 in a5fe735

	case resp.Error != nil:
	return resp.Error

[Confucian-e]

I think my code modification is the simplest and most effective way to solve this problem.

[rjl493456442]

Please try this fix #30659

[Confucian-e]

Please try this fix #30659

I understand the improvement in internal/ethapi/errors.go enhances error handling at the node level by including revert data in the error message. However, I believe we still need the client-side changes for better backwards compatibility.

Most users interact with node through the client package to send transactions, rather than running nodes themselves. If node operators don't upgrade their go-ethereum installations, clients won't be able to access revert data through the current client implementation. This creates inconsistency in error handling, especially when compared to other Ethereum libraries like ethers.js, which automatically concatenates "execution reverted" with revert data.

Therefore, I propose maintaining both changes:

1. The node-side improvement in internal/ethapi/errors.go for better error handling
2. The client-side change in rpc/json.go to ensure compatibility with non-upgraded nodes

This approach would provide a more consistent experience across different node versions, similar to other widely-used Ethereum libraries. Would you be open to considering both changes?

[Confucian-e]

@rjl493456442 Please review my new commit.
Changes made only to the node-side code, like yours, do not fully address our issue.

[rjl493456442]

@fjl Please take a look

[Confucian-e]

PR #30659 changed revert messages to include raw revert data after "execution reverted:", but this format doesn't clearly indicate the custom error nature of the revert.

This change improves upon PR #30659 by restructuring the error message to explicitly show that the revert was triggered by a Solidity custom error, separating the selector from its parameters for better clarity.

Before (PR #30659):

execution reverted: 0x96c6fd1e000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

After:

execution reverted: custom error 0x96c6fd1e: 000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

[holiman]

I don't see why

execution reverted: custom error 0x96c6fd1e: 000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

is preferrable over

execution reverted: 0x96c6fd1e000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

They both contain the same info, neither is human-friendly. The lower one provides the raw data, the upper one splits the raw data with : , but I don't see that as an improvement.

I might be missing something in the context here, as for why the upper one is preferrable.

[Confucian-e]

I don't see why

execution reverted: custom error 0x96c6fd1e: 000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

is preferrable over

execution reverted: 0x96c6fd1e000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

They both contain the same info, neither is human-friendly. The lower one provides the raw data, the upper one splits the raw data with : , but I don't see that as an improvement.

I might be missing something in the context here, as for why the upper one is preferrable.

Both formats are acceptable as long as they return the revert data. I believe the first one is preferable because it includes a "custom error" indication, which shows that the revert is due to a Solidity custom error. Additionally, separating the selector from the parameters allows users to retrieve the signature based on the selector and then parse the data. Of course, the second format works too, as long as it returns the revert data.

[Confucian-e]

The way Foundry's Anvil displays the information is:

execution reverted: custom error 0x96c6fd1e: 000000000000000000000000f805dfc724dcd7288b04d8ea7a5b0a9cda8f25cc

[fjl]

I think we should handle this purely on the client side. The revert error data is already returned to the client, in the error data field. So you just have to extract it from the error. Adding it to the error string will only encourage more parsing of error message strings.

Here is a PR for adding a helper function in ethclient that extracts revert data:
#30669. If necessary, we could extend this function to be able to parse error formats returned by anvil, etc.

[Confucian-e]

I think we should handle this purely on the client side. The revert error data is already returned to the client, in the error data field. So you just have to extract it from the error. Adding it to the error string will only encourage more parsing of error message strings.

Here is a PR for adding a helper function in ethclient that extracts revert data: #30669. If necessary, we could extend this function to be able to parse error formats returned by anvil, etc.

I completely agree. My initial commit was also aimed at improving the client side, as the node side already returns sufficient information. The current client side not exposing revert data makes it challenging for developers to diagnose the specific reasons for reverts. Therefore, I believe modifying the client side is the simplest and most effective solution. However, since the maintainers prioritize changes on the node side, I felt compelled to consider their feedback and made modifications accordingly to move things forward.

[Confucian-e]

By the way, I don't quite understand why you closed the PR from community developers only to open a new one to address the same issue. We identified the problem and enthusiastically submitted a PR to try to resolve it. Even if our changes weren’t perfect, maintainers could continue to refine the code based on our modifications and eventually merge our PR. Wouldn’t that greatly encourage community developers?

While opening a new PR allows you to quickly adjust the code according to your vision, the changes I proposed this time aren't urgent bugs. We spent 2 to 3 days discussing this, and I made many modifications. Why simply close my PR and open another one? I believe this isn’t the style that a good open-source team should have. As a recent graduate, I've submitted PRs to many blockchain open-source projects, continually revising them based on maintainers' suggestions until they were merged. However, I think I won’t submit any more PRs to Go Ethereum; I’ll just open issues from now on.

@fjl

[fjl]

@Confucian-e I'm sorry my PR discourages you from contributing.

When we receive PRs, it's not always clear whether the intention is mostly to fix an issue that blocks the contributor in their own project, or whether the PR is more like a gift to the project. In your case I strongly assumed the former because this issue, and a similar PR, has been raised multiple times before.

I chose to propose changes myself in the interest of time, and because I had a simple resolution in mind. I saw the PR as more of an extended issue report. Perhaps I should've spent the time outlining my proposed resolution, and let you figure out the code and details.

The issue itself is a tough one, and requires more than just some code changes in go-ethereum to resolve. The Ethereum core devs need to agree on a standard. As I tried to explain in the closing comment, an approach than encourages error message parsing is not acceptable to us, since it just adds to the proliferation of hacks across RPC client code bases.