fix: handle error and revert data in EthEstimateGas and EthCall #12553

virajbhartiya · 2024-10-04T03:01:15Z

Related Issues

Closes #10311

Proposed Changes

Modified EthCall and EthEstimateGas to handle errors and revert data

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title conforms with contribution conventions
Update CHANGELOG.md or signal that this change does not need it per contribution conventions
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

virajbhartiya · 2024-10-04T03:02:59Z

@aarshkshah1992 I am using

lotus/node/impl/full/eth_utils.go

Line 333 in 0e7292a

func parseEthRevert(ret []byte) string {

to get the reverted reason for the Data field.

aarshkshah1992 · 2024-10-04T06:38:31Z

node/impl/full/eth.go

+}
+
+func (e *EthCallError) Error() string {
+	return e.Message


But how will the go-jsonrpc library extract the data here to then send it to the client ?

We're not exposing the data at all here.

aarshkshah1992 · 2024-10-04T06:43:12Z

Please see https://filecoinproject.slack.com/archives/CP50PPW2X/p1727757229039759?thread_ts=1727699414.185139&cid=CP50PPW2X

aarshkshah1992 · 2024-10-04T06:44:32Z

Best way to test this is to write an itest that deploys a contract that does a divide by 0 (so there's a revert) -> send a transaction that executes the contract -> see what error you get when you call EthCall in your itests for the block which has the execution data for that contract.

I think you'll then see what I'm talking about. The client should see the contract revert reason in the data field of the error.

BigLep · 2024-10-07T23:46:41Z

@virajbhartiya: I assume you'll re-request review when you're ready for review again.

virajbhartiya · 2024-10-08T03:46:22Z

Hey @BigLep, yes I'll request a re-review, currently I'm running into a few issues while testing out on devnet so working on fixing those.

akaladarshi · 2024-10-09T15:11:10Z

@aarshkshah1992 me and @virajbhartiya was discussing about returning execution reverted as a message and adding reason behind it as data.

The changes I have pushed are not complete, need to get your thoughts on it, then will finish the changes.

aarshkshah1992 · 2024-10-09T15:24:00Z

@akaladarshi Would be great to first get this working e2e on a calibnet node and we can then discuss the exact semantics of the error message.

Also, thanks to both for this !

akaladarshi · 2024-10-11T11:55:25Z

node

@aarshkshah1992 here is the eth_estimateGas response after the changes

Here is the response from ethereum node for same data

aarshkshah1992 · 2024-10-11T12:07:42Z

@akaladarshi @virajbhartiya Great stuff ! That looks correct. Can you also confirm that we ONLY get this data field for contract reverts and that the response for all other errors is the same as it was before ? Once you confirm that, I think we can implement this for the other API as well. Thanks a lot.

aarshkshah1992 · 2024-10-15T07:02:03Z

CHANGELOG.md

@@ -3,6 +3,17 @@
 # UNRELEASED

 ## New features
+
+* Add `EthSendRawTransactionUntrusted` RPC method to be used for the gateway when accepting `EthSendRawTransaction` and `eth_sendRawTransaction`. Applies a tighter limit on the number of messages in the queue from a single sender and applies additional restrictions on nonce increments. ([filecoin-project/lotus#12431](https://github.com/filecoin-project/lotus/pull/12431))


You probably need to rebase on master again for all of these to disappear.

aarshkshah1992 · 2024-10-15T07:03:04Z

chain/types/ethtypes/errors.go

+
+// ErrorCode returns the JSON error code for a revert.
+func (e *ExecutionRevertedError) ErrorCode() int {
+	return 3


Why is this hard coded to 3 ?

Don't we have a Code field on ExecutionRevertedError ? Shouldn't we use that ?

Yeah this (3) should be default error code for EthCall and EstimateGas. Will update it.

I mean we should pass the error code/exit code from the receipt returned by applyMessage here, right ? So when EthCall/EthEstimateGas create this reverted error, they should use the exit code returned by FVM here.

Yeah that make sense, So let me confirm once what kind of error codes FVM returns and I will update it accordingly.

aarshkshah1992 · 2024-10-15T07:04:55Z

chain/types/ethtypes/errors.go

+type ExecutionRevertedError struct {
+	Message string
+	Code    int
+	Meta    []byte


Don't see this Meta field being used anywhere. It's neither written to or read from. Should be accepting it in the constructor and setting it ?

We can remove this as we have Data field now.

aarshkshah1992 · 2024-10-15T07:05:32Z

go.mod

@@ -12,6 +12,8 @@ replace github.com/filecoin-project/test-vectors => ./extern/test-vectors // pro

 replace github.com/filecoin-project/filecoin-ffi => ./extern/filecoin-ffi // provided via a git submodule

+replace github.com/filecoin-project/go-jsonrpc => github.com/virajbhartiya/go-jsonrpc v0.0.0-20241011111701-53eab64ec154


We will need to change this once the go-jsonrpc PR lands and we have a release for it.

aarshkshah1992 · 2024-10-15T07:05:51Z

node/impl/full/eth_utils.go

@@ -31,6 +31,10 @@ import (
 	"github.com/filecoin-project/lotus/chain/vm"
 )

+const (
+	errcodeDefault = -32000


What's this for ? Why did we have to add it ?

Will remove it in favor of error code 3 for EthCall and EstimateGas, but in other places for execution reverted we need to return this(-32000) code.

@akaladarshi But then why was it not a pre-existing code ? How did we handle this before you raised this PR ?

I have to look into it little bit what exact error code FVM is returning, that's why I asked it in slack (do we have same error code as ethereum): https://filecoinproject.slack.com/archives/CP50PPW2X/p1728633105115299,

Will check this and update it accordingly.

Irrespective of what it is returning, let's not change the error codes here. We only want the data field to be in like with ETH here -> everything else should stay the same right ?

aarshkshah1992 · 2024-10-15T07:06:52Z

itests/fevm_test.go

@@ -7,14 +7,17 @@ import (
 	"encoding/binary"
 	"encoding/hex"


Why do we have such a large diff in this file ? Can we get rid of all the unnecessary whitespace changes and only change the test code we need to ? Reviewing diffs like this is tricky.

It's a linter fix, everywhere there is whitespace after //

if this is from your editor doing this automatically then it's typically good practice to disable that--it's fine to make lint fixes but large diffs like that should be done as a separate PR and not come in with a PR where the changes are unrelated, or at least make them minimal (e.g. it's OK to fix up some of these around the site where you're changing things, but usually not the whole file if you can help it.)

So generally I try to do go fmt in files after changing things that's why sometimes I get large diff, anyway, I will disable it and revert these whitespace changes so we can keep it clean.

aarshkshah1992 · 2024-10-15T07:09:06Z

node/impl/full/eth.go

-		reason := parseEthRevert(res.MsgRct.Return)
-		return nil, xerrors.Errorf("message execution failed: exit %s, revert reason: %s, vm error: %s", res.MsgRct.ExitCode, reason, res.Error)
+		return nil, ethtypes.NewExecutionRevertedWithDataError(
+			errcodeDefault,


Shouldn't we pass the exit code on the receipt here ? Why are we passing errcodeDefault here ?

@aarshkshah1992 actually there was some confusion about what error code to return because, in ethereum there two different kind of error codes for the same message (execution reverted), In EthCall and EstimateGas it returns error code 3 and generally while executing message it returns -3200.

I will the default error code to 3 for EthCall and EstimateGas which we were already returning in ErrorCode()

aarshkshah1992 · 2024-10-15T07:15:04Z

node/impl/full/eth.go

@@ -1612,6 +1623,13 @@ func ethGasSearch(
 }

 func (a *EthModule) EthCall(ctx context.Context, tx ethtypes.EthCall, blkParam ethtypes.EthBlockNumberOrHash) (ethtypes.EthBytes, error) {
+	var result ethtypes.EthBytes
+	defer func() {
+		if r := recover(); r != nil {


What is this for ? Why do we need it ?

aarshkshah1992 · 2024-10-15T07:17:07Z

node/impl/full/eth.go

 		return ethtypes.EthBytes{}, nil
 	} else if len(invokeResult.MsgRct.Return) > 0 {
-		return cbg.ReadByteArray(bytes.NewReader(invokeResult.MsgRct.Return), uint64(len(invokeResult.MsgRct.Return)))
+		result, err = cbg.ReadByteArray(bytes.NewReader(invokeResult.MsgRct.Return), uint64(len(invokeResult.MsgRct.Return)))


What is this change doing ? I don't see us returning an ExecutionRevertedError anywhere.

It was added just to add more context in the error:
return nil, xerrors.Errorf("failed to read msg recipient: %w", err)

ExecutionRevertedError will be returned by the applyMessage directly as it was already returning the execution reverted error.

aarshkshah1992 · 2024-10-15T07:17:32Z

node/impl/full/eth.go

@@ -1612,6 +1623,13 @@ func ethGasSearch(
 }

 func (a *EthModule) EthCall(ctx context.Context, tx ethtypes.EthCall, blkParam ethtypes.EthBlockNumberOrHash) (ethtypes.EthBytes, error) {


Where are we returning an ExecutionRevertedError in this function ?

@aarshkshah1992
We are returning the ExecutionRevertedError from applyMessage function directly, so we don't to do it separately.

In EstimateGas we are checking separately because of GasEstimateMessageGas function also returns an error.

https://github.com/virajbhartiya/lotus/blob/b1c146a3a2f2f9ca0a1c56a7f0ef7bf438a7db59/node/impl/full/eth.go#L1435

aarshkshah1992 · 2024-10-15T07:18:09Z

@virajbhartiya @akaladarshi

Reviewed. Thanks a lot. Also need to rebase this on master and fix the conflicts.

aarshkshah1992 · 2024-10-15T09:45:25Z

Let's address the current batch of comments for which we have solid answers to @akaladarshi.

For stuff that is still unclear/ambiguous -> we can then request a review from Stebalien 👍

akaladarshi · 2024-10-15T14:32:47Z

@aarshkshah1992 So I have removed all the hardcoded error codes, and I am directly using the FVM returned error code.

Now the only thing we need to answer is whether we should register the ExecutionRevertedError with an error code like this.

If we do then we can remove the ErrorWithCode interface from go-jsonrpc.
If not then we need to keep it in order to get the code for the ExecutionRevertedError error

aarshkshah1992 · 2024-10-15T15:16:20Z

@Stebalien Please can you take a look at just the one comment above i.e. #12553 (comment) ?

aarshkshah1992 · 2024-10-15T15:16:55Z

@akaladarshi Please can you fix the CI ? Looks like it's all red.

virajbhartiya · 2024-10-15T15:18:03Z

@aarshkshah1992 I was fixing a test, I'll check why the CI is failing

Stebalien · 2024-10-15T15:31:20Z

@Stebalien Please can you take a look at just the one comment above i.e. #12553 (comment)?

Personally, I'd:

Register the error using the existing error registration logic in this PR.
If you're up for it, make a new patch that switches to an interface-based version with ErrorWithCode. IMO, that's a much better design but it's a larger refactor. I'd rather not use both mechanisms at the same time.

aarshkshah1992 · 2024-10-15T17:39:23Z

@Stebalien

Register the error using the existing error registration logic in this PR.

Which PR ? Your comment is missing a link.

rvagg · 2024-10-16T01:24:26Z

chain/types/ethtypes/errors.go

+}
+
+// NewExecutionRevertedWithDataError returns an ExecutionRevertedError with the given code and data.
+func NewExecutionRevertedWithDataError(code int, data string) *ExecutionRevertedError {


constructor name should match struct name with a New in front

rvagg · 2024-10-16T01:28:30Z

chain/types/ethtypes/errors.go

+	"golang.org/x/xerrors"
+)
+
+var ErrExecutionReverted = xerrors.New("execution reverted")


You've made 2 error objects in this file (this one, and the struct that conforms to the interface) and the only time you use this one is for its string in this file and once in the itest. I'd just either use this string, "execution reverted" inline in the constructor, or make it a private const here. It's OK to expect ErrorContains "execution reverted" in the test without linking it to an existing const, in fact that's desirable in a test so make sure what you think is a const is actually what you want it to be.

rvagg · 2024-10-16T01:31:06Z

chain/types/ethtypes/errors.go

+	if e.Message == "" {
+		return fmt.Sprintf("json-rpc error %d", e.Code)
+	}


do we expect to ever hit this?

maybe we should do away with Message entirely and just always return this, make it ("execution reverted: %d", e.Code)

and if we don't hit this code, I wonder what the point of Code is, do we use that anywhere?

Yeah so If you look at the ErrorCode() function it returns the code, which is basically for not registered errors.

If you look at this comment above I think you will get the idea: #12553 (comment)
Whatever we decide based on it, we will either remove the code or keep it.

rvagg · 2024-10-21T03:12:59Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 - Reduce size of embedded genesis CAR files by removing WASM actor blocks and compressing with zstd. This reduces the `lotus` binary size by approximately 10 MiB. ([filecoin-project/lotus#12439](https://github.com/filecoin-project/lotus/pull/12439))
 - Add ChainSafe operated Calibration archival node to the bootstrap list ([filecoin-project/lotus#12517](https://github.com/filecoin-project/lotus/pull/12517))
 - Fix hotloop in F3 pariticpation API ([filecoin-project/lotus#12575](https://github.com/filecoin-project/lotus/pull/12575))
+- Return data with `eth_call` and `eth_estimateGas` APIs `execution reverted` error ([filecoin-project/lotus#12553](https://github.com/filecoin-project/lotus/pull/12553))


Suggested change

- Return data with `eth_call` and `eth_estimateGas` APIs `execution reverted` error ([filecoin-project/lotus#12553](https://github.com/filecoin-project/lotus/pull/12553))

- Return a `Data` field from RPC when `eth_call` and `eth_estimateGas` APIs encounter `execution reverted` errors ([filecoin-project/lotus#12553](https://github.com/filecoin-project/lotus/pull/12553))

rvagg · 2024-10-21T04:09:01Z

api/api_errors.go

+func (e *ErrExecutionRevertedWithData) Error() string { return e.message }
+
+// ErrorData returns the error data.
+func (e *ErrExecutionRevertedWithData) ErrorData() interface{} { return e.data }
+
+// NewErrExecutionRevertedWithData creates a new ErrExecutionRevertedWithData.
+func NewErrExecutionRevertedWithData(data string) *ErrExecutionRevertedWithData {
+	return &ErrExecutionRevertedWithData{


Suggested change

func (e *ErrExecutionRevertedWithData) Error() string { return e.message }

// ErrorData returns the error data.

func (e *ErrExecutionRevertedWithData) ErrorData() interface{} { return e.data }

// NewErrExecutionRevertedWithData creates a new ErrExecutionRevertedWithData.

func NewErrExecutionRevertedWithData(data string) *ErrExecutionRevertedWithData {

return &ErrExecutionRevertedWithData{

func (e ErrExecutionRevertedWithData) Error() string { return e.message }

// ErrorData returns the error data.

func (e ErrExecutionRevertedWithData) ErrorData() interface{} { return e.data }

// NewErrExecutionRevertedWithData creates a new ErrExecutionRevertedWithData.

func NewErrExecutionRevertedWithData(data string) ErrExecutionRevertedWithData {

return ErrExecutionRevertedWithData{

Does jsonrpc bork still on non-interface? Not a big deal but if we can avoid pointer receivers where they are not necessary.

@rvagg They are necessary as of now because we are registering the errors as RPCErrors.Register(EExecutionRevertedWithData, new(*ErrExecutionRevertedWithData)) } so go-jsonrpc will expect error to be returned as a pointer type for c, ok := e.byType[reflect.TypeOf(err)]; .

I am trying to figure out a way, so we can get the code without worrying if the error is pointer type or not.

aarshkshah1992 reviewed Oct 4, 2024

View reviewed changes

BigLep assigned virajbhartiya Oct 7, 2024

akaladarshi force-pushed the jsonrpc branch from 8f2817f to 103b242 Compare October 11, 2024 11:46

aarshkshah1992 reviewed Oct 15, 2024

View reviewed changes

akaladarshi mentioned this pull request Oct 15, 2024

feat: add 'Data' field to 'respError' struct filecoin-project/go-jsonrpc#123

Open

virajbhartiya and others added 5 commits October 15, 2024 18:58

fix: handle error and revert data in EthEstimateGas and EthCall

19bf92e

feat: itests

1f47508

feat: Add EthCallError struct and tests

6859ee5

refactor: eth call error

61f7fe5

fix: handle error and revert data in EthCall

619b9a5

virajbhartiya and others added 3 commits October 15, 2024 19:03

feat: Refactor EthCall to handle invalid block number

e53a74d

refactor: eth call error to execution reverted error

6d69cdd

address comments

5c7bdae

akaladarshi force-pushed the jsonrpc branch from b1c146a to 5c7bdae Compare October 15, 2024 14:17

akaladarshi added 2 commits October 15, 2024 19:50

small change

d1abde9

update changelog

22d418a

update tests

273cbcc

virajbhartiya force-pushed the jsonrpc branch from c82da3d to 273cbcc Compare October 15, 2024 16:22

remove go-jsonrpc

529dd97

rvagg reviewed Oct 16, 2024

View reviewed changes

virajbhartiya and others added 4 commits October 16, 2024 13:02

add test for ethEstimateGas and addressed changes

d8015cc

rename error

cc212fe

fix lint errors

cad6f2b

refactor: execution reverted error

7aec77b

akaladarshi force-pushed the jsonrpc branch from f85e71f to 7aec77b Compare October 16, 2024 13:18

Merge branch 'master' into jsonrpc

a092883

rvagg reviewed Oct 21, 2024

View reviewed changes

akaladarshi added 3 commits October 22, 2024 16:17

implement error codec interface

0ba5a70

fix ci tests

3206a77

update go mod

40e4ca1

		@@ -12,6 +12,8 @@ replace github.com/filecoin-project/test-vectors => ./extern/test-vectors // pro

		replace github.com/filecoin-project/filecoin-ffi => ./extern/filecoin-ffi // provided via a git submodule

		replace github.com/filecoin-project/go-jsonrpc => github.com/virajbhartiya/go-jsonrpc v0.0.0-20241011111701-53eab64ec154

		@@ -1612,6 +1623,13 @@ func ethGasSearch(
		}

		func (a *EthModule) EthCall(ctx context.Context, tx ethtypes.EthCall, blkParam ethtypes.EthBlockNumberOrHash) (ethtypes.EthBytes, error) {

	- Return data with `eth_call` and `eth_estimateGas` APIs `execution reverted` error ([filecoin-project/lotus#12553](https://github.com/filecoin-project/lotus/pull/12553))
	- Return a `Data` field from RPC when `eth_call` and `eth_estimateGas` APIs encounter `execution reverted` errors ([filecoin-project/lotus#12553](https://github.com/filecoin-project/lotus/pull/12553))

fix: handle error and revert data in EthEstimateGas and EthCall #12553

Are you sure you want to change the base?

fix: handle error and revert data in EthEstimateGas and EthCall #12553

Conversation

virajbhartiya commented Oct 4, 2024

Related Issues

Proposed Changes

Checklist

virajbhartiya commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 4, 2024

aarshkshah1992 commented Oct 4, 2024 • edited Loading

BigLep commented Oct 7, 2024

virajbhartiya commented Oct 8, 2024

akaladarshi commented Oct 9, 2024

aarshkshah1992 commented Oct 9, 2024

akaladarshi commented Oct 11, 2024

aarshkshah1992 commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 15, 2024

aarshkshah1992 commented Oct 15, 2024 • edited Loading

akaladarshi commented Oct 15, 2024

aarshkshah1992 commented Oct 15, 2024

aarshkshah1992 commented Oct 15, 2024

virajbhartiya commented Oct 15, 2024

Stebalien commented Oct 15, 2024

aarshkshah1992 commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 4, 2024 •

edited

Loading

aarshkshah1992 Oct 15, 2024 •

edited

Loading

aarshkshah1992 commented Oct 15, 2024 •

edited

Loading