From 4270ed6830a6dfd824a1cad742647f81ea30b1df Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Fri, 15 Nov 2024 18:39:34 +0100 Subject: [PATCH 1/2] fix(share/availability/light): Allow retry on ODS size 1 --- share/availability/light/availability.go | 2 +- share/availability/light/availability_test.go | 100 +++++++++++------- 2 files changed, 64 insertions(+), 38 deletions(-) diff --git a/share/availability/light/availability.go b/share/availability/light/availability.go index 8fd0449065..6a23abe3d9 100644 --- a/share/availability/light/availability.go +++ b/share/availability/light/availability.go @@ -103,7 +103,7 @@ func (la *ShareAvailability) SharesAvailable(ctx context.Context, header *header } // Verify total samples count. totalSamples := len(samples.Remaining) + len(samples.Available) - if totalSamples != int(la.params.SampleAmount) { + if (totalSamples != int(la.params.SampleAmount)) && (totalSamples != len(dah.RowRoots)*len(dah.RowRoots)) { return fmt.Errorf("invalid sampling result:"+ " expected %d samples, got %d", la.params.SampleAmount, totalSamples) } diff --git a/share/availability/light/availability_test.go b/share/availability/light/availability_test.go index 5ec6e8e39e..5f24a511da 100644 --- a/share/availability/light/availability_test.go +++ b/share/availability/light/availability_test.go @@ -139,53 +139,79 @@ func TestSharesAvailableFailed(t *testing.T) { ds := datastore.NewMapDatastore() avail := NewShareAvailability(getter, ds, nil) - // Create new eds, that is not available by getter - eds := edstest.RandEDS(t, 16) - roots, err := share.NewAxisRoots(eds) - require.NoError(t, err) - eh := headertest.RandExtendedHeaderWithRoot(t, roots) + type test struct { + eh *header.ExtendedHeader + roots *share.AxisRoots + eds *rsmt2d.ExtendedDataSquare + } - // Getter doesn't have the eds, so it should fail for all samples - getter.EXPECT(). - GetShare(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(libshare.Share{}, shrex.ErrNotFound). - AnyTimes() - err = avail.SharesAvailable(ctx, eh) - require.ErrorIs(t, err, share.ErrNotAvailable) + tests := make([]test, 0) - // The datastore should now contain the sampling result with all samples in Remaining - data, err := avail.ds.Get(ctx, datastoreKeyForRoot(roots)) - require.NoError(t, err) + for i := 1; i <= 16; i *= 4 { + t.Log(i) + // Create new eds, that is not available by getter + eds := edstest.RandEDS(t, i) + roots, err := share.NewAxisRoots(eds) + require.NoError(t, err) + eh := headertest.RandExtendedHeaderWithRoot(t, roots) - var failed SamplingResult - err = json.Unmarshal(data, &failed) - require.NoError(t, err) + tests = append(tests, test{eh: eh, roots: roots, eds: eds}) + t.Log("i: ", i, " roothash: ", eh.DAH.String()) + } - require.Empty(t, failed.Available) - require.Len(t, failed.Remaining, int(avail.params.SampleAmount)) + for _, tt := range tests { + // Getter doesn't have the eds, so it should fail for all samples + getter.EXPECT(). + GetShare(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(libshare.Share{}, shrex.ErrNotFound). + AnyTimes() + + err := avail.SharesAvailable(ctx, tt.eh) + t.Log("err: ", err) + require.ErrorIs(t, err, share.ErrNotAvailable) + + // The datastore should now contain the sampling result with all samples in Remaining + data, err := avail.ds.Get(ctx, datastoreKeyForRoot(tt.roots)) + require.NoError(t, err) + + var failed SamplingResult + err = json.Unmarshal(data, &failed) + require.NoError(t, err) + + require.Empty(t, failed.Available) + if len(tt.roots.RowRoots)*len(tt.roots.RowRoots) < int(avail.params.SampleAmount) { + require.Len(t, failed.Remaining, len(tt.roots.RowRoots)*len(tt.roots.RowRoots)) + } else { + require.Len(t, failed.Remaining, int(avail.params.SampleAmount)) + } - // Simulate a getter that now returns shares successfully - successfulGetter := newOnceGetter() - avail.getter = successfulGetter + // Simulate a getter that now returns shares successfully + successfulGetter := newOnceGetter() + avail.getter = successfulGetter - // should be able to retrieve all the failed samples now - err = avail.SharesAvailable(ctx, eh) - require.NoError(t, err) + // should be able to retrieve all the failed samples now + err = avail.SharesAvailable(ctx, tt.eh) + require.NoError(t, err) - // The sampling result should now have all samples in Available - data, err = avail.ds.Get(ctx, datastoreKeyForRoot(roots)) - require.NoError(t, err) + // The sampling result should now have all samples in Available + data, err = avail.ds.Get(ctx, datastoreKeyForRoot(tt.roots)) + require.NoError(t, err) - var result SamplingResult - err = json.Unmarshal(data, &result) - require.NoError(t, err) + var result SamplingResult + err = json.Unmarshal(data, &result) + require.NoError(t, err) - require.Empty(t, result.Remaining) - require.Len(t, result.Available, int(avail.params.SampleAmount)) + require.Empty(t, result.Remaining) + if len(tt.roots.RowRoots)*len(tt.roots.RowRoots) < int(avail.params.SampleAmount) { + require.Len(t, result.Available, len(tt.roots.RowRoots)*len(tt.roots.RowRoots)) + } else { + require.Len(t, result.Available, int(avail.params.SampleAmount)) + } - // onceGetter should have no more samples stored after the call - successfulGetter.checkOnce(t) - require.ElementsMatch(t, failed.Remaining, successfulGetter.sampledList()) + // onceGetter should have no more samples stored after the call + successfulGetter.checkOnce(t) + require.ElementsMatch(t, failed.Remaining, successfulGetter.sampledList()) + } } func TestParallelAvailability(t *testing.T) { From 109a3c77218940116e5fec28f5e46a23357354ac Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:34:01 +0100 Subject: [PATCH 2/2] fix(share/availability/light): fix test for failed share recovery --- share/availability/light/availability_test.go | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/share/availability/light/availability_test.go b/share/availability/light/availability_test.go index 5f24a511da..64834d46b5 100644 --- a/share/availability/light/availability_test.go +++ b/share/availability/light/availability_test.go @@ -135,9 +135,15 @@ func TestSharesAvailableFailed(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - getter := mock.NewMockGetter(gomock.NewController(t)) + failGetter := mock.NewMockGetter(gomock.NewController(t)) + // Getter doesn't have the eds, so it should fail for all samples + failGetter.EXPECT(). + GetShare(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(libshare.Share{}, shrex.ErrNotFound). + AnyTimes() + ds := datastore.NewMapDatastore() - avail := NewShareAvailability(getter, ds, nil) + avail := NewShareAvailability(failGetter, ds, nil) type test struct { eh *header.ExtendedHeader @@ -147,8 +153,7 @@ func TestSharesAvailableFailed(t *testing.T) { tests := make([]test, 0) - for i := 1; i <= 16; i *= 4 { - t.Log(i) + for i := 1; i <= 128; i *= 4 { // Create new eds, that is not available by getter eds := edstest.RandEDS(t, i) roots, err := share.NewAxisRoots(eds) @@ -156,18 +161,12 @@ func TestSharesAvailableFailed(t *testing.T) { eh := headertest.RandExtendedHeaderWithRoot(t, roots) tests = append(tests, test{eh: eh, roots: roots, eds: eds}) - t.Log("i: ", i, " roothash: ", eh.DAH.String()) } for _, tt := range tests { - // Getter doesn't have the eds, so it should fail for all samples - getter.EXPECT(). - GetShare(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(libshare.Share{}, shrex.ErrNotFound). - AnyTimes() + avail.getter = failGetter err := avail.SharesAvailable(ctx, tt.eh) - t.Log("err: ", err) require.ErrorIs(t, err, share.ErrNotAvailable) // The datastore should now contain the sampling result with all samples in Remaining