From 867a11f0d56f8702f27a32998366d4cdd3811bf0 Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Fri, 1 Mar 2024 11:43:19 +0200 Subject: [PATCH 1/9] return empty slice and an error if blobs were not found --- blob/service.go | 13 ++++++++----- blob/service_test.go | 8 ++++---- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/blob/service.go b/blob/service.go index 25bf48fd1f..c9afef397f 100644 --- a/blob/service.go +++ b/blob/service.go @@ -164,6 +164,7 @@ func (s *Service) GetProof( // GetAll returns all blobs under the given namespaces at the given height. // GetAll can return blobs and an error in case if some requests failed. +// GetAll can return both empty values in case if blobs were not found for the requested namespaces. func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share.Namespace) ([]*Blob, error) { header, err := s.headerGetter(ctx, height) if err != nil { @@ -186,7 +187,13 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. defer wg.Done() blobs, err := s.getBlobs(ctx, namespace, header) if err != nil { - resultErr[i] = fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) + resErr := fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) + if errors.Is(err, ErrBlobNotFound) { + log.Debug(resErr) + return + } + + resultErr[i] = resErr return } @@ -202,10 +209,6 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. blobs = append(blobs, resBlobs...) } } - - if len(blobs) == 0 { - resultErr = append(resultErr, ErrBlobNotFound) - } return blobs, errors.Join(resultErr...) } diff --git a/blob/service_test.go b/blob/service_test.go index 7a99e92e06..294bdd80ca 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -295,15 +295,15 @@ func TestBlobService_Get(t *testing.T) { { name: "get all not found", doFn: func() (interface{}, error) { - namespace := share.Namespace(tmrand.Bytes(share.NamespaceSize)) - return service.GetAll(ctx, 1, []share.Namespace{namespace}) + nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(7)) + require.NoError(t, err) + return service.GetAll(ctx, 1, []share.Namespace{nid}) }, expectedResult: func(i interface{}, err error) { blobs, ok := i.([]*Blob) require.True(t, ok) assert.Empty(t, blobs) - require.Error(t, err) - require.ErrorIs(t, err, ErrBlobNotFound) + require.NoError(t, err) }, }, { From f548d252d2e636c876800885efdbd74b7c54cd8e Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Thu, 7 Mar 2024 14:46:41 +0200 Subject: [PATCH 2/9] chore: rework returned values --- blob/service.go | 51 ++++++++++++++++++++++---------------------- blob/service_test.go | 6 +----- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/blob/service.go b/blob/service.go index c9afef397f..815251c831 100644 --- a/blob/service.go +++ b/blob/service.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "math" - "sync" sdkmath "cosmossdk.io/math" "github.com/cosmos/cosmos-sdk/types" @@ -164,7 +163,6 @@ func (s *Service) GetProof( // GetAll returns all blobs under the given namespaces at the given height. // GetAll can return blobs and an error in case if some requests failed. -// GetAll can return both empty values in case if blobs were not found for the requested namespaces. func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share.Namespace) ([]*Blob, error) { header, err := s.headerGetter(ctx, height) if err != nil { @@ -172,44 +170,47 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. } var ( - resultBlobs = make([][]*Blob, len(namespaces)) - resultErr = make([]error, len(namespaces)) - ) + blobs = make([]*Blob, 0) - for _, namespace := range namespaces { - log.Debugw("performing GetAll request", "namespace", namespace.String(), "height", height) - } + blobsCh = make(chan []*Blob, 1) + errCh = make(chan error, 1) + ) - wg := sync.WaitGroup{} for i, namespace := range namespaces { - wg.Add(1) go func(i int, namespace share.Namespace) { - defer wg.Done() + log.Debugw("retrieving all blobs from", "namespace", namespace.String(), "height", height) blobs, err := s.getBlobs(ctx, namespace, header) if err != nil { - resErr := fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) - if errors.Is(err, ErrBlobNotFound) { - log.Debug(resErr) - return - } - - resultErr[i] = resErr + err = fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) + errCh <- err + log.Debug(err) return } log.Debugw("receiving blobs", "height", height, "total", len(blobs)) - resultBlobs[i] = blobs + blobsCh <- blobs }(i, namespace) } - wg.Wait() - blobs := make([]*Blob, 0) - for _, resBlobs := range resultBlobs { - if len(resBlobs) > 0 { - blobs = append(blobs, resBlobs...) + for range namespaces { + select { + case <-ctx.Done(): + return blobs, ctx.Err() + case b := <-blobsCh: + blobs = append(blobs, b...) + case resultErr := <-errCh: + if errors.Is(resultErr, ErrBlobNotFound) { + continue + + } + err = errors.Join(err, resultErr) } } - return blobs, errors.Join(resultErr...) + + if len(blobs) == 0 && err == nil { + err = ErrBlobNotFound + } + return blobs, err } // Included verifies that the blob was included in a specific height. diff --git a/blob/service_test.go b/blob/service_test.go index 294bdd80ca..0c49dc5fcb 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -141,11 +141,7 @@ func TestBlobService_Get(t *testing.T) { blobs, ok := res.([]*Blob) assert.True(t, ok) assert.NotEmpty(t, blobs) - assert.Len(t, blobs, 2) - // check the order - require.True(t, bytes.Equal(blobs[0].Namespace(), blobs0[0].Namespace())) - require.True(t, bytes.Equal(blobs[1].Namespace(), blobs0[1].Namespace())) }, }, { @@ -303,7 +299,7 @@ func TestBlobService_Get(t *testing.T) { blobs, ok := i.([]*Blob) require.True(t, ok) assert.Empty(t, blobs) - require.NoError(t, err) + assert.ErrorIs(t, err, ErrBlobNotFound) }, }, { From 228dd53a4027492c74775db3ac0d95ae687d6d27 Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Fri, 10 May 2024 17:25:39 +0300 Subject: [PATCH 3/9] chore: return empty result and error when blobs were not found --- blob/service.go | 50 +++++++++++++--------------------- blob/service_test.go | 4 +-- nodebuilder/tests/blob_test.go | 6 ++-- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/blob/service.go b/blob/service.go index 815251c831..4795e64927 100644 --- a/blob/service.go +++ b/blob/service.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "math" + "slices" + "sync" sdkmath "cosmossdk.io/math" "github.com/cosmos/cosmos-sdk/types" @@ -170,46 +172,32 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. } var ( - blobs = make([]*Blob, 0) - - blobsCh = make(chan []*Blob, 1) - errCh = make(chan error, 1) + resultBlobs = make([][]*Blob, len(namespaces)) + resultErr = make([]error, len(namespaces)) + wg = sync.WaitGroup{} ) - for i, namespace := range namespaces { + wg.Add(1) go func(i int, namespace share.Namespace) { log.Debugw("retrieving all blobs from", "namespace", namespace.String(), "height", height) + defer wg.Done() + blobs, err := s.getBlobs(ctx, namespace, header) - if err != nil { - err = fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) - errCh <- err - log.Debug(err) - return + switch { + case err == nil: + log.Debugw("receiving blobs", "height", height, "total", len(blobs)) + resultBlobs[i] = blobs + case errors.Is(err, ErrBlobNotFound): + default: + log.Debug("getting blobs for namespace(%s): %w", namespace.String(), err) + resultErr[i] = err } - - log.Debugw("receiving blobs", "height", height, "total", len(blobs)) - blobsCh <- blobs }(i, namespace) } + wg.Wait() - for range namespaces { - select { - case <-ctx.Done(): - return blobs, ctx.Err() - case b := <-blobsCh: - blobs = append(blobs, b...) - case resultErr := <-errCh: - if errors.Is(resultErr, ErrBlobNotFound) { - continue - - } - err = errors.Join(err, resultErr) - } - } - - if len(blobs) == 0 && err == nil { - err = ErrBlobNotFound - } + blobs := slices.Concat(resultBlobs...) + err = errors.Join(resultErr...) return blobs, err } diff --git a/blob/service_test.go b/blob/service_test.go index 0c49dc5fcb..8ca1c81c59 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -289,7 +289,7 @@ func TestBlobService_Get(t *testing.T) { }, }, { - name: "get all not found", + name: "empty result and err when blobs wer not found ", doFn: func() (interface{}, error) { nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(7)) require.NoError(t, err) @@ -299,7 +299,7 @@ func TestBlobService_Get(t *testing.T) { blobs, ok := i.([]*Blob) require.True(t, ok) assert.Empty(t, blobs) - assert.ErrorIs(t, err, ErrBlobNotFound) + assert.Empty(t, err) }, }, { diff --git a/nodebuilder/tests/blob_test.go b/nodebuilder/tests/blob_test.go index 60af146f67..1bce13ee84 100644 --- a/nodebuilder/tests/blob_test.go +++ b/nodebuilder/tests/blob_test.go @@ -1,5 +1,3 @@ -//go:build blob || integration - package tests import ( @@ -127,6 +125,10 @@ func TestBlobModule(t *testing.T) { assert.Nil(t, b) require.Error(t, err) require.ErrorContains(t, err, blob.ErrBlobNotFound.Error()) + + blobs, err := fullClient.Blob.GetAll(ctx, height, []share.Namespace{newBlob.Namespace()}) + require.NoError(t, err) + assert.Empty(t, blobs) }, }, { From eb6991bd29706fa15325742ea05661c632000d47 Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Fri, 10 May 2024 17:32:08 +0300 Subject: [PATCH 4/9] extend test case --- blob/service_test.go | 7 ++++++- nodebuilder/tests/blob_test.go | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/blob/service_test.go b/blob/service_test.go index 8ca1c81c59..fcfeae537c 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -132,7 +132,9 @@ func TestBlobService_Get(t *testing.T) { { name: "get all with different namespaces", doFn: func() (interface{}, error) { - b, err := service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace(), blobs0[1].Namespace()}) + nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(7)) + require.NoError(t, err) + b, err := service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace(), nid, blobs0[1].Namespace()}) return b, err }, expectedResult: func(res interface{}, err error) { @@ -142,6 +144,9 @@ func TestBlobService_Get(t *testing.T) { assert.True(t, ok) assert.NotEmpty(t, blobs) assert.Len(t, blobs, 2) + // check the order + require.True(t, bytes.Equal(blobs[0].Namespace(), blobs0[0].Namespace())) + require.True(t, bytes.Equal(blobs[1].Namespace(), blobs0[1].Namespace())) }, }, { diff --git a/nodebuilder/tests/blob_test.go b/nodebuilder/tests/blob_test.go index 1bce13ee84..bd225d7612 100644 --- a/nodebuilder/tests/blob_test.go +++ b/nodebuilder/tests/blob_test.go @@ -1,3 +1,5 @@ +//go:build blob || integration + package tests import ( From 4b884247fb753e4beb851bce3f583c7a74296688 Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Mon, 13 May 2024 12:46:01 +0300 Subject: [PATCH 5/9] add comment --- blob/service.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/blob/service.go b/blob/service.go index 4795e64927..a362f3516f 100644 --- a/blob/service.go +++ b/blob/service.go @@ -164,7 +164,14 @@ func (s *Service) GetProof( } // GetAll returns all blobs under the given namespaces at the given height. -// GetAll can return blobs and an error in case if some requests failed. +// If all blobs were found without any errors, the user will receive a list of blobs. +// If the BlobService couldn't find any blobs under the requested namespaces, +// the user will receive an empty list of blobs along with an empty error. +// If some of the requested namespaces were not found, the user will receive all the found blobs and an empty error. +// If there were internal errors during some of the requests, +// the user will receive all found blobs along with a combined error message. +// +// All blobs will preserve the order of the namespaces that were requested. func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share.Namespace) ([]*Blob, error) { header, err := s.headerGetter(ctx, height) if err != nil { @@ -189,7 +196,7 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. resultBlobs[i] = blobs case errors.Is(err, ErrBlobNotFound): default: - log.Debug("getting blobs for namespace(%s): %w", namespace.String(), err) + log.Debugf("getting blobs for namespace(%s): %v", namespace.String(), err) resultErr[i] = err } }(i, namespace) From 734d18501b7aff6fff46e4a0d028e7f49474a1df Mon Sep 17 00:00:00 2001 From: Viacheslav Gonkivskyi Date: Mon, 13 May 2024 16:04:29 +0300 Subject: [PATCH 6/9] fix nits --- blob/service.go | 2 +- blob/service_test.go | 11 +++++++---- nodebuilder/blob/blob.go | 10 +++++++++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/blob/service.go b/blob/service.go index a362f3516f..458ea2f4a7 100644 --- a/blob/service.go +++ b/blob/service.go @@ -192,7 +192,7 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. blobs, err := s.getBlobs(ctx, namespace, header) switch { case err == nil: - log.Debugw("receiving blobs", "height", height, "total", len(blobs)) + log.Infow("retrieved blobs", "height", height, "total", len(blobs)) resultBlobs[i] = blobs case errors.Is(err, ErrBlobNotFound): default: diff --git a/blob/service_test.go b/blob/service_test.go index fcfeae537c..b65feb887a 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -16,9 +16,11 @@ import ( tmrand "github.com/tendermint/tendermint/libs/rand" "github.com/celestiaorg/celestia-app/pkg/appconsts" + appns "github.com/celestiaorg/celestia-app/pkg/namespace" "github.com/celestiaorg/celestia-app/pkg/shares" "github.com/celestiaorg/go-header/store" + "github.com/celestiaorg/celestia-node/blob/blobtest" "github.com/celestiaorg/celestia-node/header" "github.com/celestiaorg/celestia-node/header/headertest" @@ -294,9 +296,9 @@ func TestBlobService_Get(t *testing.T) { }, }, { - name: "empty result and err when blobs wer not found ", + name: "empty result and err when blobs were not found ", doFn: func() (interface{}, error) { - nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(7)) + nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(appns.NamespaceVersionZeroIDSize)) require.NoError(t, err) return service.GetAll(ctx, 1, []share.Namespace{nid}) }, @@ -523,8 +525,9 @@ func TestAllPaddingSharesInEDS(t *testing.T) { } service := NewService(nil, getters.NewIPLDGetter(bs), fn) - _, err = service.GetAll(ctx, 1, []share.Namespace{nid}) - require.Error(t, err) + newBlobs, err := service.GetAll(ctx, 1, []share.Namespace{nid}) + require.NoError(t, err) + assert.Empty(t, newBlobs) } func TestSkipPaddingsAndRetrieveBlob(t *testing.T) { diff --git a/nodebuilder/blob/blob.go b/nodebuilder/blob/blob.go index f87105541a..dccaff50ec 100644 --- a/nodebuilder/blob/blob.go +++ b/nodebuilder/blob/blob.go @@ -19,7 +19,15 @@ type Module interface { Submit(_ context.Context, _ []*blob.Blob, _ blob.GasPrice) (height uint64, _ error) // Get retrieves the blob by commitment under the given namespace and height. Get(_ context.Context, height uint64, _ share.Namespace, _ blob.Commitment) (*blob.Blob, error) - // GetAll returns all blobs at the given height under the given namespaces. + // GetAll returns all blobs under the given namespaces at the given height. + // If all blobs were found without any errors, the user will receive a list of blobs. + // If the BlobService couldn't find any blobs under the requested namespaces, + // the user will receive an empty list of blobs along with an empty error. + // If some of the requested namespaces were not found, the user will receive all the found blobs and an empty error. + // If there were internal errors during some of the requests, + // the user will receive all found blobs along with a combined error message. + // + // All blobs will preserve the order of the namespaces that were requested. GetAll(_ context.Context, height uint64, _ []share.Namespace) ([]*blob.Blob, error) // GetProof retrieves proofs in the given namespaces at the given height by commitment. GetProof(_ context.Context, height uint64, _ share.Namespace, _ blob.Commitment) (*blob.Proof, error) From a9398b7c5a55dbe91246cb8f2ec0fd13cbfc912c Mon Sep 17 00:00:00 2001 From: vgonkivs Date: Wed, 3 Jul 2024 12:59:50 +0300 Subject: [PATCH 7/9] improvements --- blob/service.go | 16 ++++++---------- blob/service_test.go | 24 ++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/blob/service.go b/blob/service.go index 458ea2f4a7..8105b88a27 100644 --- a/blob/service.go +++ b/blob/service.go @@ -190,15 +190,14 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. defer wg.Done() blobs, err := s.getBlobs(ctx, namespace, header) - switch { - case err == nil: - log.Infow("retrieved blobs", "height", height, "total", len(blobs)) - resultBlobs[i] = blobs - case errors.Is(err, ErrBlobNotFound): - default: + if err != nil && !errors.Is(err, ErrBlobNotFound) { log.Debugf("getting blobs for namespace(%s): %v", namespace.String(), err) resultErr[i] = err } + if len(blobs) > 0 { + log.Infow("retrieved blobs", "height", height, "total", len(blobs)) + resultBlobs[i] = blobs + } }(i, namespace) } wg.Wait() @@ -412,8 +411,5 @@ func (s *Service) getBlobs( sharesParser := &parser{verifyFn: verifyFn} _, _, err = s.retrieve(ctx, header.Height(), namespace, sharesParser) - if len(blobs) == 0 { - return nil, ErrBlobNotFound - } - return blobs, nil + return blobs, err } diff --git a/blob/service_test.go b/blob/service_test.go index b65feb887a..df82a1b781 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -4,11 +4,13 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "sort" "testing" "time" + "github.com/golang/mock/gomock" ds "github.com/ipfs/go-datastore" ds_sync "github.com/ipfs/go-datastore/sync" "github.com/stretchr/testify/assert" @@ -20,10 +22,10 @@ import ( "github.com/celestiaorg/celestia-app/pkg/shares" "github.com/celestiaorg/go-header/store" - "github.com/celestiaorg/celestia-node/blob/blobtest" "github.com/celestiaorg/celestia-node/header" "github.com/celestiaorg/celestia-node/header/headertest" + shareMock "github.com/celestiaorg/celestia-node/nodebuilder/share/mocks" "github.com/celestiaorg/celestia-node/share" "github.com/celestiaorg/celestia-node/share/getters" "github.com/celestiaorg/celestia-node/share/ipld" @@ -306,7 +308,7 @@ func TestBlobService_Get(t *testing.T) { blobs, ok := i.([]*Blob) require.True(t, ok) assert.Empty(t, blobs) - assert.Empty(t, err) + assert.Nil(t, err) }, }, { @@ -328,6 +330,24 @@ func TestBlobService_Get(t *testing.T) { require.NoError(t, proof.equal(*newProof)) }, }, + { + name: "internal error", + doFn: func() (interface{}, error) { + ctrl := gomock.NewController(t) + shareGetterMock := shareMock.NewMockModule(ctrl) + service.shareGetter = shareGetterMock + shareGetterMock.EXPECT(). + GetSharesByNamespace(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("internal error")) + return service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace()}) + }, + expectedResult: func(res interface{}, err error) { + blobs, ok := res.([]*Blob) + require.True(t, ok) + assert.Empty(t, blobs) + require.Error(t, err) + }, + }, } for _, tt := range test { From 9d73b73a9082aa0c5b3bc3c27e85c79e6e84edb4 Mon Sep 17 00:00:00 2001 From: vgonkivs Date: Wed, 3 Jul 2024 16:50:34 +0300 Subject: [PATCH 8/9] improve tests --- blob/service.go | 2 +- blob/service_test.go | 25 ++++++++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/blob/service.go b/blob/service.go index 8105b88a27..ee1ef73bcc 100644 --- a/blob/service.go +++ b/blob/service.go @@ -191,7 +191,7 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. blobs, err := s.getBlobs(ctx, namespace, header) if err != nil && !errors.Is(err, ErrBlobNotFound) { - log.Debugf("getting blobs for namespace(%s): %v", namespace.String(), err) + log.Errorf("getting blobs for namespaceID(%s): %v", namespace.ID().String(), err) resultErr[i] = err } if len(blobs) > 0 { diff --git a/blob/service_test.go b/blob/service_test.go index df82a1b781..672ce091c1 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -307,7 +307,7 @@ func TestBlobService_Get(t *testing.T) { expectedResult: func(i interface{}, err error) { blobs, ok := i.([]*Blob) require.True(t, ok) - assert.Empty(t, blobs) + assert.Nil(t, blobs) assert.Nil(t, err) }, }, @@ -334,18 +334,29 @@ func TestBlobService_Get(t *testing.T) { name: "internal error", doFn: func() (interface{}, error) { ctrl := gomock.NewController(t) + shareService := service.shareGetter shareGetterMock := shareMock.NewMockModule(ctrl) - service.shareGetter = shareGetterMock shareGetterMock.EXPECT(). GetSharesByNamespace(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil, errors.New("internal error")) - return service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace()}) + DoAndReturn( + func(ctx context.Context, h *header.ExtendedHeader, ns share.Namespace) (share.NamespacedShares, error) { + if ns.Equals(blobs0[0].Namespace()) { + return nil, errors.New("internal error") + } + return shareService.GetSharesByNamespace(ctx, h, ns) + }).AnyTimes() + + service.shareGetter = shareGetterMock + return service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace(), blobs1[0].Namespace()}) }, expectedResult: func(res interface{}, err error) { blobs, ok := res.([]*Blob) - require.True(t, ok) - assert.Empty(t, blobs) - require.Error(t, err) + assert.True(t, ok) + assert.Error(t, err) + assert.Contains(t, err.Error(), "internal error") + assert.Equal(t, blobs[0].Namespace(), blobs1[0].Namespace()) + assert.NotEmpty(t, blobs) + assert.Len(t, blobs, len(blobs1)) }, }, } From fb043fa13cdccddc339d835479a5ff13c5008104 Mon Sep 17 00:00:00 2001 From: vgonkivs Date: Wed, 3 Jul 2024 19:11:39 +0300 Subject: [PATCH 9/9] fix nits --- blob/service_test.go | 118 +++++++++++++++++++++++++++++++------------ 1 file changed, 86 insertions(+), 32 deletions(-) diff --git a/blob/service_test.go b/blob/service_test.go index 672ce091c1..fc48f96eb9 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -43,15 +43,15 @@ func TestBlobService_Get(t *testing.T) { appBlobs, err := blobtest.GenerateV0Blobs([]int{blobSize0, blobSize1}, false) require.NoError(t, err) - blobs0, err := convertBlobs(appBlobs...) + blobsWithDiffNamespaces, err := convertBlobs(appBlobs...) require.NoError(t, err) appBlobs, err = blobtest.GenerateV0Blobs([]int{blobSize2, blobSize3}, true) require.NoError(t, err) - blobs1, err := convertBlobs(appBlobs...) + blobsWithSameNamespace, err := convertBlobs(appBlobs...) require.NoError(t, err) - service := createService(ctx, t, append(blobs0, blobs1...)) + service := createService(ctx, t, append(blobsWithDiffNamespaces, blobsWithSameNamespace...)) test := []struct { name string doFn func() (interface{}, error) @@ -60,7 +60,10 @@ func TestBlobService_Get(t *testing.T) { { name: "get single blob", doFn: func() (interface{}, error) { - b, err := service.Get(ctx, 1, blobs0[0].Namespace(), blobs0[0].Commitment) + b, err := service.Get(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + blobsWithDiffNamespaces[0].Commitment, + ) return []*Blob{b}, err }, expectedResult: func(res interface{}, err error) { @@ -71,13 +74,13 @@ func TestBlobService_Get(t *testing.T) { assert.True(t, ok) assert.Len(t, blobs, 1) - assert.Equal(t, blobs0[0].Commitment, blobs[0].Commitment) + assert.Equal(t, blobsWithDiffNamespaces[0].Commitment, blobs[0].Commitment) }, }, { name: "get all with the same namespace", doFn: func() (interface{}, error) { - return service.GetAll(ctx, 1, []share.Namespace{blobs1[0].Namespace()}) + return service.GetAll(ctx, 1, []share.Namespace{blobsWithSameNamespace[0].Namespace()}) }, expectedResult: func(res interface{}, err error) { require.NoError(t, err) @@ -88,19 +91,25 @@ func TestBlobService_Get(t *testing.T) { assert.Len(t, blobs, 2) - for i := range blobs1 { - require.Equal(t, blobs1[i].Commitment, blobs[i].Commitment) + for i := range blobsWithSameNamespace { + require.Equal(t, blobsWithSameNamespace[i].Commitment, blobs[i].Commitment) } }, }, { name: "verify indexes", doFn: func() (interface{}, error) { - b0, err := service.Get(ctx, 1, blobs0[0].Namespace(), blobs0[0].Commitment) + b0, err := service.Get(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + blobsWithDiffNamespaces[0].Commitment, + ) require.NoError(t, err) - b1, err := service.Get(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + b1, err := service.Get(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) require.NoError(t, err) - b23, err := service.GetAll(ctx, 1, []share.Namespace{blobs1[0].Namespace()}) + b23, err := service.GetAll(ctx, 1, []share.Namespace{blobsWithSameNamespace[0].Namespace()}) require.NoError(t, err) return []*Blob{b0, b1, b23[0], b23[1]}, nil }, @@ -138,7 +147,12 @@ func TestBlobService_Get(t *testing.T) { doFn: func() (interface{}, error) { nid, err := share.NewBlobNamespaceV0(tmrand.Bytes(7)) require.NoError(t, err) - b, err := service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace(), nid, blobs0[1].Namespace()}) + b, err := service.GetAll(ctx, 1, + []share.Namespace{ + blobsWithDiffNamespaces[0].Namespace(), nid, + blobsWithDiffNamespaces[1].Namespace(), + }, + ) return b, err }, expectedResult: func(res interface{}, err error) { @@ -149,14 +163,17 @@ func TestBlobService_Get(t *testing.T) { assert.NotEmpty(t, blobs) assert.Len(t, blobs, 2) // check the order - require.True(t, bytes.Equal(blobs[0].Namespace(), blobs0[0].Namespace())) - require.True(t, bytes.Equal(blobs[1].Namespace(), blobs0[1].Namespace())) + require.True(t, bytes.Equal(blobs[0].Namespace(), blobsWithDiffNamespaces[0].Namespace())) + require.True(t, bytes.Equal(blobs[1].Namespace(), blobsWithDiffNamespaces[1].Namespace())) }, }, { name: "get blob with incorrect commitment", doFn: func() (interface{}, error) { - b, err := service.Get(ctx, 1, blobs0[0].Namespace(), blobs0[1].Commitment) + b, err := service.Get(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) return []*Blob{b}, err }, expectedResult: func(res interface{}, err error) { @@ -189,7 +206,10 @@ func TestBlobService_Get(t *testing.T) { { name: "get proof", doFn: func() (interface{}, error) { - proof, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + proof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) return proof, err }, expectedResult: func(res interface{}, err error) { @@ -216,17 +236,24 @@ func TestBlobService_Get(t *testing.T) { t.Fatal("could not prove the shares") } - rawShares, err := BlobsToShares(blobs0[1]) + rawShares, err := BlobsToShares(blobsWithDiffNamespaces[1]) require.NoError(t, err) - verifyFn(t, rawShares, proof, blobs0[1].Namespace()) + verifyFn(t, rawShares, proof, blobsWithDiffNamespaces[1].Namespace()) }, }, { name: "verify inclusion", doFn: func() (interface{}, error) { - proof, err := service.GetProof(ctx, 1, blobs0[0].Namespace(), blobs0[0].Commitment) + proof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + blobsWithDiffNamespaces[0].Commitment, + ) require.NoError(t, err) - return service.Included(ctx, 1, blobs0[0].Namespace(), proof, blobs0[0].Commitment) + return service.Included(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + proof, + blobsWithDiffNamespaces[0].Commitment, + ) }, expectedResult: func(res interface{}, err error) { require.NoError(t, err) @@ -238,9 +265,16 @@ func TestBlobService_Get(t *testing.T) { { name: "verify inclusion fails with different proof", doFn: func() (interface{}, error) { - proof, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + proof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) require.NoError(t, err) - return service.Included(ctx, 1, blobs0[0].Namespace(), proof, blobs0[0].Commitment) + return service.Included(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + proof, + blobsWithDiffNamespaces[0].Commitment, + ) }, expectedResult: func(res interface{}, err error) { require.Error(t, err) @@ -258,7 +292,10 @@ func TestBlobService_Get(t *testing.T) { blob, err := convertBlobs(appBlob...) require.NoError(t, err) - proof, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + proof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) require.NoError(t, err) return service.Included(ctx, 1, blob[0].Namespace(), proof, blob[0].Commitment) }, @@ -272,11 +309,17 @@ func TestBlobService_Get(t *testing.T) { { name: "count proofs for the blob", doFn: func() (interface{}, error) { - proof0, err := service.GetProof(ctx, 1, blobs0[0].Namespace(), blobs0[0].Commitment) + proof0, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[0].Namespace(), + blobsWithDiffNamespaces[0].Commitment, + ) if err != nil { return nil, err } - proof1, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + proof1, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) if err != nil { return nil, err } @@ -308,13 +351,16 @@ func TestBlobService_Get(t *testing.T) { blobs, ok := i.([]*Blob) require.True(t, ok) assert.Nil(t, blobs) - assert.Nil(t, err) + assert.NoError(t, err) }, }, { name: "marshal proof", doFn: func() (interface{}, error) { - proof, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + proof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) require.NoError(t, err) return json.Marshal(proof) }, @@ -325,7 +371,10 @@ func TestBlobService_Get(t *testing.T) { var proof Proof require.NoError(t, json.Unmarshal(jsonData, &proof)) - newProof, err := service.GetProof(ctx, 1, blobs0[1].Namespace(), blobs0[1].Commitment) + newProof, err := service.GetProof(ctx, 1, + blobsWithDiffNamespaces[1].Namespace(), + blobsWithDiffNamespaces[1].Commitment, + ) require.NoError(t, err) require.NoError(t, proof.equal(*newProof)) }, @@ -340,23 +389,28 @@ func TestBlobService_Get(t *testing.T) { GetSharesByNamespace(gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn( func(ctx context.Context, h *header.ExtendedHeader, ns share.Namespace) (share.NamespacedShares, error) { - if ns.Equals(blobs0[0].Namespace()) { + if ns.Equals(blobsWithDiffNamespaces[0].Namespace()) { return nil, errors.New("internal error") } return shareService.GetSharesByNamespace(ctx, h, ns) }).AnyTimes() service.shareGetter = shareGetterMock - return service.GetAll(ctx, 1, []share.Namespace{blobs0[0].Namespace(), blobs1[0].Namespace()}) + return service.GetAll(ctx, 1, + []share.Namespace{ + blobsWithDiffNamespaces[0].Namespace(), + blobsWithSameNamespace[0].Namespace(), + }, + ) }, expectedResult: func(res interface{}, err error) { blobs, ok := res.([]*Blob) assert.True(t, ok) assert.Error(t, err) assert.Contains(t, err.Error(), "internal error") - assert.Equal(t, blobs[0].Namespace(), blobs1[0].Namespace()) + assert.Equal(t, blobs[0].Namespace(), blobsWithSameNamespace[0].Namespace()) assert.NotEmpty(t, blobs) - assert.Len(t, blobs, len(blobs1)) + assert.Len(t, blobs, len(blobsWithSameNamespace)) }, }, }