From f2c1d6d774b0cf940345e04c8928d1c89b3e35fe Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Tue, 31 Oct 2023 12:56:13 -0700 Subject: [PATCH 1/2] [Execution] Return OutOfRange instead of Internal when account block is not cached --- engine/execution/rpc/engine.go | 16 ++++++++++------ engine/execution/rpc/engine_test.go | 23 +++++++++++++++++++++++ engine/execution/scripts/engine.go | 16 +++++++++++++--- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index ba39738812c..125fea1ee2f 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -26,6 +26,7 @@ import ( "github.com/onflow/flow-go/engine/common/rpc" "github.com/onflow/flow-go/engine/common/rpc/convert" exeEng "github.com/onflow/flow-go/engine/execution" + "github.com/onflow/flow-go/engine/execution/scripts" fvmerrors "github.com/onflow/flow-go/fvm/errors" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/state/protocol" @@ -543,13 +544,16 @@ func (h *handler) GetAccountAtBlockID( } value, err := h.engine.GetAccount(ctx, flowAddress, blockFlowID) - if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "account with address %s not found", flowAddress) - } - if fvmerrors.IsAccountNotFoundError(err) { - return nil, status.Errorf(codes.NotFound, "account not found") - } if err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "account with address %s not found", flowAddress) + } + if errors.Is(err, scripts.ErrStateCommitmentPruned) { + return nil, status.Errorf(codes.OutOfRange, "state for block ID %s not available", blockFlowID) + } + if fvmerrors.IsAccountNotFoundError(err) { + return nil, status.Errorf(codes.NotFound, "account not found") + } return nil, status.Errorf(codes.Internal, "failed to get account: %v", err) } diff --git a/engine/execution/rpc/engine_test.go b/engine/execution/rpc/engine_test.go index 3ebce486b81..6aec7283fb4 100644 --- a/engine/execution/rpc/engine_test.go +++ b/engine/execution/rpc/engine_test.go @@ -3,6 +3,7 @@ package rpc import ( "context" "errors" + "fmt" "math/rand" "testing" @@ -19,6 +20,7 @@ import ( "github.com/onflow/flow-go/engine/common/rpc/convert" mockEng "github.com/onflow/flow-go/engine/execution/mock" + "github.com/onflow/flow-go/engine/execution/scripts" "github.com/onflow/flow-go/model/flow" realstorage "github.com/onflow/flow-go/storage" storage "github.com/onflow/flow-go/storage/mock" @@ -324,6 +326,27 @@ func (suite *Suite) TestGetAccountAtBlockID() { suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) }) + + suite.Run("valid request for unavailable data", func() { + suite.commits.On("ByBlockID", id).Return(nil, nil).Once() + + expectedErr := fmt.Errorf( + "failed to execute script at block (%s): %w (%s). "+ + "this error usually happens if the reference "+ + "block for this script is not set to a recent block.", + id, + scripts.ErrStateCommitmentPruned, + unittest.IdentifierFixture(), + ) + + mockEngine.On("GetAccount", mock.Anything, serviceAddress, id).Return(nil, expectedErr).Once() + + req := createReq(id[:], serviceAddress.Bytes()) + + resp, err := handler.GetAccountAtBlockID(context.Background(), req) + suite.Assert().Nil(resp) + suite.Assert().Equal(codes.OutOfRange, status.Code(err)) + }) } // Test GetRegisterAtBlockID tests the GetRegisterAtBlockID API call diff --git a/engine/execution/scripts/engine.go b/engine/execution/scripts/engine.go index 55ba967f8a9..0f25cf409ab 100644 --- a/engine/execution/scripts/engine.go +++ b/engine/execution/scripts/engine.go @@ -15,6 +15,8 @@ import ( "github.com/onflow/flow-go/state/protocol" ) +var ErrStateCommitmentPruned = fmt.Errorf("state commitment not found") + type Engine struct { unit *engine.Unit log zerolog.Logger @@ -63,7 +65,14 @@ func (e *Engine) ExecuteScriptAtBlockID( // return early if state with the given state commitment is not in memory // and already purged. This reduces allocations for scripts targeting old blocks. if !e.execState.HasState(stateCommit) { - return nil, fmt.Errorf("failed to execute script at block (%s): state commitment not found (%s). this error usually happens if the reference block for this script is not set to a recent block", blockID.String(), hex.EncodeToString(stateCommit[:])) + return nil, fmt.Errorf( + "failed to execute script at block (%s): %w (%s). "+ + "this error usually happens if the reference "+ + "block for this script is not set to a recent block.", + blockID.String(), + ErrStateCommitmentPruned, + hex.EncodeToString(stateCommit[:]), + ) } header, err := e.state.AtBlockID(blockID).Head() @@ -117,10 +126,11 @@ func (e *Engine) GetAccount( // and already purged. This reduces allocations for get accounts targeting old blocks. if !e.execState.HasState(stateCommit) { return nil, fmt.Errorf( - "failed to get account at block (%s): state commitment not "+ - "found (%s). this error usually happens if the reference "+ + "failed to get account at block (%s): %w (%s). "+ + "this error usually happens if the reference "+ "block for this script is not set to a recent block.", blockID.String(), + ErrStateCommitmentPruned, hex.EncodeToString(stateCommit[:])) } From 7ce6920b4b6c4f31bdfe9c01cbd9709e3b4f207f Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Tue, 31 Oct 2023 13:52:17 -0700 Subject: [PATCH 2/2] Update engine/execution/rpc/engine.go Co-authored-by: Leo Zhang --- engine/execution/rpc/engine.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index 125fea1ee2f..4a0745fc3e1 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -545,12 +545,12 @@ func (h *handler) GetAccountAtBlockID( value, err := h.engine.GetAccount(ctx, flowAddress, blockFlowID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "account with address %s not found", flowAddress) - } if errors.Is(err, scripts.ErrStateCommitmentPruned) { return nil, status.Errorf(codes.OutOfRange, "state for block ID %s not available", blockFlowID) } + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "account with address %s not found", flowAddress) + } if fvmerrors.IsAccountNotFoundError(err) { return nil, status.Errorf(codes.NotFound, "account not found") }