Skip to content

Commit

Permalink
Merge pull request #559 from onflow/gregor/local-state/error-handle-fix
Browse files Browse the repository at this point in the history
Improve error handling on the local state
  • Loading branch information
sideninja authored Sep 19, 2024
2 parents 6e24eb6 + 89f8fca commit 11833eb
Show file tree
Hide file tree
Showing 7 changed files with 291 additions and 21 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ require (
github.com/hashicorp/golang-lru/v2 v2.0.7
github.com/onflow/atree v0.8.0-rc.6
github.com/onflow/cadence v1.0.0-preview.52
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357
github.com/onflow/flow-go-sdk v1.0.0-preview.56
github.com/onflow/flow/protobuf/go/flow v0.4.7
github.com/onflow/go-ethereum v1.14.7
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1865,8 +1865,8 @@ github.com/onflow/flow-ft/lib/go/contracts v1.0.0 h1:mToacZ5NWqtlWwk/7RgIl/jeKB/
github.com/onflow/flow-ft/lib/go/contracts v1.0.0/go.mod h1:PwsL8fC81cjnUnTfmyL/HOIyHnyaw/JA474Wfj2tl6A=
github.com/onflow/flow-ft/lib/go/templates v1.0.0 h1:6cMS/lUJJ17HjKBfMO/eh0GGvnpElPgBXx7h5aoWJhs=
github.com/onflow/flow-ft/lib/go/templates v1.0.0/go.mod h1:uQ8XFqmMK2jxyBSVrmyuwdWjTEb+6zGjRYotfDJ5pAE=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8 h1:5GKWWxpTc2o4EFg+SGvepkTsFeNHRYizQVeX4w05wtI=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8/go.mod h1:Gdqw1ptnAUuB0izif88PWMK8abe655Hr8iEkXXuUJl4=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357 h1:7gJ5RVKZEsUqPSKglpMXUBn+hceJ1cd/PsmLVsd5uzQ=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357/go.mod h1:Gdqw1ptnAUuB0izif88PWMK8abe655Hr8iEkXXuUJl4=
github.com/onflow/flow-go-sdk v1.0.0-M1/go.mod h1:TDW0MNuCs4SvqYRUzkbRnRmHQL1h4X8wURsCw9P9beo=
github.com/onflow/flow-go-sdk v1.0.0-preview.56 h1:ZnFznUXI1V8iZ+cKxoJRIeQwJTHItriKpnoKf8hFFso=
github.com/onflow/flow-go-sdk v1.0.0-preview.56/go.mod h1:rBRNboXaTprn7M0MeO6/R1bxNpctbrx66I2FLp0V6fM=
Expand Down
24 changes: 16 additions & 8 deletions services/requester/client_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package requester

import (
"context"
"errors"
"math/big"
"reflect"
"sync"
Expand All @@ -13,6 +14,7 @@ import (

"github.com/onflow/flow-evm-gateway/config"
"github.com/onflow/flow-evm-gateway/metrics"
errs "github.com/onflow/flow-evm-gateway/models/errors"
"github.com/onflow/flow-evm-gateway/services/state"
"github.com/onflow/flow-evm-gateway/storage"
"github.com/onflow/flow-evm-gateway/storage/pebble"
Expand Down Expand Up @@ -241,28 +243,34 @@ func handleCall[T any](
logger.Error().
Any("local", localRes).
Any("remote", remoteRes).
Msg("results from local and remote client are note the same")
Msg("results from local and remote client are not the same")
}
}

// make sure if both return an error the errors are the same
if localErr != nil && remoteErr != nil &&
localErr.Error() != remoteErr.Error() {
// if error on EN is that state is pruned or AN is rate limiting the request,
// we return the local error because it's more correct
if errors.Is(remoteErr, errs.ErrHeightOutOfRange) || errors.Is(remoteErr, errs.ErrRateLimit) {
return localRes, localErr
}

logger.Error().
Str("local", localErr.Error()).
Str("remote", remoteErr.Error()).
Msg("errors from local and remote client are note the same")
Msg("errors from local and remote client are not the same")
}

// if remote received an error but local call worked, return the local result
// this can be due to rate-limits or pruned state on AN/EN
if localErr == nil && remoteErr != nil {
logger.Warn().
Str("remote-error", remoteErr.Error()).
Any("local-result", localRes).
Msg("error from remote client but not from local client")

// todo check the error type equals to a whitelist (rate-limits, pruned state (EN)...)
if !errors.Is(remoteErr, errs.ErrHeightOutOfRange) || !errors.Is(remoteErr, errs.ErrRateLimit) {
logger.Warn().
Str("remote-error", remoteErr.Error()).
Any("local-result", localRes).
Msg("error from remote client but not from local client")
}

return localRes, nil
}
Expand Down
250 changes: 250 additions & 0 deletions services/requester/client_handler_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
package requester

import (
"bytes"
"errors"
"testing"
"time"

"github.com/rs/zerolog"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// Test data structures
type TestData struct {
Field1 int
Field2 string
}

type TestStruct struct {
Value int
}

// Helper function to check if log output contains a specific message
func containsLogMessage(logOutput, message string) bool {
return bytes.Contains([]byte(logOutput), []byte(message))
}

func TestHandleCall_BothSuccess_SameResult(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 42, nil
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 42, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
assert.Equal(t, 42, result)

logOutput := buf.String()
assert.NotContains(t, logOutput, "error")
}

func TestHandleCall_BothSuccess_DifferentResult(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 42, nil
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 43, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
assert.Equal(t, 43, result)

logOutput := buf.String()
assert.Contains(t, logOutput, "results from local and remote client are not the same")
}

func TestHandleCall_LocalSuccess_RemoteFail(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 42, nil
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 0, errors.New("remote error")
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
assert.Equal(t, 42, result)

logOutput := buf.String()
assert.Contains(t, logOutput, "error from remote client but not from local client")
}

func TestHandleCall_LocalFail_RemoteSuccess(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 0, errors.New("local error")
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 43, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
assert.Equal(t, 43, result)

logOutput := buf.String()
assert.Contains(t, logOutput, "error from local client but not from remote client")
}

func TestHandleCall_BothFail_SameError(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 0, errors.New("common error")
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 0, errors.New("common error")
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

_, err := handleCall(local, remote, logger)
require.Error(t, err)
assert.Equal(t, "common error", err.Error())

logOutput := buf.String()
assert.NotContains(t, logOutput, "errors from local and remote client are not the same")
}

func TestHandleCall_BothFail_DifferentErrors(t *testing.T) {
local := func() (int, error) {
time.Sleep(10 * time.Millisecond)
return 0, errors.New("local error")
}

remote := func() (int, error) {
time.Sleep(20 * time.Millisecond)
return 0, errors.New("remote error")
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

_, err := handleCall(local, remote, logger)
require.Error(t, err)
assert.Equal(t, "remote error", err.Error())

logOutput := buf.String()
assert.Contains(t, logOutput, "errors from local and remote client are not the same")
}

func TestHandleCall_StructType_BothSuccess_SameResult(t *testing.T) {
local := func() (TestData, error) {
time.Sleep(10 * time.Millisecond)
return TestData{Field1: 1, Field2: "test"}, nil
}

remote := func() (TestData, error) {
time.Sleep(20 * time.Millisecond)
return TestData{Field1: 1, Field2: "test"}, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
expected := TestData{Field1: 1, Field2: "test"}
assert.Equal(t, expected, result)

logOutput := buf.String()
assert.NotContains(t, logOutput, "error")
}

func TestHandleCall_StructType_BothSuccess_DifferentResult(t *testing.T) {
local := func() (TestData, error) {
time.Sleep(10 * time.Millisecond)
return TestData{Field1: 1, Field2: "test"}, nil
}

remote := func() (TestData, error) {
time.Sleep(20 * time.Millisecond)
return TestData{Field1: 2, Field2: "test"}, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
expected := TestData{Field1: 2, Field2: "test"}
assert.Equal(t, expected, result)

logOutput := buf.String()
assert.Contains(t, logOutput, "results from local and remote client are not the same")
}

func TestHandleCall_PointerType_LocalNil_RemoteNonNil(t *testing.T) {
local := func() (*TestStruct, error) {
time.Sleep(10 * time.Millisecond)
return nil, nil
}

remote := func() (*TestStruct, error) {
time.Sleep(20 * time.Millisecond)
return &TestStruct{Value: 1}, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
require.NotNil(t, result)
assert.Equal(t, 1, result.Value)

logOutput := buf.String()
assert.Contains(t, logOutput, "results from local and remote client are not the same")
}

func TestHandleCall_PointerType_LocalNonNil_RemoteNil(t *testing.T) {
local := func() (*TestStruct, error) {
time.Sleep(10 * time.Millisecond)
return &TestStruct{Value: 1}, nil
}

remote := func() (*TestStruct, error) {
time.Sleep(20 * time.Millisecond)
return nil, nil
}

var buf bytes.Buffer
logger := zerolog.New(&buf).With().Timestamp().Logger()

result, err := handleCall(local, remote, logger)
require.NoError(t, err)
require.Nil(t, result)

logOutput := buf.String()
assert.Contains(t, logOutput, "results from local and remote client are not the same")
}
26 changes: 19 additions & 7 deletions services/requester/remote_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,7 @@ func (e *RemoteClient) executeScriptAtHeight(
)
}
if err != nil {
// if snapshot doesn't exist on EN, the height at which script was executed is out
// of the boundaries the EN keeps state, so return out of range
const storageError = "failed to create storage snapshot"
if strings.Contains(err.Error(), storageError) {
return nil, errs.NewHeightOutOfRangeError(height)
}
return nil, parseError(err, height)
} else if key != "" && e.scriptCache != nil { // if error is nil and key is supported add to cache
e.scriptCache.Add(key, res)
}
Expand Down Expand Up @@ -757,7 +752,8 @@ func cadenceStringToBytes(value cadence.Value) ([]byte, error) {
return code, nil
}

// parseResult
// parseResult will check if the error code is present, which means there was an actual error during execution,
// the error is then returned as a typed error instead.
func parseResult(res cadence.Value) (*evmTypes.ResultSummary, error) {
result, err := evmImpl.ResultSummaryFromEVMResultValue(res)
if err != nil {
Expand All @@ -774,6 +770,22 @@ func parseResult(res cadence.Value) (*evmTypes.ResultSummary, error) {
return result, err
}

func parseError(err error, height uint64) error {
// if snapshot doesn't exist on EN, the height at which script was executed is out
// of the boundaries the EN keeps state, so return out of range
const storageError = "failed to create storage snapshot"
if strings.Contains(err.Error(), storageError) {
return errs.NewHeightOutOfRangeError(height)
}
// the AN rate-limited the request
const rateLimitError = "ResourceExhausted"
if strings.Contains(err.Error(), rateLimitError) {
return errs.ErrRateLimit
}

return err
}

// cacheKey builds the cache key from the script type, height and arguments.
func cacheKey(scriptType scriptType, height uint64, args []cadence.Value) string {
key := fmt.Sprintf("%d%d", scriptType, height)
Expand Down
2 changes: 1 addition & 1 deletion tests/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/onflow/crypto v0.25.2
github.com/onflow/flow-emulator v1.0.0-preview.42
github.com/onflow/flow-evm-gateway v0.0.0-20240201154855-4d4d3d3f19c7
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357
github.com/onflow/flow-go-sdk v1.0.0-preview.56
github.com/onflow/go-ethereum v1.14.7
github.com/rs/zerolog v1.31.0
Expand Down
4 changes: 2 additions & 2 deletions tests/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -2093,8 +2093,8 @@ github.com/onflow/flow-ft/lib/go/contracts v1.0.0 h1:mToacZ5NWqtlWwk/7RgIl/jeKB/
github.com/onflow/flow-ft/lib/go/contracts v1.0.0/go.mod h1:PwsL8fC81cjnUnTfmyL/HOIyHnyaw/JA474Wfj2tl6A=
github.com/onflow/flow-ft/lib/go/templates v1.0.0 h1:6cMS/lUJJ17HjKBfMO/eh0GGvnpElPgBXx7h5aoWJhs=
github.com/onflow/flow-ft/lib/go/templates v1.0.0/go.mod h1:uQ8XFqmMK2jxyBSVrmyuwdWjTEb+6zGjRYotfDJ5pAE=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8 h1:5GKWWxpTc2o4EFg+SGvepkTsFeNHRYizQVeX4w05wtI=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240918123637-27d2c56494f8/go.mod h1:Gdqw1ptnAUuB0izif88PWMK8abe655Hr8iEkXXuUJl4=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357 h1:7gJ5RVKZEsUqPSKglpMXUBn+hceJ1cd/PsmLVsd5uzQ=
github.com/onflow/flow-go v0.37.10-util-ensure-checkpoint-exists.0.20240914104351-c2d9833c3357/go.mod h1:Gdqw1ptnAUuB0izif88PWMK8abe655Hr8iEkXXuUJl4=
github.com/onflow/flow-go-sdk v1.0.0-M1/go.mod h1:TDW0MNuCs4SvqYRUzkbRnRmHQL1h4X8wURsCw9P9beo=
github.com/onflow/flow-go-sdk v1.0.0-preview.56 h1:ZnFznUXI1V8iZ+cKxoJRIeQwJTHItriKpnoKf8hFFso=
github.com/onflow/flow-go-sdk v1.0.0-preview.56/go.mod h1:rBRNboXaTprn7M0MeO6/R1bxNpctbrx66I2FLp0V6fM=
Expand Down

0 comments on commit 11833eb

Please sign in to comment.