From 36cda908b66f6ce9d285921e965b468b017bf15f Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Thu, 15 Jun 2023 11:45:21 -0700 Subject: [PATCH 1/5] [Execution] Check if block exists locally in RPC endpoints --- engine/execution/rpc/engine.go | 50 +++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index 08de8c5919c..00b232e221f 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -27,6 +27,8 @@ import ( "github.com/onflow/flow-go/storage" ) +const DefaultMaxBlockRange = 250 + // Config defines the configurable options for the gRPC server. type Config struct { ListenAddr string @@ -98,6 +100,7 @@ func New( transactionResults: txResults, commits: commits, log: log, + maxBlockRange: DefaultMaxBlockRange, }, server: server, config: config, @@ -157,6 +160,7 @@ type handler struct { transactionResults storage.TransactionResults log zerolog.Logger commits storage.Commits + maxBlockRange int } var _ execution.ExecutionAPIServer = &handler{} @@ -176,6 +180,14 @@ func (h *handler) ExecuteScriptAtBlockID( return nil, err } + // return a more user friendly error if block does not exist + if _, err = h.headers.ByBlockID(blockID); err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) + } + return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + } + value, err := h.engine.ExecuteScriptAtBlockID(ctx, req.GetScript(), req.GetArguments(), blockID) if err != nil { // return code 3 as this passes the litmus test in our context @@ -229,6 +241,22 @@ func (h *handler) GetEventsForBlockIDs(_ context.Context, return nil, err } + if len(blockIDs) > h.maxBlockRange { + return nil, status.Errorf(codes.InvalidArgument, "too many block IDs requested") + } + + // make sure all blocks exist first to fail fast and avoid unused lookups + // must verify block exists first, since ByBlockID* calls on sets like events or commits will + // return an empty slice if block does not exist + for _, blockID := range flowBlockIDs { + if _, err = h.headers.ByBlockID(blockID); err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) + } + return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + } + } + results := make([]*execution.GetEventsForBlockIDsResponse_Result, len(blockIDs)) // collect all the events and create a EventsResponse_Result for each block @@ -394,6 +422,15 @@ func (h *handler) GetTransactionResultsByBlockID( return nil, status.Errorf(codes.InvalidArgument, "invalid blockID: %v", err) } + // must verify block exists first, since transactionResults.ByBlockID will return an empty slice + // if block does not exist + if _, err = h.headers.ByBlockID(blockID); err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) + } + return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + } + // Get all tx results txResults, err := h.transactionResults.ByBlockID(blockID) if err != nil { @@ -496,6 +533,14 @@ func (h *handler) GetAccountAtBlockID( return nil, status.Errorf(codes.InvalidArgument, "invalid address: %v", err) } + // return a more user friendly error if block does not exist + if _, err = h.headers.ByBlockID(blockFlowID); err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "block %s not found", blockFlowID) + } + return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + } + 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) @@ -540,7 +585,10 @@ func (h *handler) GetLatestBlockHeader( header, err = h.state.Final().Head() } if err != nil { - return nil, status.Errorf(codes.NotFound, "not found: %v", err) + // this header MUST exist in the db, otherwise the node likely has inconsistent state. + // Don't crash as a result of an external API request, but other components will likely panic. + h.log.Err(err).Msg("unable to get latest block header") + return nil, status.Errorf(codes.Internal, "unable to get latest header: %v", err) } return h.blockHeaderResponse(header) From 2dcd5dc6a605a3f06e0d8247c86e81f5a45bbe20 Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Thu, 15 Jun 2023 16:26:54 -0700 Subject: [PATCH 2/5] Update to use commits instead of headers --- engine/access/rpc/backend/backend_accounts.go | 5 +- engine/access/rpc/backend/backend_events.go | 2 +- .../rpc/backend/backend_transactions.go | 9 --- engine/execution/rpc/engine.go | 61 +++++++++---------- 4 files changed, 32 insertions(+), 45 deletions(-) diff --git a/engine/access/rpc/backend/backend_accounts.go b/engine/access/rpc/backend/backend_accounts.go index a3a41053c61..3684c92f5d0 100644 --- a/engine/access/rpc/backend/backend_accounts.go +++ b/engine/access/rpc/backend/backend_accounts.go @@ -109,11 +109,10 @@ func (b *backendAccounts) getAccountAtBlockID( func (b *backendAccounts) getAccountFromAnyExeNode(ctx context.Context, execNodes flow.IdentityList, req *execproto.GetAccountAtBlockIDRequest) (*execproto.GetAccountAtBlockIDResponse, error) { var errors *multierror.Error for _, execNode := range execNodes { - // TODO: use the GRPC Client interceptor start := time.Now() - resp, err := b.tryGetAccount(ctx, execNode, req) duration := time.Since(start) + if err == nil { // return if any execution node replied successfully b.log.Debug(). @@ -124,6 +123,7 @@ func (b *backendAccounts) getAccountFromAnyExeNode(ctx context.Context, execNode Msg("Successfully got account info") return resp, nil } + b.log.Error(). Str("execution_node", execNode.String()). Hex("block_id", req.GetBlockId()). @@ -131,6 +131,7 @@ func (b *backendAccounts) getAccountFromAnyExeNode(ctx context.Context, execNode Int64("rtt_ms", duration.Milliseconds()). Err(err). Msg("failed to execute GetAccount") + errors = multierror.Append(errors, err) } diff --git a/engine/access/rpc/backend/backend_events.go b/engine/access/rpc/backend/backend_events.go index f48ba395947..981d1456d54 100644 --- a/engine/access/rpc/backend/backend_events.go +++ b/engine/access/rpc/backend/backend_events.go @@ -187,7 +187,7 @@ func verifyAndConvertToAccessEvents( events, err := convert.MessagesToEventsFromVersion(result.GetEvents(), version) if err != nil { - return nil, fmt.Errorf("failed to unmarshall events in event %d with encoding version %s: %w", + return nil, fmt.Errorf("failed to unmarshal events in event %d with encoding version %s: %w", i, version.String(), err) } diff --git a/engine/access/rpc/backend/backend_transactions.go b/engine/access/rpc/backend/backend_transactions.go index 5e955d78c5c..026e162357a 100644 --- a/engine/access/rpc/backend/backend_transactions.go +++ b/engine/access/rpc/backend/backend_transactions.go @@ -797,9 +797,6 @@ func (b *backendTransactions) getTransactionResultFromAnyExeNode( Msg("Successfully got transaction results from any node") return resp, nil } - if status.Code(err) == codes.NotFound { - return nil, err - } errs = multierror.Append(errs, err) } @@ -856,9 +853,6 @@ func (b *backendTransactions) getTransactionResultsByBlockIDFromAnyExeNode( Msg("Successfully got transaction results from any node") return resp, nil } - if status.Code(err) == codes.NotFound { - return nil, err - } errs = multierror.Append(errs, err) } @@ -916,9 +910,6 @@ func (b *backendTransactions) getTransactionResultByIndexFromAnyExeNode( Msg("Successfully got transaction results from any node") return resp, nil } - if status.Code(err) == codes.NotFound { - return nil, err - } errs = multierror.Append(errs, err) } diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index 00b232e221f..3cb49a6ca8c 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -27,7 +27,7 @@ import ( "github.com/onflow/flow-go/storage" ) -const DefaultMaxBlockRange = 250 +const DefaultMaxBlockRange = 300 // Config defines the configurable options for the gRPC server. type Config struct { @@ -166,7 +166,10 @@ type handler struct { var _ execution.ExecutionAPIServer = &handler{} // Ping responds to requests when the server is up. -func (h *handler) Ping(_ context.Context, _ *execution.PingRequest) (*execution.PingResponse, error) { +func (h *handler) Ping( + _ context.Context, + _ *execution.PingRequest, +) (*execution.PingResponse, error) { return &execution.PingResponse{}, nil } @@ -180,12 +183,12 @@ func (h *handler) ExecuteScriptAtBlockID( return nil, err } - // return a more user friendly error if block does not exist - if _, err = h.headers.ByBlockID(blockID); err != nil { + // return a more user friendly error if block has not been executed + if _, err = h.commits.ByBlockID(blockID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockID) } - return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockID) } value, err := h.engine.ExecuteScriptAtBlockID(ctx, req.GetScript(), req.GetArguments(), blockID) @@ -226,8 +229,10 @@ func (h *handler) GetRegisterAtBlockID( return res, nil } -func (h *handler) GetEventsForBlockIDs(_ context.Context, - req *execution.GetEventsForBlockIDsRequest) (*execution.GetEventsForBlockIDsResponse, error) { +func (h *handler) GetEventsForBlockIDs( + _ context.Context, + req *execution.GetEventsForBlockIDsRequest, +) (*execution.GetEventsForBlockIDsResponse, error) { // validate request blockIDs := req.GetBlockIds() @@ -245,18 +250,6 @@ func (h *handler) GetEventsForBlockIDs(_ context.Context, return nil, status.Errorf(codes.InvalidArgument, "too many block IDs requested") } - // make sure all blocks exist first to fail fast and avoid unused lookups - // must verify block exists first, since ByBlockID* calls on sets like events or commits will - // return an empty slice if block does not exist - for _, blockID := range flowBlockIDs { - if _, err = h.headers.ByBlockID(blockID); err != nil { - if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) - } - return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) - } - } - results := make([]*execution.GetEventsForBlockIDsResponse_Result, len(blockIDs)) // collect all the events and create a EventsResponse_Result for each block @@ -264,7 +257,7 @@ func (h *handler) GetEventsForBlockIDs(_ context.Context, // Check if block has been executed if _, err := h.commits.ByBlockID(bID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "state commitment for block ID %s does not exist", bID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", bID) } return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", bID) } @@ -422,13 +415,13 @@ func (h *handler) GetTransactionResultsByBlockID( return nil, status.Errorf(codes.InvalidArgument, "invalid blockID: %v", err) } - // must verify block exists first, since transactionResults.ByBlockID will return an empty slice - // if block does not exist - if _, err = h.headers.ByBlockID(blockID); err != nil { + // must verify block was locally executed first since transactionResults.ByBlockID will return + // an empty slice if block does not exist + if _, err = h.commits.ByBlockID(blockID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s not found", blockID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockID) } - return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockID) } // Get all tx results @@ -498,8 +491,10 @@ func (h *handler) GetTransactionResultsByBlockID( } // eventResult creates EventsResponse_Result from flow.Event for the given blockID -func (h *handler) eventResult(blockID flow.Identifier, - flowEvents []flow.Event) (*execution.GetEventsForBlockIDsResponse_Result, error) { +func (h *handler) eventResult( + blockID flow.Identifier, + flowEvents []flow.Event, +) (*execution.GetEventsForBlockIDsResponse_Result, error) { // convert events to event message events := convert.EventsToMessages(flowEvents) @@ -533,12 +528,12 @@ func (h *handler) GetAccountAtBlockID( return nil, status.Errorf(codes.InvalidArgument, "invalid address: %v", err) } - // return a more user friendly error if block does not exist - if _, err = h.headers.ByBlockID(blockFlowID); err != nil { + // return a more user friendly error if block has not been executed + if _, err = h.commits.ByBlockID(blockFlowID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s not found", blockFlowID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockFlowID) } - return nil, status.Errorf(codes.Internal, "failed to get header for block: %v", err) + return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockFlowID) } value, err := h.engine.GetAccount(ctx, flowAddress, blockFlowID) @@ -587,7 +582,7 @@ func (h *handler) GetLatestBlockHeader( if err != nil { // this header MUST exist in the db, otherwise the node likely has inconsistent state. // Don't crash as a result of an external API request, but other components will likely panic. - h.log.Err(err).Msg("unable to get latest block header") + h.log.Err(err).Msg("failed to get latest block header. potentially inconsistent protocol state.") return nil, status.Errorf(codes.Internal, "unable to get latest header: %v", err) } From 581b085a70b5981df149fc87636da676bb3b5b97 Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Thu, 10 Aug 2023 15:22:13 -0700 Subject: [PATCH 3/5] make missing header error more specific --- engine/execution/rpc/engine.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index 3cb49a6ca8c..a69caaaa6d8 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -186,7 +186,7 @@ func (h *handler) ExecuteScriptAtBlockID( // return a more user friendly error if block has not been executed if _, err = h.commits.ByBlockID(blockID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node or was pruned", blockID) } return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockID) } @@ -257,7 +257,7 @@ func (h *handler) GetEventsForBlockIDs( // Check if block has been executed if _, err := h.commits.ByBlockID(bID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", bID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node or was pruned", bID) } return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", bID) } @@ -419,7 +419,7 @@ func (h *handler) GetTransactionResultsByBlockID( // an empty slice if block does not exist if _, err = h.commits.ByBlockID(blockID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node or was pruned", blockID) } return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockID) } @@ -531,7 +531,7 @@ func (h *handler) GetAccountAtBlockID( // return a more user friendly error if block has not been executed if _, err = h.commits.ByBlockID(blockFlowID); err != nil { if errors.Is(err, storage.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node", blockFlowID) + return nil, status.Errorf(codes.NotFound, "block %s has not been executed by node or was pruned", blockFlowID) } return nil, status.Errorf(codes.Internal, "state commitment for block ID %s could not be retrieved", blockFlowID) } From 16855ede00508569007f1180bd78e93acd90f0ea Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Fri, 11 Aug 2023 16:37:50 -0700 Subject: [PATCH 4/5] fix unittests --- engine/access/rpc/backend/backend_test.go | 4 ++-- engine/access/rpc/backend/retry_test.go | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/engine/access/rpc/backend/backend_test.go b/engine/access/rpc/backend/backend_test.go index 9c87d88a4e4..544319dc937 100644 --- a/engine/access/rpc/backend/backend_test.go +++ b/engine/access/rpc/backend/backend_test.go @@ -856,7 +856,7 @@ func (suite *Suite) TestTransactionStatusTransition() { suite.execClient. On("GetTransactionResult", ctx, exeEventReq). Return(exeEventResp, status.Errorf(codes.NotFound, "not found")). - Once() + Times(len(fixedENIDs)) // should call each EN once // first call - when block under test is greater height than the sealed head, but execution node does not know about Tx result, err := backend.GetTransactionResult(ctx, txID, flow.ZeroID, flow.ZeroID) @@ -1114,7 +1114,7 @@ func (suite *Suite) TestTransactionPendingToFinalizedStatusTransition() { suite.execClient. On("GetTransactionResult", ctx, exeEventReq). Return(exeEventResp, status.Errorf(codes.NotFound, "not found")). - Once() + Times(len(enIDs)) // should call each EN once // create a mock connection factory connFactory := suite.setupConnectionFactory() diff --git a/engine/access/rpc/backend/retry_test.go b/engine/access/rpc/backend/retry_test.go index 07bb77aa8a9..ede8d5b9332 100644 --- a/engine/access/rpc/backend/retry_test.go +++ b/engine/access/rpc/backend/retry_test.go @@ -163,7 +163,10 @@ func (suite *Suite) TestSuccessfulTransactionsDontRetry() { suite.colClient.On("SendTransaction", mock.Anything, mock.Anything).Return(&access.SendTransactionResponse{}, nil) // return not found to return finalized status - suite.execClient.On("GetTransactionResult", ctx, &exeEventReq).Return(&exeEventResp, status.Errorf(codes.NotFound, "not found")).Once() + suite.execClient.On("GetTransactionResult", ctx, &exeEventReq). + Return(&exeEventResp, status.Errorf(codes.NotFound, "not found")). + Times(len(enIDs)) // should call each EN once + // first call - when block under test is greater height than the sealed head, but execution node does not know about Tx result, err := backend.GetTransactionResult(ctx, txID, flow.ZeroID, flow.ZeroID) suite.checkResponse(result, err) From e5f33b227ecdcfb2efe6d8c122342b9d653f37d0 Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Mon, 28 Aug 2023 14:32:28 -0700 Subject: [PATCH 5/5] fix and cleanup en rpc engine tests --- engine/execution/rpc/engine.go | 2 +- engine/execution/rpc/engine_test.go | 199 ++++++++++++---------------- 2 files changed, 83 insertions(+), 118 deletions(-) diff --git a/engine/execution/rpc/engine.go b/engine/execution/rpc/engine.go index a69caaaa6d8..5a6a5b98892 100644 --- a/engine/execution/rpc/engine.go +++ b/engine/execution/rpc/engine.go @@ -247,7 +247,7 @@ func (h *handler) GetEventsForBlockIDs( } if len(blockIDs) > h.maxBlockRange { - return nil, status.Errorf(codes.InvalidArgument, "too many block IDs requested") + return nil, status.Errorf(codes.InvalidArgument, "too many block IDs requested: %d > %d", len(blockIDs), h.maxBlockRange) } results := make([]*execution.GetEventsForBlockIDsResponse_Result, len(blockIDs)) diff --git a/engine/execution/rpc/engine_test.go b/engine/execution/rpc/engine_test.go index ed85f6043e3..3ebce486b81 100644 --- a/engine/execution/rpc/engine_test.go +++ b/engine/execution/rpc/engine_test.go @@ -41,20 +41,21 @@ func TestHandler(t *testing.T) { func (suite *Suite) SetupTest() { suite.log = zerolog.Logger{} - suite.events = new(storage.Events) - suite.exeResults = new(storage.ExecutionResults) - suite.txResults = new(storage.TransactionResults) - suite.commits = new(storage.Commits) - suite.headers = new(storage.Headers) + suite.events = storage.NewEvents(suite.T()) + suite.exeResults = storage.NewExecutionResults(suite.T()) + suite.txResults = storage.NewTransactionResults(suite.T()) + suite.commits = storage.NewCommits(suite.T()) + suite.headers = storage.NewHeaders(suite.T()) } // TestExecuteScriptAtBlockID tests the ExecuteScriptAtBlockID API call func (suite *Suite) TestExecuteScriptAtBlockID() { // setup handler - mockEngine := new(mockEng.ScriptExecutor) + mockEngine := mockEng.NewScriptExecutor(suite.T()) handler := &handler{ - engine: mockEngine, - chain: flow.Mainnet, + engine: mockEngine, + chain: flow.Mainnet, + commits: suite.commits, } // setup dummy request/response @@ -72,6 +73,7 @@ func (suite *Suite) TestExecuteScriptAtBlockID() { } suite.Run("happy path with successful script execution", func() { + suite.commits.On("ByBlockID", mockIdentifier).Return(nil, nil).Once() mockEngine.On("ExecuteScriptAtBlockID", ctx, script, arguments, mockIdentifier). Return(scriptExecValue, nil).Once() response, err := handler.ExecuteScriptAtBlockID(ctx, &executionReq) @@ -80,7 +82,15 @@ func (suite *Suite) TestExecuteScriptAtBlockID() { mockEngine.AssertExpectations(suite.T()) }) + suite.Run("valid request for unknown block", func() { + suite.commits.On("ByBlockID", mockIdentifier).Return(nil, realstorage.ErrNotFound).Once() + _, err := handler.ExecuteScriptAtBlockID(ctx, &executionReq) + suite.Require().Error(err) + errors.Is(err, status.Error(codes.NotFound, "")) + }) + suite.Run("valid request with script execution failure", func() { + suite.commits.On("ByBlockID", mockIdentifier).Return(nil, nil).Once() mockEngine.On("ExecuteScriptAtBlockID", ctx, script, arguments, mockIdentifier). Return(nil, status.Error(codes.InvalidArgument, "")).Once() _, err := handler.ExecuteScriptAtBlockID(ctx, &executionReq) @@ -125,7 +135,6 @@ func (suite *Suite) TestGetEventsForBlockIDs() { eventMessages[j] = convert.EventToMessage(e) } // expect one call to lookup result for each block ID - //suite.exeResults.On("ByBlockID", id).Return(nil, nil).Once() suite.commits.On("ByBlockID", id).Return(nil, nil).Once() // expect one call to lookup events for each block ID @@ -150,6 +159,7 @@ func (suite *Suite) TestGetEventsForBlockIDs() { transactionResults: suite.txResults, commits: suite.commits, chain: flow.Mainnet, + maxBlockRange: DefaultMaxBlockRange, } concoctReq := func(errType string, blockIDs [][]byte) *execution.GetEventsForBlockIDsRequest { @@ -173,9 +183,6 @@ func (suite *Suite) TestGetEventsForBlockIDs() { actualResult := resp.GetResults() suite.Require().ElementsMatch(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) }) // failure path - empty even type in the request results in an error @@ -189,9 +196,6 @@ func (suite *Suite) TestGetEventsForBlockIDs() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) // failure path - empty block ids in request results in an error @@ -205,9 +209,6 @@ func (suite *Suite) TestGetEventsForBlockIDs() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) // failure path - non-existent block id in request results in an error @@ -226,9 +227,23 @@ func (suite *Suite) TestGetEventsForBlockIDs() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.NotFound, "")) + }) - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) + // request for too many blocks - receives a InvalidArgument error + suite.Run("request for too many blocks", func() { + + // update range so it's smaller than list of blockIDs + handler.maxBlockRange = totalBlocks / 2 + + // create a valid API request + req := concoctReq(string(flow.EventAccountCreated), blockIDs) + + // execute the GetEventsForBlockIDs call + _, err := handler.GetEventsForBlockIDs(context.Background(), req) + + // check that an error was received + suite.Require().Error(err) + errors.Is(err, status.Error(codes.InvalidArgument, "")) }) } @@ -242,12 +257,13 @@ func (suite *Suite) TestGetAccountAtBlockID() { Address: serviceAddress, } - mockEngine := new(mockEng.ScriptExecutor) + mockEngine := mockEng.NewScriptExecutor(suite.T()) // create the handler handler := &handler{ - engine: mockEngine, - chain: flow.Mainnet, + engine: mockEngine, + chain: flow.Mainnet, + commits: suite.commits, } createReq := func(id []byte, address []byte) *execution.GetAccountAtBlockIDRequest { @@ -260,6 +276,8 @@ func (suite *Suite) TestGetAccountAtBlockID() { suite.Run("happy path with valid request", func() { // setup mock expectations + suite.commits.On("ByBlockID", id).Return(nil, nil).Once() + mockEngine.On("GetAccount", mock.Anything, serviceAddress, id).Return(&serviceAccount, nil).Once() req := createReq(id[:], serviceAddress.Bytes()) @@ -272,7 +290,19 @@ func (suite *Suite) TestGetAccountAtBlockID() { suite.Require().NoError(err) suite.Require().Empty( cmp.Diff(expectedAccount, actualAccount, protocmp.Transform())) - mockEngine.AssertExpectations(suite.T()) + }) + + suite.Run("invalid request with unknown block id", func() { + + // setup mock expectations + suite.commits.On("ByBlockID", id).Return(nil, realstorage.ErrNotFound).Once() + + req := createReq(id[:], serviceAddress.Bytes()) + + _, err := handler.GetAccountAtBlockID(context.Background(), req) + + suite.Require().Error(err) + errors.Is(err, status.Error(codes.NotFound, "")) }) suite.Run("invalid request with nil block id", func() { @@ -282,6 +312,7 @@ func (suite *Suite) TestGetAccountAtBlockID() { _, err := handler.GetAccountAtBlockID(context.Background(), req) suite.Require().Error(err) + errors.Is(err, status.Error(codes.InvalidArgument, "")) }) suite.Run("invalid request with nil root address", func() { @@ -291,6 +322,7 @@ func (suite *Suite) TestGetAccountAtBlockID() { _, err := handler.GetAccountAtBlockID(context.Background(), req) suite.Require().Error(err) + errors.Is(err, status.Error(codes.InvalidArgument, "")) }) } @@ -301,7 +333,7 @@ func (suite *Suite) TestGetRegisterAtBlockID() { serviceAddress := flow.Mainnet.Chain().ServiceAddress() validKey := []byte("exists") - mockEngine := new(mockEng.ScriptExecutor) + mockEngine := mockEng.NewScriptExecutor(suite.T()) // create the handler handler := &handler{ @@ -329,7 +361,6 @@ func (suite *Suite) TestGetRegisterAtBlockID() { value := resp.GetValue() suite.Require().NoError(err) suite.Require().True(len(value) > 0) - mockEngine.AssertExpectations(suite.T()) }) suite.Run("invalid request with bad address", func() { @@ -365,15 +396,13 @@ func (suite *Suite) TestGetTransactionResult() { // expect a call to lookup events by block ID and transaction ID suite.events.On("ByBlockIDTransactionID", bID, txID).Return(eventsForTx, nil) - // expect a call to lookup each block - suite.headers.On("ByID", block.ID()).Return(&block, true) - // create the handler createHandler := func(txResults *storage.TransactionResults) *handler { handler := &handler{ headers: suite.headers, events: suite.events, transactionResults: txResults, + commits: suite.commits, chain: flow.Mainnet, } return handler @@ -412,7 +441,7 @@ func (suite *Suite) TestGetTransactionResult() { } // expect a call to lookup transaction result by block ID and transaction ID, return a result with no error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResult := flow.TransactionResult{ TransactionID: flow.Identifier{}, ErrorMessage: "", @@ -432,10 +461,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that all fields in response are as expected assertEqual(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResults.AssertExpectations(suite.T()) }) // happy path - valid requests receives all events for the given transaction by index @@ -449,7 +474,7 @@ func (suite *Suite) TestGetTransactionResult() { } // expect a call to lookup transaction result by block ID and transaction ID, return a result with no error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResult := flow.TransactionResult{ TransactionID: flow.Identifier{}, ErrorMessage: "", @@ -472,10 +497,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that all fields in response are as expected assertEqual(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResults.AssertExpectations(suite.T()) }) // happy path - valid requests receives all events and an error for the given transaction @@ -489,7 +510,7 @@ func (suite *Suite) TestGetTransactionResult() { } // setup the storage to return a transaction error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResult := flow.TransactionResult{ TransactionID: txID, ErrorMessage: "runtime error", @@ -509,10 +530,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that all fields in response are as expected assertEqual(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResults.AssertExpectations(suite.T()) }) // happy path - valid requests receives all events and an error for the given transaction @@ -526,7 +543,7 @@ func (suite *Suite) TestGetTransactionResult() { } // setup the storage to return a transaction error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResult := flow.TransactionResult{ TransactionID: txID, ErrorMessage: "runtime error", @@ -551,8 +568,6 @@ func (suite *Suite) TestGetTransactionResult() { assertEqual(expectedResult, actualResult) // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResults.AssertExpectations(suite.T()) }) // failure path - nil transaction ID in the request results in an error @@ -561,11 +576,7 @@ func (suite *Suite) TestGetTransactionResult() { // create an API request with transaction ID as nil req := concoctReq(bID[:], nil) - // expect a call to lookup transaction result by block ID and transaction ID, return an error - txResults := new(storage.TransactionResults) - - txResults.On("ByBlockIDTransactionID", bID, nil).Return(nil, status.Error(codes.InvalidArgument, "")).Once() - + txResults := storage.NewTransactionResults(suite.T()) handler := createHandler(txResults) _, err := handler.GetTransactionResult(context.Background(), req) @@ -573,9 +584,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) // failure path - nil block id in the request results in an error @@ -584,10 +592,7 @@ func (suite *Suite) TestGetTransactionResult() { // create an API request with a nil block id req := concoctReq(nil, txID[:]) - txResults := new(storage.TransactionResults) - - txResults.On("ByBlockIDTransactionID", nil, txID).Return(nil, status.Error(codes.InvalidArgument, "")).Once() - + txResults := storage.NewTransactionResults(suite.T()) handler := createHandler(txResults) _, err := handler.GetTransactionResult(context.Background(), req) @@ -595,9 +600,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) // failure path - nil block id in the index request results in an error @@ -606,10 +608,7 @@ func (suite *Suite) TestGetTransactionResult() { // create an API request with a nil block id req := concoctIndexReq(nil, txIndex) - txResults := new(storage.TransactionResults) - - txResults.On("ByBlockIDTransactionIndex", nil, txID).Return(nil, status.Error(codes.InvalidArgument, "")).Once() - + txResults := storage.NewTransactionResults(suite.T()) handler := createHandler(txResults) _, err := handler.GetTransactionResultByIndex(context.Background(), req) @@ -617,9 +616,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) // failure path - non-existent transaction ID in request results in an error @@ -631,7 +627,7 @@ func (suite *Suite) TestGetTransactionResult() { req := concoctReq(bID[:], wrongTxID[:]) // expect a storage call for the invalid tx ID but return an error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResults.On("ByBlockIDTransactionID", bID, wrongTxID).Return(nil, status.Error(codes.Internal, "")).Once() handler := createHandler(txResults) @@ -641,9 +637,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.Internal, "")) - - // check that one storage call was made - suite.events.AssertExpectations(suite.T()) }) // failure path - non-existent transaction index in request results in an error @@ -655,7 +648,7 @@ func (suite *Suite) TestGetTransactionResult() { req := concoctIndexReq(bID[:], wrongTxIndex) // expect a storage call for the invalid tx ID but return an error - txResults := new(storage.TransactionResults) + txResults := storage.NewTransactionResults(suite.T()) txResults.On("ByBlockIDTransactionIndex", bID, wrongTxIndex).Return(nil, status.Error(codes.Internal, "")).Once() handler := createHandler(txResults) @@ -665,9 +658,6 @@ func (suite *Suite) TestGetTransactionResult() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.Internal, "")) - - // check that one storage call was made - suite.events.AssertExpectations(suite.T()) }) } @@ -710,6 +700,7 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { headers: suite.headers, events: suite.events, transactionResults: txResults, + commits: suite.commits, chain: flow.Mainnet, } return handler @@ -736,6 +727,8 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { // happy path - valid requests receives all events for the given transaction suite.Run("happy path with valid events and no transaction error", func() { + suite.commits.On("ByBlockID", bID).Return(nil, nil).Once() + // expect a call to lookup events by block ID and transaction ID suite.events.On("ByBlockID", bID).Return(eventsForBlock, nil).Once() @@ -756,7 +749,7 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { } // expect a call to lookup transaction result by block ID return a result with no error - txResultsMock := new(storage.TransactionResults) + txResultsMock := storage.NewTransactionResults(suite.T()) txResults := []flow.TransactionResult{ { TransactionID: tx1ID, @@ -782,15 +775,13 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { // check that all fields in response are as expected assertEqual(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResultsMock.AssertExpectations(suite.T()) }) // happy path - valid requests receives all events and an error for the given transaction suite.Run("happy path with valid events and a transaction error", func() { + suite.commits.On("ByBlockID", bID).Return(nil, nil).Once() + // expect a call to lookup events by block ID and transaction ID suite.events.On("ByBlockID", bID).Return(eventsForBlock, nil).Once() @@ -811,7 +802,7 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { } // expect a call to lookup transaction result by block ID return a result with no error - txResultsMock := new(storage.TransactionResults) + txResultsMock := storage.NewTransactionResults(suite.T()) txResults := []flow.TransactionResult{ { TransactionID: tx1ID, @@ -837,10 +828,6 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { // check that all fields in response are as expected assertEqual(expectedResult, actualResult) - - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResultsMock.AssertExpectations(suite.T()) }) // failure path - nil block id in the request results in an error @@ -849,10 +836,7 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { // create an API request with a nil block id req := concoctReq(nil) - txResults := new(storage.TransactionResults) - - txResults.On("ByBlockID", nil).Return(nil, status.Error(codes.InvalidArgument, "")).Once() - + txResults := storage.NewTransactionResults(suite.T()) handler := createHandler(txResults) _, err := handler.GetTransactionResultsByBlockID(context.Background(), req) @@ -860,43 +844,24 @@ func (suite *Suite) TestGetTransactionResultsByBlockID() { // check that an error was received suite.Require().Error(err) errors.Is(err, status.Error(codes.InvalidArgument, "")) - - // check that no storage calls was made - suite.events.AssertExpectations(suite.T()) }) - // failure path - nonexisting block id in the request results in valid, but empty + // failure path - nonexisting block id in the request results in not found error suite.Run("request with nonexisting block ID", func() { - // expect a call to lookup events by block ID and transaction ID - suite.events.On("ByBlockID", nonexistingBlockID).Return(eventsForBlock, nil).Once() - - // create the expected result - expectedResult := &execution.GetTransactionResultsResponse{ - TransactionResults: []*execution.GetTransactionResultResponse{}, - } - - // expect a call to lookup transaction result by block ID return a result with no error - txResultsMock := new(storage.TransactionResults) - var txResults []flow.TransactionResult - txResultsMock.On("ByBlockID", nonexistingBlockID).Return(txResults, nil).Once() + suite.commits.On("ByBlockID", nonexistingBlockID).Return(nil, realstorage.ErrNotFound).Once() + txResultsMock := storage.NewTransactionResults(suite.T()) handler := createHandler(txResultsMock) // create a valid API request req := concoctReq(nonexistingBlockID[:]) // execute the GetTransactionResult call - actualResult, err := handler.GetTransactionResultsByBlockID(context.Background(), req) - - // check that a successful response is received - suite.Require().NoError(err) - - // check that all fields in response are as expected - assertEqual(expectedResult, actualResult) + _, err := handler.GetTransactionResultsByBlockID(context.Background(), req) - // check that appropriate storage calls were made - suite.events.AssertExpectations(suite.T()) - txResultsMock.AssertExpectations(suite.T()) + // check that an error was received + suite.Require().Error(err) + errors.Is(err, status.Error(codes.NotFound, "")) }) }