Provide a way to get the revert reason string from eth_estimateGas

[MicahZoltu]

System information

Geth version: v1.9.10
OS & Version: Windows

Summary

Return the revert reason string when eth_estimateGas fails due to a revert.

Description

Most dapps execute an eth_estimateGas transaction prior to submitting a transaction. Along with being necessary in order to calculate gas costs for the transaction, it also is an opportunity to make sure the transaction will succeed prior to submitting it to the blockchain where failure still costs the user money (gas).

However, if the transaction fails due to a revert with a revert reason string, Geth will not return the reason string to the caller. The lack of revert reason string can lead to a frustrating user experience where there is not enough information presented to the dapp to appropriately handle or inform the user.

eth_call currently returns the reason string (though as expressed in #19027 the current mechanism is problematic). eth_estimateGas should similarly return the reason string when it is available.

Note: I recommend fixing #19027 at the same time as this. Consider following Parity's lead and returning the error in the error.data of a JSON-RPC error response.

[karalabe]

Hey there. We do think the request is completely valid, but given our past experiences with requests like this, it's super hard to go back and forth on theoretical requests like "would be nice to return the revert reason". What would really really be super helpful is if you could deploy some repro contracts on Rinkeby / Goerli and provide a list like:

• eth_estimateGas(contract1, input), should return error X
• eth_call(contract2, input) should return Y

That way it would be completely clear what the expected behavior is and it's also easy to debate what is the correct behavior in that specific instance.

[MicahZoltu]

I don't have a Rinkeby or Goerli node, nor Rinkeby/Goerli ETH, but I do have a private geth, Parity and Nethermind node that I can test against.

Given the following contract:

pragma solidity ^0.6.4;
contract Apple {
	function banana() external {
		require(false, "revert reason");
	}
}

Calling eth_estimateGas on the banana function looks like this:

{
	"jsonrpc":"2.0",
	"id":234,
	"method":"eth_estimateGas",
	"params":[
		{
			"from":"0x913da4198e6be1d5f5e4a40d0667f70c0b5430eb",
			"to":"0xaf517e20601df8d8584035eb895c02713bc1f3a4",
			"data":"0xc3cafc6f",
			"value":"0x0"
		}
	]
}

The expected response would look something like this:

{
	"jsonrpc":"2.0",
	"error":{
		"code":-32015,
		"message":"VM execution error.",
		"data":"Reverted 0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000d72657665727420726561736f6e00000000000000000000000000000000000000"
	},
	"id":126
}

The same response would be returned if eth_call was used.

This format was chosen because it matches what Parity does. It probably wouldn't by my first choice, having the Reverted prefix on the data property just makes parsing a bit more painful than necessary, but it isn't terrible and cross-client compatibility is always nice. I think the ABI encoding of the revert reason is done by Solidity when it generates those messages, and I wouldn't want the client to be messing with that.

[holiman]

We've discucssed this a bit, in depth. The problem with how the whole call-chain / evm works right now, is that we convolute evm-errors type 1 (evm could not be constructed, or evm could not properly execute because we have missing data, or the database is toast), with errors type 2 (the interpreter is fine, but the application-layer execution failed due to e.g. OOG).

So we have one error representing both "could not execute evm" and "execution fine, result was failure".
The way forward is to let evm-Calls return an ExecutionResult type, which may contain additional info (result, revert-data, evm-error). And the evm-Calls would return errors only meaning "the evm could not execute successfully".

When that is in place, it will be a lot simpler to fix this and related issues.

[MicahZoltu]

@holiman That sounds like a pretty large undertaking and breaking change, not to mention would require buy-in from other clients to be broadly useful by dapps. While I agree that the design you have described would be superior, I'm a little concerned that such a solution is probably years away and there is perhaps a shorter term solution to the immediate problem.

Do you have a sense on how high of a priority it is to take on the work you have described? If it is something that is relatively high priority and likely to happen sooner rather than later, then I would tentatively support the better solution rather than a temporary solution. If it is "something on our roadmap, but not yet at the top of the list" then I'm a bit more hesitant.

[karalabe]

What Martin suggested is purely a Geth internal refactor.

[MicahZoltu]

Ah, I misunderstood it as a change to the way you return results, not the way you internally process/handle results. Feel free to ignore the above comment in that case, since an internal refactor doesn't require nearly the amount of effort as a significant API change does!