From 3c2a92fd81850a897c2a3663839d1dd29e47ea7f Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Mon, 10 Oct 2022 19:12:00 -0400 Subject: [PATCH 01/13] fix: gossip block announce only after block sucessfully imported --- dot/core/service.go | 43 ++++++++++++++++++++++++++--------- dot/network/block_announce.go | 11 ++++----- dot/sync/chain_processor.go | 1 + dot/sync/chain_sync.go | 2 +- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 6ed9a62032..57c3d27106 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -127,40 +127,61 @@ func (s *Service) StorageRoot() (common.Hash, error) { // HandleBlockImport handles a block that was imported via the network func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieState) error { - return s.handleBlock(block, state) + err := s.handleBlock(block, state) + if err != nil { + return fmt.Errorf("handling block: %s", err) + } + + const isBestBlock = false + blockAnnounce, err := createBlockAnnounce(block, isBestBlock) + if err != nil { + logger.Errorf("creating block announce: %s", err) + } + + s.net.GossipMessage(blockAnnounce) + return nil } // HandleBlockProduced handles a block that was produced by us // It is handled the same as an imported block in terms of state updates; the only difference // is we send a BlockAnnounceMessage to our peers. func (s *Service) HandleBlockProduced(block *types.Block, state *rtstorage.TrieState) error { - if err := s.handleBlock(block, state); err != nil { - return err + err := s.handleBlock(block, state) + if err != nil { + return fmt.Errorf("handling block: %s", err) } + const isBestBlock = true + blockAnnounce, err := createBlockAnnounce(block, isBestBlock) + if err != nil { + logger.Errorf("creating block announce: %s", err) + } + + s.net.GossipMessage(blockAnnounce) + return nil +} + +func createBlockAnnounce(block *types.Block, isBestBlock bool) (blockAnnounce *network.BlockAnnounceMessage, err error) { digest := types.NewDigest() for i := range block.Header.Digest.Types { digestValue, err := block.Header.Digest.Types[i].Value() if err != nil { - return fmt.Errorf("getting value of digest type at index %d: %w", i, err) + return nil, fmt.Errorf("getting value of digest type at index %d: %w", i, err) } err = digest.Add(digestValue) if err != nil { - return err + return nil, err } } - msg := &network.BlockAnnounceMessage{ + return &network.BlockAnnounceMessage{ ParentHash: block.Header.ParentHash, Number: block.Header.Number, StateRoot: block.Header.StateRoot, ExtrinsicsRoot: block.Header.ExtrinsicsRoot, Digest: digest, - BestBlock: true, - } - - s.net.GossipMessage(msg) - return nil + BestBlock: isBestBlock, + }, nil } func (s *Service) handleBlock(block *types.Block, state *rtstorage.TrieState) error { diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go index 83800d8e49..a45b2c9a64 100644 --- a/dot/network/block_announce.go +++ b/dot/network/block_announce.go @@ -203,14 +203,13 @@ func (s *Service) validateBlockAnnounceHandshake(from peer.ID, hs Handshake) err // if some more blocks are required to sync the announced block, the node will open a sync stream // with its peer and send a BlockRequest message func (s *Service) handleBlockAnnounceMessage(from peer.ID, msg NotificationsMessage) (propagate bool, err error) { + propagate = false + bam, ok := msg.(*BlockAnnounceMessage) if !ok { - return false, errors.New("invalid message") - } - - if err = s.syncer.HandleBlockAnnounce(from, bam); err != nil { - return false, err + return propagate, errors.New("invalid message") } - return true, nil + err = s.syncer.HandleBlockAnnounce(from, bam) + return propagate, err } diff --git a/dot/sync/chain_processor.go b/dot/sync/chain_processor.go index 9f0ba79ba8..001cd5274f 100644 --- a/dot/sync/chain_processor.go +++ b/dot/sync/chain_processor.go @@ -238,6 +238,7 @@ func (s *chainProcessor) handleBlock(block *types.Block) error { } logger.Debugf("🔗 imported block number %d with hash %s", block.Header.Number, block.Header.Hash()) + // should we announce the block here? blockHash := block.Header.Hash() s.telemetry.SendMessage(telemetry.NewBlockImport( diff --git a/dot/sync/chain_sync.go b/dot/sync/chain_sync.go index 6294365757..30090f3186 100644 --- a/dot/sync/chain_sync.go +++ b/dot/sync/chain_sync.go @@ -632,7 +632,7 @@ func (cs *chainSync) dispatchWorker(w *worker) { "start hash %s, target hash %s, "+ "request data %d, direction %s", w.id, - w.startNumber, w.targetNumber, + *w.startNumber, *w.targetNumber, w.startHash, w.targetHash, w.requestData, w.direction) From 069109b1ceeeacc8cd86bd1296138ad1e26f3566 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Mon, 10 Oct 2022 19:25:04 -0400 Subject: [PATCH 02/13] chore: imported block could be a best block --- dot/core/service.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dot/core/service.go b/dot/core/service.go index 57c3d27106..cf72ef52d8 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -132,7 +132,13 @@ func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieSta return fmt.Errorf("handling block: %s", err) } - const isBestBlock = false + isBestBlock := false + bestBlockHash := s.blockState.BestBlockHash() + + if bestBlockHash.Equal(block.Header.Hash()) { + isBestBlock = true + } + blockAnnounce, err := createBlockAnnounce(block, isBestBlock) if err != nil { logger.Errorf("creating block announce: %s", err) From b3c1ceaf293c35e792285c83cd29d723aef12acb Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Tue, 11 Oct 2022 11:00:35 -0400 Subject: [PATCH 03/13] chore: fix lint warns --- dot/core/service.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dot/core/service.go b/dot/core/service.go index cf72ef52d8..e4c77fefae 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -167,7 +167,8 @@ func (s *Service) HandleBlockProduced(block *types.Block, state *rtstorage.TrieS return nil } -func createBlockAnnounce(block *types.Block, isBestBlock bool) (blockAnnounce *network.BlockAnnounceMessage, err error) { +func createBlockAnnounce(block *types.Block, isBestBlock bool) ( + blockAnnounce *network.BlockAnnounceMessage, err error) { digest := types.NewDigest() for i := range block.Header.Digest.Types { digestValue, err := block.Header.Digest.Types[i].Value() From f9c0d5200b6c94234d338be47fb8523273dfb57e Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Tue, 11 Oct 2022 13:54:11 -0400 Subject: [PATCH 04/13] chore: simplify bool var, remove unneeded comment --- dot/core/service.go | 10 +++------- dot/network/block_announce.go | 6 ++---- dot/sync/chain_processor.go | 1 - dot/sync/chain_sync.go | 8 ++++---- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index e4c77fefae..6dd568f651 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -132,16 +132,12 @@ func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieSta return fmt.Errorf("handling block: %s", err) } - isBestBlock := false bestBlockHash := s.blockState.BestBlockHash() - - if bestBlockHash.Equal(block.Header.Hash()) { - isBestBlock = true - } + isBestBlock := bestBlockHash.Equal(block.Header.Hash()) blockAnnounce, err := createBlockAnnounce(block, isBestBlock) if err != nil { - logger.Errorf("creating block announce: %s", err) + return fmt.Errorf("creating block announce: %w", err) } s.net.GossipMessage(blockAnnounce) @@ -160,7 +156,7 @@ func (s *Service) HandleBlockProduced(block *types.Block, state *rtstorage.TrieS const isBestBlock = true blockAnnounce, err := createBlockAnnounce(block, isBestBlock) if err != nil { - logger.Errorf("creating block announce: %s", err) + return fmt.Errorf("creating block announce: %w", err) } s.net.GossipMessage(blockAnnounce) diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go index a45b2c9a64..54a1dc5df1 100644 --- a/dot/network/block_announce.go +++ b/dot/network/block_announce.go @@ -203,13 +203,11 @@ func (s *Service) validateBlockAnnounceHandshake(from peer.ID, hs Handshake) err // if some more blocks are required to sync the announced block, the node will open a sync stream // with its peer and send a BlockRequest message func (s *Service) handleBlockAnnounceMessage(from peer.ID, msg NotificationsMessage) (propagate bool, err error) { - propagate = false - bam, ok := msg.(*BlockAnnounceMessage) if !ok { - return propagate, errors.New("invalid message") + return false, errors.New("invalid message") } err = s.syncer.HandleBlockAnnounce(from, bam) - return propagate, err + return false, err } diff --git a/dot/sync/chain_processor.go b/dot/sync/chain_processor.go index 001cd5274f..9f0ba79ba8 100644 --- a/dot/sync/chain_processor.go +++ b/dot/sync/chain_processor.go @@ -238,7 +238,6 @@ func (s *chainProcessor) handleBlock(block *types.Block) error { } logger.Debugf("🔗 imported block number %d with hash %s", block.Header.Number, block.Header.Hash()) - // should we announce the block here? blockHash := block.Header.Hash() s.telemetry.SendMessage(telemetry.NewBlockImport( diff --git a/dot/sync/chain_sync.go b/dot/sync/chain_sync.go index 30090f3186..c79425e248 100644 --- a/dot/sync/chain_sync.go +++ b/dot/sync/chain_sync.go @@ -627,6 +627,10 @@ func (cs *chainSync) tryDispatchWorker(w *worker) { // if it fails due to any reason, it sets the worker `err` and returns // this function always places the worker into the `resultCh` for result handling upon return func (cs *chainSync) dispatchWorker(w *worker) { + if w.targetNumber == nil || w.startNumber == nil { + return + } + logger.Debugf("dispatching sync worker id %d, "+ "start number %d, target number %d, "+ "start hash %s, target hash %s, "+ @@ -636,10 +640,6 @@ func (cs *chainSync) dispatchWorker(w *worker) { w.startHash, w.targetHash, w.requestData, w.direction) - if w.targetNumber == nil || w.startNumber == nil { - return - } - start := time.Now() defer func() { end := time.Now() From 35b18ff6561b430cbaf99a3865df171e2f2a8e43 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Tue, 11 Oct 2022 16:47:56 -0400 Subject: [PATCH 05/13] wip: maybe propagate message when we have the block --- dot/network/block_announce.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go index 54a1dc5df1..2248d2c995 100644 --- a/dot/network/block_announce.go +++ b/dot/network/block_announce.go @@ -208,6 +208,8 @@ func (s *Service) handleBlockAnnounceMessage(from peer.ID, msg NotificationsMess return false, errors.New("invalid message") } + // TODO announce if we already have the block err = s.syncer.HandleBlockAnnounce(from, bam) + return false, err } From 48fcdf69b3243a7ed7a137b7a3b0ef77172e8115 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 13 Oct 2022 15:05:54 -0400 Subject: [PATCH 06/13] chore: change test to check `propagate` var is `false` --- dot/network/block_announce_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go index 2068db0701..badb38fc59 100644 --- a/dot/network/block_announce_test.go +++ b/dot/network/block_announce_test.go @@ -143,7 +143,7 @@ func TestHandleBlockAnnounceMessage(t *testing.T) { propagate, err := s.handleBlockAnnounceMessage(peerID, msg) require.NoError(t, err) - require.True(t, propagate) + require.False(t, propagate) } func TestValidateBlockAnnounceHandshake(t *testing.T) { From 80c8616d603ebd5f67d0a6c7e6143051e07b6201 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 13 Oct 2022 17:31:53 -0400 Subject: [PATCH 07/13] chore: increase code coverage + fix tests --- dot/network/block_announce.go | 4 ++ dot/network/block_announce_test.go | 64 +++++++++++++++++++------ dot/sync/chain_sync.go | 3 +- dot/sync/chain_sync_test.go | 76 +++++++++++++++++++++++++++++- 4 files changed, 129 insertions(+), 18 deletions(-) diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go index 2248d2c995..2e9f3caf27 100644 --- a/dot/network/block_announce.go +++ b/dot/network/block_announce.go @@ -9,6 +9,7 @@ import ( "github.com/ChainSafe/gossamer/dot/peerset" "github.com/ChainSafe/gossamer/dot/types" + "github.com/ChainSafe/gossamer/lib/blocktree" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/pkg/scale" @@ -210,6 +211,9 @@ func (s *Service) handleBlockAnnounceMessage(from peer.ID, msg NotificationsMess // TODO announce if we already have the block err = s.syncer.HandleBlockAnnounce(from, bam) + if errors.Is(err, blocktree.ErrBlockExists) { + return true, nil + } return false, err } diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go index badb38fc59..9dbce1e814 100644 --- a/dot/network/block_announce_test.go +++ b/dot/network/block_announce_test.go @@ -7,8 +7,10 @@ import ( "testing" "github.com/ChainSafe/gossamer/dot/types" + "github.com/ChainSafe/gossamer/lib/blocktree" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/pkg/scale" + gomock "github.com/golang/mock/gomock" "github.com/libp2p/go-libp2p-core/peer" "github.com/stretchr/testify/require" @@ -126,24 +128,56 @@ func TestDecodeBlockAnnounceHandshake(t *testing.T) { func TestHandleBlockAnnounceMessage(t *testing.T) { t.Parallel() - config := &Config{ - BasePath: t.TempDir(), - Port: availablePort(t), - NoBootstrap: true, - NoMDNS: true, + testCases := map[string]struct { + propagate bool + mockSyncer func(*testing.T, peer.ID, *BlockAnnounceMessage) Syncer + }{ + "block already exists": { + mockSyncer: func(t *testing.T, peer peer.ID, blockAnnounceMessage *BlockAnnounceMessage) Syncer { + ctrl := gomock.NewController(t) + syncer := NewMockSyncer(ctrl) + syncer.EXPECT(). + HandleBlockAnnounce(peer, blockAnnounceMessage). + Return(blocktree.ErrBlockExists). + Times(1) + + return syncer + }, + propagate: true, + }, + "block does not exists": { + propagate: false, + }, } - s := createTestService(t, config) - - peerID := peer.ID("noot") - msg := &BlockAnnounceMessage{ - Number: 10, - Digest: types.NewDigest(), + for tname, tt := range testCases { + tt := tt + + t.Run(tname, func(t *testing.T) { + config := &Config{ + BasePath: t.TempDir(), + Port: availablePort(t), + NoBootstrap: true, + NoMDNS: true, + } + + peerID := peer.ID("noot") + msg := &BlockAnnounceMessage{ + Number: 10, + Digest: types.NewDigest(), + } + + if tt.mockSyncer != nil { + config.Syncer = tt.mockSyncer(t, peerID, msg) + } + + s := createTestService(t, config) + gotPropagate, err := s.handleBlockAnnounceMessage(peerID, msg) + + require.NoError(t, err) + require.Equal(t, tt.propagate, gotPropagate) + }) } - - propagate, err := s.handleBlockAnnounceMessage(peerID, msg) - require.NoError(t, err) - require.False(t, propagate) } func TestValidateBlockAnnounceHandshake(t *testing.T) { diff --git a/dot/sync/chain_sync.go b/dot/sync/chain_sync.go index c79425e248..7ca4d6de50 100644 --- a/dot/sync/chain_sync.go +++ b/dot/sync/chain_sync.go @@ -21,6 +21,7 @@ import ( "github.com/ChainSafe/gossamer/dot/network" "github.com/ChainSafe/gossamer/dot/peerset" "github.com/ChainSafe/gossamer/dot/types" + "github.com/ChainSafe/gossamer/lib/blocktree" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/lib/common/variadic" ) @@ -251,7 +252,7 @@ func (cs *chainSync) setBlockAnnounce(from peer.ID, header *types.Header) error } if has { - return nil + return blocktree.ErrBlockExists } if err = cs.pendingBlocks.addHeader(header); err != nil { diff --git a/dot/sync/chain_sync_test.go b/dot/sync/chain_sync_test.go index 18b13d0bf3..203742816f 100644 --- a/dot/sync/chain_sync_test.go +++ b/dot/sync/chain_sync_test.go @@ -12,6 +12,7 @@ import ( "github.com/ChainSafe/gossamer/dot/network" "github.com/ChainSafe/gossamer/dot/peerset" "github.com/ChainSafe/gossamer/dot/types" + "github.com/ChainSafe/gossamer/lib/blocktree" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/lib/common/variadic" "github.com/ChainSafe/gossamer/lib/trie" @@ -1246,6 +1247,8 @@ func Test_chainSync_start(t *testing.T) { } } +var blockAnnounceHeader = &types.Header{Number: 2} + func Test_chainSync_setBlockAnnounce(t *testing.T) { type args struct { from peer.ID @@ -1257,8 +1260,9 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { wantErr error }{ "base case": { + wantErr: blocktree.ErrBlockExists, args: args{ - header: &types.Header{Number: 2}, + header: blockAnnounceHeader, }, chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { mockBlockState := NewMockBlockState(ctrl) @@ -1271,13 +1275,81 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { } }, }, + "err_when_calling_has_header": { + wantErr: errors.New("checking header exists"), + args: args{ + header: blockAnnounceHeader, + }, + chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { + mockBlockState := NewMockBlockState(ctrl) + mockBlockState.EXPECT(). + HasHeader(common.MustHexToHash( + "0x05bdcc454f60a08d427d05e7f19f240fdc391f570ab76fcb96ecca0b5823d3bf")). + Return(false, errors.New("checking header exists")) + mockDisjointBlockSet := NewMockDisjointBlockSet(ctrl) + return chainSync{ + blockState: mockBlockState, + pendingBlocks: mockDisjointBlockSet, + } + }, + }, + "adding_block_header_to_pending_blocks": { + args: args{ + header: blockAnnounceHeader, + }, + chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { + argumentHeaderHash := common.MustHexToHash( + "0x05bdcc454f60a08d427d05e7f19f240fdc391f570ab76fcb96ecca0b5823d3bf") + + mockBlockState := NewMockBlockState(ctrl) + mockBlockState.EXPECT(). + HasHeader(argumentHeaderHash). + Return(false, nil) + + mockBlockState.EXPECT(). + BestBlockHeader(). + Return(&types.Header{Number: 1}, nil) + + mockDisjointBlockSet := NewMockDisjointBlockSet(ctrl) + mockDisjointBlockSet.EXPECT(). + addHeader(blockAnnounceHeader). + Return(nil) + + mockDisjointBlockSet.EXPECT(). + addHashAndNumber(argumentHeaderHash, uint(2)). + Return(nil) + + return chainSync{ + blockState: mockBlockState, + pendingBlocks: mockDisjointBlockSet, + peerState: make(map[peer.ID]*peerState), + // creating an buffered channel for this specific test + // since it will put a work on the queue and an unbufered channel + // will hang until we read on this channel and the goal is to + // put the work on the channel and don't block + workQueue: make(chan *peerState, 1), + } + }, + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { ctrl := gomock.NewController(t) sync := tt.chainSyncBuilder(ctrl) err := sync.setBlockAnnounce(tt.args.from, tt.args.header) - assert.ErrorIs(t, err, tt.wantErr) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.NoError(t, err) + } + + if sync.workQueue != nil { + assert.LessOrEqual(t, len(sync.workQueue), 1) + if len(sync.workQueue) > 0 { + <-sync.workQueue + close(sync.workQueue) + } + } }) } } From 642b8470c4b6178ae01b371902e34e8cbed5082d Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 13 Oct 2022 17:45:53 -0400 Subject: [PATCH 08/13] chore: fix lint warns --- dot/network/block_announce_test.go | 2 ++ dot/sync/chain_sync_test.go | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go index 9dbce1e814..a5a6ed71a0 100644 --- a/dot/network/block_announce_test.go +++ b/dot/network/block_announce_test.go @@ -154,6 +154,8 @@ func TestHandleBlockAnnounceMessage(t *testing.T) { tt := tt t.Run(tname, func(t *testing.T) { + t.Parallel() + config := &Config{ BasePath: t.TempDir(), Port: availablePort(t), diff --git a/dot/sync/chain_sync_test.go b/dot/sync/chain_sync_test.go index 203742816f..1c152e4b0f 100644 --- a/dot/sync/chain_sync_test.go +++ b/dot/sync/chain_sync_test.go @@ -1250,6 +1250,8 @@ func Test_chainSync_start(t *testing.T) { var blockAnnounceHeader = &types.Header{Number: 2} func Test_chainSync_setBlockAnnounce(t *testing.T) { + t.Parallel() + type args struct { from peer.ID header *types.Header @@ -1322,7 +1324,7 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { return chainSync{ blockState: mockBlockState, pendingBlocks: mockDisjointBlockSet, - peerState: make(map[peer.ID]*peerState), + peerState: make(map[peer.ID]*peerState), // creating an buffered channel for this specific test // since it will put a work on the queue and an unbufered channel // will hang until we read on this channel and the goal is to @@ -1334,6 +1336,7 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { + t.Parallel() ctrl := gomock.NewController(t) sync := tt.chainSyncBuilder(ctrl) err := sync.setBlockAnnounce(tt.args.from, tt.args.header) From a539478f7d5c1a3f4c81d19811c145eda483f04e Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 13 Oct 2022 19:17:20 -0400 Subject: [PATCH 09/13] chore: solving error wrapping --- dot/core/service.go | 2 +- dot/core/service_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 6dd568f651..fe494eedd6 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -150,7 +150,7 @@ func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieSta func (s *Service) HandleBlockProduced(block *types.Block, state *rtstorage.TrieState) error { err := s.handleBlock(block, state) if err != nil { - return fmt.Errorf("handling block: %s", err) + return fmt.Errorf("handling block: %w", err) } const isBestBlock = true diff --git a/dot/core/service_test.go b/dot/core/service_test.go index e541556755..52b8299756 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -480,9 +480,10 @@ func Test_Service_HandleBlockProduced(t *testing.T) { t.Parallel() execTest := func(t *testing.T, s *Service, block *types.Block, trieState *rtstorage.TrieState, expErr error) { err := s.HandleBlockProduced(block, trieState) - assert.ErrorIs(t, err, expErr) + require.ErrorIs(t, err, expErr) if expErr != nil { - assert.EqualError(t, err, expErr.Error()) + wrapped := fmt.Errorf("handling block: %w", expErr) + assert.EqualError(t, err, wrapped.Error()) } } t.Run("nil input", func(t *testing.T) { From 6eea08bdb61980a5311d5bf806bba8e9318f72a8 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Fri, 14 Oct 2022 15:36:43 -0400 Subject: [PATCH 10/13] chore: remove useless comment --- dot/network/block_announce.go | 1 - 1 file changed, 1 deletion(-) diff --git a/dot/network/block_announce.go b/dot/network/block_announce.go index 2e9f3caf27..b4091aacde 100644 --- a/dot/network/block_announce.go +++ b/dot/network/block_announce.go @@ -209,7 +209,6 @@ func (s *Service) handleBlockAnnounceMessage(from peer.ID, msg NotificationsMess return false, errors.New("invalid message") } - // TODO announce if we already have the block err = s.syncer.HandleBlockAnnounce(from, bam) if errors.Is(err, blocktree.ErrBlockExists) { return true, nil From 7cccaf9da3212e90593c501cc92d1cc37a665887 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Mon, 17 Oct 2022 22:21:08 -0400 Subject: [PATCH 11/13] chore: address comment --- dot/core/service.go | 4 ++-- dot/core/service_test.go | 5 ++--- dot/network/block_announce_test.go | 8 +++----- dot/sync/chain_sync_test.go | 27 +++++++++++---------------- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index fe494eedd6..24ffb6d650 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -129,7 +129,7 @@ func (s *Service) StorageRoot() (common.Hash, error) { func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieState) error { err := s.handleBlock(block, state) if err != nil { - return fmt.Errorf("handling block: %s", err) + return fmt.Errorf("handling block: %w", err) } bestBlockHash := s.blockState.BestBlockHash() @@ -173,7 +173,7 @@ func createBlockAnnounce(block *types.Block, isBestBlock bool) ( } err = digest.Add(digestValue) if err != nil { - return nil, err + return nil, fmt.Errorf("adding digest value for type at index %d: %w", i, err) } } diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 52b8299756..75b01b7291 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -480,10 +480,9 @@ func Test_Service_HandleBlockProduced(t *testing.T) { t.Parallel() execTest := func(t *testing.T, s *Service, block *types.Block, trieState *rtstorage.TrieState, expErr error) { err := s.HandleBlockProduced(block, trieState) - require.ErrorIs(t, err, expErr) + require.ErrorIs(t, expErr, err) if expErr != nil { - wrapped := fmt.Errorf("handling block: %w", expErr) - assert.EqualError(t, err, wrapped.Error()) + assert.EqualError(t, err, "handling block: "+expErr.Error()) } } t.Run("nil input", func(t *testing.T) { diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go index a5a6ed71a0..91d14312f6 100644 --- a/dot/network/block_announce_test.go +++ b/dot/network/block_announce_test.go @@ -138,9 +138,7 @@ func TestHandleBlockAnnounceMessage(t *testing.T) { syncer := NewMockSyncer(ctrl) syncer.EXPECT(). HandleBlockAnnounce(peer, blockAnnounceMessage). - Return(blocktree.ErrBlockExists). - Times(1) - + Return(blocktree.ErrBlockExists) return syncer }, propagate: true, @@ -173,8 +171,8 @@ func TestHandleBlockAnnounceMessage(t *testing.T) { config.Syncer = tt.mockSyncer(t, peerID, msg) } - s := createTestService(t, config) - gotPropagate, err := s.handleBlockAnnounceMessage(peerID, msg) + service := createTestService(t, config) + gotPropagate, err := service.handleBlockAnnounceMessage(peerID, msg) require.NoError(t, err) require.Equal(t, tt.propagate, gotPropagate) diff --git a/dot/sync/chain_sync_test.go b/dot/sync/chain_sync_test.go index 1c152e4b0f..b6daf18edf 100644 --- a/dot/sync/chain_sync_test.go +++ b/dot/sync/chain_sync_test.go @@ -1247,8 +1247,6 @@ func Test_chainSync_start(t *testing.T) { } } -var blockAnnounceHeader = &types.Header{Number: 2} - func Test_chainSync_setBlockAnnounce(t *testing.T) { t.Parallel() @@ -1257,16 +1255,16 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { header *types.Header } tests := map[string]struct { - chainSyncBuilder func(ctrl *gomock.Controller) chainSync + chainSyncBuilder func(*types.Header, *gomock.Controller) chainSync args args wantErr error }{ "base case": { wantErr: blocktree.ErrBlockExists, args: args{ - header: blockAnnounceHeader, + header: &types.Header{Number: 2}, }, - chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { + chainSyncBuilder: func(_ *types.Header, ctrl *gomock.Controller) chainSync { mockBlockState := NewMockBlockState(ctrl) mockBlockState.EXPECT().HasHeader(common.MustHexToHash( "0x05bdcc454f60a08d427d05e7f19f240fdc391f570ab76fcb96ecca0b5823d3bf")).Return(true, nil) @@ -1280,9 +1278,9 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { "err_when_calling_has_header": { wantErr: errors.New("checking header exists"), args: args{ - header: blockAnnounceHeader, + header: &types.Header{Number: 2}, }, - chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { + chainSyncBuilder: func(_ *types.Header, ctrl *gomock.Controller) chainSync { mockBlockState := NewMockBlockState(ctrl) mockBlockState.EXPECT(). HasHeader(common.MustHexToHash( @@ -1297,9 +1295,9 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { }, "adding_block_header_to_pending_blocks": { args: args{ - header: blockAnnounceHeader, + header: &types.Header{Number: 2}, }, - chainSyncBuilder: func(ctrl *gomock.Controller) chainSync { + chainSyncBuilder: func(expectedHeader *types.Header, ctrl *gomock.Controller) chainSync { argumentHeaderHash := common.MustHexToHash( "0x05bdcc454f60a08d427d05e7f19f240fdc391f570ab76fcb96ecca0b5823d3bf") @@ -1314,7 +1312,7 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { mockDisjointBlockSet := NewMockDisjointBlockSet(ctrl) mockDisjointBlockSet.EXPECT(). - addHeader(blockAnnounceHeader). + addHeader(expectedHeader). Return(nil) mockDisjointBlockSet.EXPECT(). @@ -1335,10 +1333,11 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { }, } for name, tt := range tests { + tt := tt t.Run(name, func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) - sync := tt.chainSyncBuilder(ctrl) + sync := tt.chainSyncBuilder(tt.args.header, ctrl) err := sync.setBlockAnnounce(tt.args.from, tt.args.header) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) @@ -1347,11 +1346,7 @@ func Test_chainSync_setBlockAnnounce(t *testing.T) { } if sync.workQueue != nil { - assert.LessOrEqual(t, len(sync.workQueue), 1) - if len(sync.workQueue) > 0 { - <-sync.workQueue - close(sync.workQueue) - } + assert.Equal(t, len(sync.workQueue), 1) } }) } From 1cde08c9e8eb6e663bbb6ae2eeda4a2b0d2ea484 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 20 Oct 2022 14:13:01 -0400 Subject: [PATCH 12/13] chore: fix inverted `errors.Is` check --- dot/core/service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 75b01b7291..bb98124291 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -480,7 +480,7 @@ func Test_Service_HandleBlockProduced(t *testing.T) { t.Parallel() execTest := func(t *testing.T, s *Service, block *types.Block, trieState *rtstorage.TrieState, expErr error) { err := s.HandleBlockProduced(block, trieState) - require.ErrorIs(t, expErr, err) + require.ErrorIs(t, err, expErr) if expErr != nil { assert.EqualError(t, err, "handling block: "+expErr.Error()) } From 5b004be59d42570e22d2fd17ad4f1e8d18b0cb6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ecl=C3=A9sio=20Junior?= Date: Thu, 20 Oct 2022 21:22:31 -0400 Subject: [PATCH 13/13] chore Co-authored-by: Timothy Wu --- dot/core/service.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 24ffb6d650..833f63e984 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -153,8 +153,7 @@ func (s *Service) HandleBlockProduced(block *types.Block, state *rtstorage.TrieS return fmt.Errorf("handling block: %w", err) } - const isBestBlock = true - blockAnnounce, err := createBlockAnnounce(block, isBestBlock) + blockAnnounce, err := createBlockAnnounce(block, true) if err != nil { return fmt.Errorf("creating block announce: %w", err) }