Update eth_call code. #105

i-norden · 2021-09-29T05:29:28Z

arijitAD commented

2021-09-29 05:29:28 +00:00

(Migrated from github.com)

No description provided.

i-norden reviewed 2021-09-30 02:52:33 +00:00

i-norden left a comment

Made some comments, this eth_call stuff just keeps getting more complex huh. Especially with our proxy calling, which as an aside we may want to generally consider refactoring/cleaning up at some point if possible as in the current approach it does make the error handling even more confusing.

pkg/eth/api.go

i-norden commented

2021-09-30 02:29:41 +00:00

Echoing what I said in slack: since it is not feasible to be certain whether or not the call was reverted due to incomplete state in our Postgres ETH-IPLD database or due to the nature of the call itself (that is, that it would fail on complete state), the safest thing to do is assume it was the former and not return the revert error and instead set the err var as it is being set currently such that we then call out to the proxy client to try and satisfy the request there.

If it is the former case, the proxy client will be able to satisfy the call (assuming it has the required state) and we will potentially fill in the missing data (with the call to pea.writeStateDiffAtOrFor, although that will only fill in the missing data if the missing nodes are in the statediff at this height/hash). If it is the later case, the proxy client will run into the same revert reason and the revert error will bubble up.

Echoing what I said in slack: since it is not feasible to be certain whether or not the call was reverted due to incomplete state in our Postgres ETH-IPLD database or due to the nature of the call itself (that is, that it would fail on complete state), the safest thing to do is assume it was the former and not return the revert error and instead set the err var as it is being set currently such that we then call out to the proxy client to try and satisfy the request there. If it is the former case, the proxy client will be able to satisfy the call (assuming it has the required state) and we will potentially fill in the missing data (with the call to `pea.writeStateDiffAtOrFor`, although that will only fill in the missing data if the missing nodes are in the statediff at this height/hash). If it is the later case, the proxy client will run into the same revert reason and the revert error will bubble up.

i-norden commented

2021-09-30 02:48:51 +00:00

As far as I can see, result.Err could be non-nil due to our IPLD-ETH database being incomplete. As such, if we make it down to here with a non-nil result.Err- and haven't already tried calling out to a proxy- we should do so. Particularly since result.Return() will only ever return nil if result.Err is non-nil: f2491c5ed7/core/state_transition.go (L101).

As far as I can see, `result.Err` could be non-nil due to our IPLD-ETH database being incomplete. As such, if we make it down to here with a non-nil `result.Err`- and haven't already tried calling out to a proxy- we should do so. Particularly since `result.Return()` will only ever return nil if `result.Err` is non-nil: https://github.com/ethereum/go-ethereum/blob/f2491c5ed77b9554bd979a426db741a99acf6fa4/core/state_transition.go#L101.

pkg/eth/api.go

i-norden commented

2021-09-30 02:44:13 +00:00

This err when not nil, causing us to enter this block, is being overshadowed on the return at end of the method since we return result.Err instead. In otherwords, this err that causes us to enter this proxy call block has no way of ever bubbling up. If the err returned by the proxy CallContext call is not nil we fall through and return result.Err instead of the err that originally caused us to enter the proxy call block. This is problematic because result.Err could very well be nil in that case, so we would be returning a nil error from this method even though there was one (actually two, one that caused us to call out to the proxy- the one we want to return- and the one due to the proxy call failing- which we can ignore).

So I think when the err returned by DoCall is not nil, and we hit another non-nil err on the proxy call, we need to return the original err instead of result.Err.

This `err` when not nil, causing us to enter this block, is being overshadowed on the return at end of the method since we return `result.Err` instead. In otherwords, this `err` that causes us to enter this proxy call block has no way of ever bubbling up. If the `err` returned by the proxy `CallContext` call is not nil we fall through and return `result.Err` instead of the `err` that originally caused us to enter the proxy call block. This is problematic because `result.Err` could very well be nil in that case, so we would be returning a nil error from this method even though there was one (actually two, one that caused us to call out to the proxy- the one we want to return- and the one due to the proxy call failing- which we can ignore). So I think when the `err` returned by `DoCall` is not nil, and we hit another non-nil `err` on the proxy call, we need to return the original `err` instead of `result.Err`.

arijitAD (Migrated from github.com) reviewed 2021-09-30 11:10:55 +00:00

pkg/eth/api.go

arijitAD (Migrated from github.com) commented

2021-09-30 11:10:55 +00:00

I have updated the error logic.

i-norden reviewed 2021-09-30 12:50:39 +00:00

pkg/eth/api.go

						
				@ -902,0 +909,4 @@

				//

				// Note, this function doesn't make and changes in the state/blockchain and is

				// useful to execute and retrieve values.

				func (pea *PublicEthAPI) Call(ctx context.Context, args CallArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride) (hexutil.Bytes, error) {

i-norden commented

2021-09-30 12:48:20 +00:00

For now this should be left as err :=, if we hit an error on the proxy call we'd prefer to return the original error that caused us to call out to the proxy rather than the proxy error.

What we really should do, but we can make this a separate issue, is track all the errors in an []error and then when we return from this method we concatenate all those error messages into a single error.

For now this should be left as `err :=`, if we hit an error on the proxy call we'd prefer to return the original error that caused us to call out to the proxy rather than the proxy error. What we really should do, but we can make this a separate issue, is track all the errors in an `[]error` and then when we return from this method we concatenate all those error messages into a single error.

arijitAD (Migrated from github.com) reviewed 2021-09-30 13:45:37 +00:00

pkg/eth/api.go

						
				@ -902,0 +909,4 @@

				//

				// Note, this function doesn't make and changes in the state/blockchain and is

				// useful to execute and retrieve values.

				func (pea *PublicEthAPI) Call(ctx context.Context, args CallArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride) (hexutil.Bytes, error) {

arijitAD (Migrated from github.com) commented

2021-09-30 13:45:37 +00:00

Done
Created an issue https://github.com/vulcanize/ipld-eth-server/issues/109

Done Created an issue https://github.com/vulcanize/ipld-eth-server/issues/109

i-norden reviewed 2021-09-30 13:49:55 +00:00

pkg/eth/api.go

						
				@ -902,0 +909,4 @@

				//

				// Note, this function doesn't make and changes in the state/blockchain and is

				// useful to execute and retrieve values.

				func (pea *PublicEthAPI) Call(ctx context.Context, args CallArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride) (hexutil.Bytes, error) {

i-norden commented

2021-09-30 13:49:55 +00:00

Thanks! Sorry, I should have mentioned I already created an issue so I closed yours.

i-norden approved these changes 2021-09-30 14:40:35 +00:00

i-norden left a comment

LGTM!

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/ipld-eth-server#105