chore: fix removeIrrelevantFragments out of bounds panic

ChainSafe · Sep 13, 2024 · 330444c · 330444c
1 parent f233d05
commit 330444c
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 77 deletions.
diff --git a/dot/sync/mocks_test.go b/dot/sync/mocks_test.go
diff --git a/dot/sync/unready_blocks.go b/dot/sync/unready_blocks.go
@@ -138,29 +138,25 @@ func (u *unreadyBlocks) removeIrrelevantFragments(finalisedNumber uint) {
 		return value.Header.Number <= finalisedNumber
 	})
 
-	idxsToRemove := make([]int, 0, len(u.disjointFragments))
-	for fragmentIdx, fragment := range u.disjointFragments {
+	fragmentIdx := 0
+	for _, fragment := range u.disjointFragments {
 		// the fragments are sorted in ascending order
 		// starting from the latest item and going backwards
 		// we have a higher chance to find the idx that has
 		// a block with number lower or equal the finalised one
 		idx := len(fragment) - 1
-		for idx >= 0 {
+		for ; idx >= 0; idx-- {
 			if fragment[idx].Header.Number <= finalisedNumber {
 				break
 			}
-			idx--
 		}
 
 		updatedFragment := fragment[idx+1:]
-		if len(updatedFragment) == 0 {
-			idxsToRemove = append(idxsToRemove, fragmentIdx)
-		} else {
+		if len(updatedFragment) != 0 {
 			u.disjointFragments[fragmentIdx] = updatedFragment
+			fragmentIdx++
 		}
 	}
 
-	for _, idx := range idxsToRemove {
-		u.disjointFragments = append(u.disjointFragments[:idx], u.disjointFragments[idx+1:]...)
-	}
+	u.disjointFragments = u.disjointFragments[:fragmentIdx]
 }
diff --git a/dot/sync/unready_blocks_test.go b/dot/sync/unready_blocks_test.go
@@ -8,37 +8,109 @@ import (
 )
 
 func TestUnreadyBlocks_removeIrrelevantFragments(t *testing.T) {
-	ub := newUnreadyBlocks()
-	ub.disjointFragments = [][]*types.BlockData{
-		// first fragment
-		{
+	t.Run("removing all disjoint fragment", func(t *testing.T) {
+		ub := newUnreadyBlocks()
+		ub.disjointFragments = [][]*types.BlockData{
 			{
-				Header: &types.Header{
-					Number: 192,
+				{
+					Header: &types.Header{
+						Number: 100,
+					},
 				},
 			},
-
 			{
-				Header: &types.Header{
-					Number: 191,
+				{
+					Header: &types.Header{
+						Number: 99,
+					},
+				},
+			},
+			{
+				{
+					Header: &types.Header{
+						Number: 92,
+					},
+				},
+			},
+		}
+		ub.removeIrrelevantFragments(100)
+		require.Empty(t, ub.disjointFragments)
+	})
+
+	t.Run("removing irrelevant fragments", func(t *testing.T) {
+		ub := newUnreadyBlocks()
+		ub.disjointFragments = [][]*types.BlockData{
+			// first fragment
+			{
+				{
+					Header: &types.Header{
+						Number: 192,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 191,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 190,
+					},
 				},
 			},
 
+			// second fragment
 			{
-				Header: &types.Header{
-					Number: 190,
+				{
+					Header: &types.Header{
+						Number: 253,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 254,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 255,
+					},
 				},
 			},
-		},
 
-		// second fragment
-		{
+			// third fragment
 			{
-				Header: &types.Header{
-					Number: 253,
+				{
+					Header: &types.Header{
+						Number: 1022,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 1023,
+					},
+				},
+
+				{
+					Header: &types.Header{
+						Number: 1024,
+					},
 				},
 			},
+		}
 
+		// the first fragment should be removed
+		// the second fragment should have only 2 items
+		// the third frament shold not be affected
+		ub.removeIrrelevantFragments(253)
+		require.Len(t, ub.disjointFragments, 2)
+
+		expectedSecondFrag := []*types.BlockData{
 			{
 				Header: &types.Header{
 					Number: 254,
@@ -50,10 +122,9 @@ func TestUnreadyBlocks_removeIrrelevantFragments(t *testing.T) {
 					Number: 255,
 				},
 			},
-		},
+		}
 
-		// third fragment
-		{
+		expectedThirdFragment := []*types.BlockData{
 			{
 				Header: &types.Header{
 					Number: 1022,
@@ -71,48 +142,37 @@ func TestUnreadyBlocks_removeIrrelevantFragments(t *testing.T) {
 					Number: 1024,
 				},
 			},
-		},
-	}
-
-	// the first fragment should be removed
-	// the second fragment should have only 2 items
-	// the third frament shold not be affected
-	ub.removeIrrelevantFragments(253)
-	require.Len(t, ub.disjointFragments, 2)
-
-	expectedSecondFrag := []*types.BlockData{
-		{
-			Header: &types.Header{
-				Number: 254,
-			},
-		},
-
-		{
-			Header: &types.Header{
-				Number: 255,
-			},
-		},
-	}
-
-	expectedThirdFragment := []*types.BlockData{
-		{
-			Header: &types.Header{
-				Number: 1022,
+		}
+		require.Equal(t, ub.disjointFragments[0], expectedSecondFrag)
+		require.Equal(t, ub.disjointFragments[1], expectedThirdFragment)
+	})
+
+	t.Run("keep all fragments", func(t *testing.T) {
+		ub := newUnreadyBlocks()
+		ub.disjointFragments = [][]*types.BlockData{
+			{
+				{
+					Header: &types.Header{
+						Number: 101,
+					},
+				},
 			},
-		},
-
-		{
-			Header: &types.Header{
-				Number: 1023,
+			{
+				{
+					Header: &types.Header{
+						Number: 103,
+					},
+				},
 			},
-		},
-
-		{
-			Header: &types.Header{
-				Number: 1024,
+			{
+				{
+					Header: &types.Header{
+						Number: 104,
+					},
+				},
 			},
-		},
-	}
-	require.Equal(t, ub.disjointFragments[0], expectedSecondFrag)
-	require.Equal(t, ub.disjointFragments[1], expectedThirdFragment)
+		}
+		ub.removeIrrelevantFragments(100)
+		require.Len(t, ub.disjointFragments, 3)
+	})
 }
diff --git a/scripts/retrieve_block/retrieve_block.go b/scripts/retrieve_block/retrieve_block.go
@@ -118,7 +118,6 @@ func main() {
 
 	protocolID := protocol.ID(fmt.Sprintf("/%s/sync/2", chain.ProtocolID))
 	for _, bootnodesAddr := range bootnodes {
-		log.Println("connecting...")
 		err := p2pHost.Connect(ctx, bootnodesAddr)
 		if err != nil {
 			log.Printf("fail with: %s\n", err.Error())