From 330444c8396d979ce090f01da3e4e0782abd5605 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Fri, 13 Sep 2024 16:09:47 -0400 Subject: [PATCH] chore: fix `removeIrrelevantFragments` out of bounds panic --- dot/sync/mocks_test.go | 12 +- dot/sync/unready_blocks.go | 16 +- dot/sync/unready_blocks_test.go | 180 +++++++++++++++-------- scripts/retrieve_block/retrieve_block.go | 1 - 4 files changed, 132 insertions(+), 77 deletions(-) diff --git a/dot/sync/mocks_test.go b/dot/sync/mocks_test.go index 1a3c3d00bd..e006ce4493 100644 --- a/dot/sync/mocks_test.go +++ b/dot/sync/mocks_test.go @@ -692,16 +692,16 @@ func (mr *MockNetworkMockRecorder) GetRequestResponseProtocol(arg0, arg1, arg2 a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestResponseProtocol", reflect.TypeOf((*MockNetwork)(nil).GetRequestResponseProtocol), arg0, arg1, arg2) } -// GossipMessage mocks base method. -func (m *MockNetwork) GossipMessage(arg0 network.NotificationsMessage) { +// GossipMessageExcluding mocks base method. +func (m *MockNetwork) GossipMessageExcluding(arg0 network.NotificationsMessage, arg1 peer.ID) { m.ctrl.T.Helper() - m.ctrl.Call(m, "GossipMessage", arg0) + m.ctrl.Call(m, "GossipMessageExcluding", arg0, arg1) } -// GossipMessage indicates an expected call of GossipMessage. -func (mr *MockNetworkMockRecorder) GossipMessage(arg0 any) *gomock.Call { +// GossipMessageExcluding indicates an expected call of GossipMessageExcluding. +func (mr *MockNetworkMockRecorder) GossipMessageExcluding(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GossipMessage", reflect.TypeOf((*MockNetwork)(nil).GossipMessage), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GossipMessageExcluding", reflect.TypeOf((*MockNetwork)(nil).GossipMessageExcluding), arg0, arg1) } // ReportPeer mocks base method. diff --git a/dot/sync/unready_blocks.go b/dot/sync/unready_blocks.go index ac6a8a2cd7..0baaba9382 100644 --- 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 index 74280c8c86..b435bf6d1a 100644 --- 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 index 219f4f0b24..ef19f5124d 100644 --- 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())