improve test suite

dot/sync/fullsync.go

@@ -288,10 +288,9 @@ func (f *FullSyncStrategy) OnBlockAnnounceHandshake(from peer.ID, msg *network.B
 	return nil
 }
 
-func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnounceMessage) (
-	gossip bool, repChange *Change, err error) {
+func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnounceMessage) (repChange *Change, err error) {
 	if f.blockState.IsPaused() {
-		return false, nil, errors.New("blockstate service is paused")
+		return nil, errors.New("blockstate service is paused")
 	}
 
 	blockAnnounceHeader := types.NewHeader(msg.ParentHash, msg.StateRoot, msg.ExtrinsicsRoot, msg.Number, msg.Digest)
@@ -308,7 +307,7 @@ func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnou
 		logger.Infof("bad block receive from %s: #%d (%s) is a bad block",
 			from, blockAnnounceHeader.Number, blockAnnounceHeaderHash)
 
-		return false, &Change{
+		return &Change{
 			who: from,
 			rep: peerset.ReputationChange{
 				Value:  peerset.BadBlockAnnouncementValue,
@@ -323,12 +322,12 @@ func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnou
 
 	highestFinalized, err := f.blockState.GetHighestFinalisedHeader()
 	if err != nil {
-		return false, nil, fmt.Errorf("get highest finalised header: %w", err)
+		return nil, fmt.Errorf("get highest finalised header: %w", err)
 	}
 
 	// check if the announced block is relevant
 	if blockAnnounceHeader.Number <= highestFinalized.Number || f.blockAlreadyTracked(blockAnnounceHeader) {
-		logger.Infof("announced block irrelevant #%d (%s)", blockAnnounceHeader.Number, blockAnnounceHeaderHash.Short())
+		logger.Infof("ignoring announced block #%d (%s)", blockAnnounceHeader.Number, blockAnnounceHeaderHash.Short())
 		repChange = &Change{
 			who: from,
 			rep: peerset.ReputationChange{
@@ -337,28 +336,28 @@ func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnou
 			},
 		}
 
-		return false, repChange, fmt.Errorf("%w: peer %s, block number #%d (%s)",
+		return repChange, fmt.Errorf("%w: peer %s, block number #%d (%s)",
 			errPeerOnInvalidFork, from, blockAnnounceHeader.Number, blockAnnounceHeaderHash.String())
 	}
 
 	logger.Infof("relevant announced block #%d (%s)", blockAnnounceHeader.Number, blockAnnounceHeaderHash.Short())
 	bestBlockHeader, err := f.blockState.BestBlockHeader()
 	if err != nil {
-		return false, nil, fmt.Errorf("get best block header: %w", err)
+		return nil, fmt.Errorf("get best block header: %w", err)
 	}
 
-	// if we still far from aproaching the calculated target
+	// if we still far from aproaching the announced block
 	// then we can ignore the block announce
 	mx := max(blockAnnounceHeader.Number, bestBlockHeader.Number)
 	mn := min(blockAnnounceHeader.Number, bestBlockHeader.Number)
 	if (mx - mn) > messages.MaxBlocksInResponse {
-		return true, nil, nil
+		return nil, nil
 	}
 
 	has, err := f.blockState.HasHeader(blockAnnounceHeaderHash)
 	if err != nil {
 		if !errors.Is(err, database.ErrNotFound) {
-			return false, nil, fmt.Errorf("checking if header exists: %w", err)
+			return nil, fmt.Errorf("checking if header exists: %w", err)
 		}
 	}
 
@@ -372,7 +371,7 @@ func (f *FullSyncStrategy) OnBlockAnnounce(from peer.ID, msg *network.BlockAnnou
 		logger.Infof("announced block already exists #%d (%s)", blockAnnounceHeader.Number, blockAnnounceHeaderHash.Short())
 	}
 
-	return true, &Change{
+	return &Change{
 		who: from,
 		rep: peerset.ReputationChange{
 			Value:  peerset.GossipSuccessValue,

dot/sync/fullsync_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/ChainSafe/gossamer/dot/network"
 	"github.com/ChainSafe/gossamer/dot/network/messages"
+	"github.com/ChainSafe/gossamer/dot/peerset"
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/common/variadic"
@@ -310,10 +311,6 @@ func TestFullSyncBlockAnnounce(t *testing.T) {
 			BestBlockHeader().
 			Return(highestFinalizedHeader, nil)
 
-		// mockBlockState.EXPECT().
-		// 	HasHeader(gomock.AnyOf(common.Hash{})).
-		// 	Return(false, nil)
-
 		fsCfg := &FullSyncConfig{
 			BlockState: mockBlockState,
 		}
@@ -342,7 +339,7 @@ func TestFullSyncBlockAnnounce(t *testing.T) {
 			BestBlock:      true,
 		}
 
-		_, rep, err := fs.OnBlockAnnounce(firstPeer, firstBlockAnnounce)
+		rep, err := fs.OnBlockAnnounce(firstPeer, firstBlockAnnounce)
 		require.NoError(t, err)
 		require.Nil(t, rep)
 		require.Zero(t, fs.requestQueue.Len())
@@ -359,39 +356,25 @@ func TestFullSyncBlockAnnounce(t *testing.T) {
 
 		ctrl := gomock.NewController(t)
 		mockBlockState := NewMockBlockState(ctrl)
-		mockBlockState.EXPECT().IsPaused().Return(false)
+		mockBlockState.EXPECT().IsPaused().Return(false).Times(2)
 		mockBlockState.EXPECT().
 			GetHighestFinalisedHeader().
-			Return(highestFinalizedHeader, nil)
+			Return(highestFinalizedHeader, nil).Times(2)
 
 		mockBlockState.EXPECT().
 			BestBlockHeader().
-			Return(highestFinalizedHeader, nil)
+			Return(highestFinalizedHeader, nil).Times(2)
 
 		mockBlockState.EXPECT().
 			HasHeader(gomock.AssignableToTypeOf(common.Hash{})).
 			Return(false, nil)
-
 		fsCfg := &FullSyncConfig{
 			BlockState: mockBlockState,
 		}
 
 		fs := NewFullSyncStrategy(fsCfg)
 
-		firstPeer := peer.ID("fst-peer")
-		firstHandshake := &network.BlockAnnounceHandshake{
-			Roles:           1,
-			BestBlockNumber: 17,
-			BestBlockHash:   common.BytesToHash([]byte{0, 1, 2}),
-			GenesisHash:     common.BytesToHash([]byte{1, 1, 1, 1}),
-		}
-
-		err := fs.OnBlockAnnounceHandshake(firstPeer, firstHandshake)
-		require.NoError(t, err)
-
-		// still far from aproaching the calculated target
-		// then we can ignore the block announce
-		firstBlockAnnounce := &network.BlockAnnounceMessage{
+		announceOfBlock17 := &network.BlockAnnounceMessage{
 			ParentHash:     common.BytesToHash([]byte{0, 1, 2}),
 			Number:         17,
 			StateRoot:      common.BytesToHash([]byte{3, 3, 3, 3}),
@@ -400,12 +383,103 @@ func TestFullSyncBlockAnnounce(t *testing.T) {
 			BestBlock:      true,
 		}
 
-		// the announced block 17 is not far from our best block (0) then
-		// we will consider it and start a ancestor search
-		_, rep, err := fs.OnBlockAnnounce(firstPeer, firstBlockAnnounce)
-		require.NoError(t, err)
-		require.Nil(t, rep)
-		require.Equal(t, 1, fs.requestQueue.Len())
+		t.Run("peer_announces_block_17", func(t *testing.T) {
+			firstPeer := peer.ID("fst-peer")
+			firstHandshake := &network.BlockAnnounceHandshake{
+				Roles:           1,
+				BestBlockNumber: 17,
+				BestBlockHash:   common.BytesToHash([]byte{0, 1, 2}),
+				GenesisHash:     common.BytesToHash([]byte{1, 1, 1, 1}),
+			}
+
+			err := fs.OnBlockAnnounceHandshake(firstPeer, firstHandshake)
+			require.NoError(t, err)
+
+			// still far from aproaching the calculated target
+			// then we can ignore the block announce
+
+			// the announced block 17 is not far from our best block (0) then
+			// we will consider it and start a ancestor search
+			rep, err := fs.OnBlockAnnounce(firstPeer, announceOfBlock17)
+			require.NoError(t, err)
+
+			expectedReputation := &Change{
+				who: firstPeer,
+				rep: peerset.ReputationChange{
+					Value:  peerset.GossipSuccessValue,
+					Reason: peerset.GossipSuccessReason,
+				},
+			}
+			require.Equal(t, expectedReputation, rep)
+			require.Equal(t, 1, fs.requestQueue.Len())
+		})
+
+		t.Run("peer_B_announces_a_tracked_block", func(t *testing.T) {
+			sndPeer := peer.ID("snd-peer")
+			firstHandshake := &network.BlockAnnounceHandshake{
+				Roles:           1,
+				BestBlockNumber: 17,
+				BestBlockHash:   common.BytesToHash([]byte{0, 1, 2}),
+				GenesisHash:     common.BytesToHash([]byte{1, 1, 1, 1}),
+			}
+
+			err := fs.OnBlockAnnounceHandshake(sndPeer, firstHandshake)
+			require.NoError(t, err)
+
+			// the announced block 17 is already tracked by our node
+			// then we will ignore it
+			rep, err := fs.OnBlockAnnounce(sndPeer, announceOfBlock17)
+			require.ErrorIs(t, err, errPeerOnInvalidFork)
+
+			expectedReputation := &Change{
+				who: sndPeer,
+				rep: peerset.ReputationChange{
+					Value:  peerset.NotRelevantBlockAnnounceValue,
+					Reason: peerset.NotRelevantBlockAnnounceReason,
+				},
+			}
+			require.Equal(t, expectedReputation, rep)
+
+			// the queue should not change
+			require.Equal(t, 1, fs.requestQueue.Len())
+		})
+
+		t.Run("call_fullsync_next_actions_should_have_request_for_block_body", func(t *testing.T) {
+			refTo := func(v uint32) *uint32 {
+				return &v
+			}
+
+			tasks, err := fs.NextActions()
+			require.NoError(t, err)
+			require.Len(t, tasks, 2)
+
+			requests := make([]messages.P2PMessage, len(tasks))
+			for idx, task := range tasks {
+				requests[idx] = task.request
+			}
+
+			block17 := types.NewHeader(announceOfBlock17.ParentHash,
+				announceOfBlock17.StateRoot, announceOfBlock17.ExtrinsicsRoot,
+				announceOfBlock17.Number, announceOfBlock17.Digest)
+			block17Hash := block17.Hash()
+
+			expectedRequests := []messages.P2PMessage{
+				&messages.BlockRequestMessage{
+					RequestedData: messages.RequestedDataBody + messages.RequestedDataJustification,
+					StartingBlock: *variadic.Uint32OrHashFrom(block17Hash),
+					Direction:     messages.Ascending,
+					Max:           refTo(1),
+				},
+				&messages.BlockRequestMessage{
+					RequestedData: messages.BootstrapRequestData,
+					StartingBlock: *variadic.Uint32OrHashFrom(uint32(1)),
+					Direction:     messages.Ascending,
+					Max:           refTo(17),
+				},
+			}
+
+			require.Equal(t, expectedRequests, requests)
+		})
 	})
 
 }

dot/sync/peer_view.go

@@ -51,20 +51,15 @@ func (p *peerViewSet) getTarget() uint32 {
 		return p.target
 	}
 
-	numbers := make([]uint32, len(p.view))
+	currMax := p.target
 	// we are going to sort the data and remove the outliers then we will return the avg of all the valid elements
-	for idx, view := range maps.Values(p.view) {
-		numbers[idx] = view.bestBlockNumber
-	}
-
-	sum, count := nonOutliersSumCount(numbers)
-	quotientBigInt := uint32(big.NewInt(0).Div(sum, big.NewInt(int64(count))).Uint64())
-
-	if p.target >= quotientBigInt {
-		return p.target
+	for _, view := range maps.Values(p.view) {
+		if view.bestBlockNumber > currMax {
+			currMax = view.bestBlockNumber
+		}
 	}
 
-	p.target = quotientBigInt // cache latest calculated target
+	p.target = currMax // cache latest calculated target
 	return p.target
 }
 

dot/sync/service.go

@@ -69,7 +69,7 @@ type Change struct {
 }
 
 type Strategy interface {
-	OnBlockAnnounce(from peer.ID, msg *network.BlockAnnounceMessage) (gossip bool, repChange *Change, err error)
+	OnBlockAnnounce(from peer.ID, msg *network.BlockAnnounceMessage) (repChange *Change, err error)
 	OnBlockAnnounceHandshake(from peer.ID, msg *network.BlockAnnounceHandshake) error
 	NextActions() ([]*syncTask, error)
 	IsFinished(results []*syncTaskResult) (done bool, repChanges []Change, blocks []peer.ID, err error)
@@ -181,7 +181,7 @@ func (s *SyncService) HandleBlockAnnounce(from peer.ID, msg *network.BlockAnnoun
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	_, repChange, err := s.currentStrategy.OnBlockAnnounce(from, msg)
+	repChange, err := s.currentStrategy.OnBlockAnnounce(from, msg)
 	if err != nil {
 		return fmt.Errorf("while handling block announce: %w", err)
 	}