From 37fda37ca1939a26be7439b6fc071ea8faa39f7f Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Thu, 22 Sep 2022 14:15:01 -0400 Subject: [PATCH] chore(dot/sync): `handleJustification` returns errors (#2810) - Remove nil check on header and justification (unused, see caller checks - and caller should be the ones checking what they pass down) - Return wrapped errors to callers - Change callers to handle non-nil errors and return them up the call stack --- dot/sync/chain_processor.go | 30 +++--- dot/sync/chain_processor_integration_test.go | 3 +- dot/sync/chain_processor_test.go | 105 +++++++++++++++---- 3 files changed, 107 insertions(+), 31 deletions(-) diff --git a/dot/sync/chain_processor.go b/dot/sync/chain_processor.go index e2d7ff74ed..9f0ba79ba8 100644 --- a/dot/sync/chain_processor.go +++ b/dot/sync/chain_processor.go @@ -134,7 +134,10 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error { if bd.Justification != nil { logger.Debugf("handling Justification for block number %d with hash %s...", block.Header.Number, bd.Hash) - s.handleJustification(&block.Header, *bd.Justification) + err = s.handleJustification(&block.Header, *bd.Justification) + if err != nil { + return fmt.Errorf("handling justification: %w", err) + } } // TODO: this is probably unnecessary, since the state is already in the database @@ -177,7 +180,10 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error { if bd.Justification != nil && bd.Header != nil { logger.Debugf("handling Justification for block number %d with hash %s...", bd.Number(), bd.Hash) - s.handleJustification(bd.Header, *bd.Justification) + err = s.handleJustification(bd.Header, *bd.Justification) + if err != nil { + return fmt.Errorf("handling justification: %w", err) + } } if err := s.blockState.CompareAndSetBlockData(bd); err != nil { @@ -242,22 +248,22 @@ func (s *chainProcessor) handleBlock(block *types.Block) error { return nil } -func (s *chainProcessor) handleJustification(header *types.Header, justification []byte) { - if len(justification) == 0 || header == nil { - return +func (s *chainProcessor) handleJustification(header *types.Header, justification []byte) (err error) { + if len(justification) == 0 { + return nil } - returnedJustification, err := s.finalityGadget.VerifyBlockJustification(header.Hash(), justification) + headerHash := header.Hash() + returnedJustification, err := s.finalityGadget.VerifyBlockJustification(headerHash, justification) if err != nil { - logger.Warnf("failed to verify block number %d and hash %s justification: %s", header.Number, header.Hash(), err) - return + return fmt.Errorf("verifying block number %d justification: %w", header.Number, err) } - err = s.blockState.SetJustification(header.Hash(), returnedJustification) + err = s.blockState.SetJustification(headerHash, returnedJustification) if err != nil { - logger.Errorf("failed tostore justification: %s", err) - return + return fmt.Errorf("setting justification for block number %d: %w", header.Number, err) } - logger.Infof("🔨 finalised block number %d with hash %s", header.Number, header.Hash()) + logger.Infof("🔨 finalised block number %d with hash %s", header.Number, headerHash) + return nil } diff --git a/dot/sync/chain_processor_integration_test.go b/dot/sync/chain_processor_integration_test.go index fa43c84b5e..499276db3a 100644 --- a/dot/sync/chain_processor_integration_test.go +++ b/dot/sync/chain_processor_integration_test.go @@ -234,7 +234,8 @@ func TestChainProcessor_HandleJustification(t *testing.T) { }) require.NoError(t, err) - syncer.chainProcessor.(*chainProcessor).handleJustification(header, just) + err = syncer.chainProcessor.(*chainProcessor).handleJustification(header, just) + require.NoError(t, err) res, err := syncer.blockState.GetJustification(header.Hash()) require.NoError(t, err) diff --git a/dot/sync/chain_processor_test.go b/dot/sync/chain_processor_test.go index b785bd9830..f9671d843f 100644 --- a/dot/sync/chain_processor_test.go +++ b/dot/sync/chain_processor_test.go @@ -234,7 +234,11 @@ func Test_chainProcessor_handleBody(t *testing.T) { func Test_chainProcessor_handleJustification(t *testing.T) { t.Parallel() - expectedHash := common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a") + header := &types.Header{ + Number: 2, + } + headerHash := header.Hash() + errTest := errors.New("test error") type args struct { header *types.Header @@ -243,6 +247,8 @@ func Test_chainProcessor_handleJustification(t *testing.T) { tests := map[string]struct { chainProcessorBuilder func(ctrl *gomock.Controller) chainProcessor args args + sentinelError error + errorMessage string }{ "nil justification and header": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { @@ -252,52 +258,50 @@ func Test_chainProcessor_handleJustification(t *testing.T) { "invalid justification": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { mockFinalityGadget := NewMockFinalityGadget(ctrl) - mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, - []byte(`x`)).Return(nil, errors.New("error")) + mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, + []byte(`x`)).Return(nil, errTest) return chainProcessor{ finalityGadget: mockFinalityGadget, } }, args: args{ - header: &types.Header{ - Number: 0, - }, + header: header, justification: []byte(`x`), }, + sentinelError: errTest, + errorMessage: "verifying block number 2 justification: test error", }, "set justification error": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { mockBlockState := NewMockBlockState(ctrl) - mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`xx`)).Return(errors.New("fake error")) + mockBlockState.EXPECT().SetJustification(headerHash, []byte(`xx`)).Return(errTest) mockFinalityGadget := NewMockFinalityGadget(ctrl) - mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`xx`)).Return([]byte(`xx`), nil) + mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`xx`)).Return([]byte(`xx`), nil) return chainProcessor{ blockState: mockBlockState, finalityGadget: mockFinalityGadget, } }, args: args{ - header: &types.Header{ - Number: 0, - }, + header: header, justification: []byte(`xx`), }, + sentinelError: errTest, + errorMessage: "setting justification for block number 2: test error", }, "base case set": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { mockBlockState := NewMockBlockState(ctrl) - mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`1234`)).Return(nil) + mockBlockState.EXPECT().SetJustification(headerHash, []byte(`1234`)).Return(nil) mockFinalityGadget := NewMockFinalityGadget(ctrl) - mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`1234`)).Return([]byte(`1234`), nil) + mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`1234`)).Return([]byte(`1234`), nil) return chainProcessor{ blockState: mockBlockState, finalityGadget: mockFinalityGadget, } }, args: args{ - header: &types.Header{ - Number: 0, - }, + header: header, justification: []byte(`1234`), }, }, @@ -307,8 +311,15 @@ func Test_chainProcessor_handleJustification(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) - s := tt.chainProcessorBuilder(ctrl) - s.handleJustification(tt.args.header, tt.args.justification) + + processor := tt.chainProcessorBuilder(ctrl) + + err := processor.handleJustification(tt.args.header, tt.args.justification) + + assert.ErrorIs(t, err, tt.sentinelError) + if tt.sentinelError != nil { + assert.EqualError(t, err, tt.errorMessage) + } }) } } @@ -394,6 +405,39 @@ func Test_chainProcessor_processBlockData(t *testing.T) { Justification: &[]byte{1, 2, 3}, }, }, + "header and body - fail to handle justification": { + chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { + header := types.Header{ + Number: uint(1), + } + headerHash := header.Hash() + block := &types.Block{ + Header: header, + } + + blockState := NewMockBlockState(ctrl) + blockState.EXPECT().HasHeader(common.Hash{1}).Return(true, nil) + blockState.EXPECT().HasBlockBody(common.Hash{1}).Return(true, nil) + blockState.EXPECT().GetBlockByHash(common.Hash{1}). + Return(block, nil) + blockState.EXPECT().AddBlockToBlockTree(block).Return(nil) + + finalityGadget := NewMockFinalityGadget(ctrl) + finalityGadget.EXPECT(). + VerifyBlockJustification(headerHash, []byte{1, 2, 3}). + Return(nil, mockError) + + return chainProcessor{ + blockState: blockState, + finalityGadget: finalityGadget, + } + }, + blockData: &types.BlockData{ + Hash: common.Hash{1}, + Justification: &[]byte{1, 2, 3}, + }, + expectedError: mockError, + }, "handle block data justification != nil": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { mockBlock := &types.Block{ @@ -592,6 +636,31 @@ func Test_chainProcessor_processBlockData(t *testing.T) { Hash: common.Hash{1, 2, 3}, }, }, + "no header and body - fail to handle justification": { + chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { + blockState := NewMockBlockState(ctrl) + blockState.EXPECT().HasHeader(common.Hash{1}).Return(false, nil) + blockState.EXPECT().HasBlockBody(common.Hash{1}).Return(true, nil) + + finalityGadget := NewMockFinalityGadget(ctrl) + expectedBlockDataHeader := &types.Header{Number: 2} + expectedBlockDataHeaderHash := expectedBlockDataHeader.Hash() + finalityGadget.EXPECT(). + VerifyBlockJustification(expectedBlockDataHeaderHash, []byte{1, 2, 3}). + Return(nil, mockError) + + return chainProcessor{ + blockState: blockState, + finalityGadget: finalityGadget, + } + }, + blockData: &types.BlockData{ + Hash: common.Hash{1}, + Header: &types.Header{Number: 2}, + Justification: &[]byte{1, 2, 3}, + }, + expectedError: mockError, + }, "handle compareAndSetBlockData error": { chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor { mockBlockState := NewMockBlockState(ctrl)