From 7c867959e3ef368181e456559a27af9acdc48c64 Mon Sep 17 00:00:00 2001 From: Viacheslav Date: Thu, 4 Jul 2024 15:39:05 +0300 Subject: [PATCH] misc(blob/service)!: return single ErrNotFound in GetAll (#3223) Resolves #3192 --- blob/service.go | 51 +++++------ blob/service_test.go | 161 +++++++++++++++++++++++++-------- nodebuilder/blob/blob.go | 10 +- nodebuilder/tests/blob_test.go | 4 + 4 files changed, 161 insertions(+), 65 deletions(-) diff --git a/blob/service.go b/blob/service.go index 25bf48fd1f..ee1ef73bcc 100644 --- a/blob/service.go +++ b/blob/service.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "slices" "sync" sdkmath "cosmossdk.io/math" @@ -163,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 { @@ -173,40 +181,30 @@ func (s *Service) GetAll(ctx context.Context, height uint64, namespaces []share. var ( resultBlobs = make([][]*Blob, len(namespaces)) resultErr = make([]error, len(namespaces)) + wg = sync.WaitGroup{} ) - - for _, namespace := range namespaces { - log.Debugw("performing GetAll request", "namespace", namespace.String(), "height", height) - } - - 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 { - resultErr[i] = fmt.Errorf("getting blobs for namespace(%s): %w", namespace.String(), err) - return + if err != nil && !errors.Is(err, ErrBlobNotFound) { + log.Errorf("getting blobs for namespaceID(%s): %v", namespace.ID().String(), err) + resultErr[i] = err + } + if len(blobs) > 0 { + log.Infow("retrieved blobs", "height", height, "total", len(blobs)) + resultBlobs[i] = blobs } - - log.Debugw("receiving blobs", "height", height, "total", len(blobs)) - resultBlobs[i] = blobs }(i, namespace) } wg.Wait() - blobs := make([]*Blob, 0) - for _, resBlobs := range resultBlobs { - if len(resBlobs) > 0 { - blobs = append(blobs, resBlobs...) - } - } - - if len(blobs) == 0 { - resultErr = append(resultErr, ErrBlobNotFound) - } - return blobs, errors.Join(resultErr...) + blobs := slices.Concat(resultBlobs...) + err = errors.Join(resultErr...) + return blobs, err } // Included verifies that the blob was included in a specific height. @@ -413,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 7a99e92e06..fc48f96eb9 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" @@ -16,12 +18,14 @@ 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" + 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" @@ -39,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) @@ -56,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) { @@ -67,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) @@ -84,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 }, @@ -132,7 +145,14 @@ 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{ + blobsWithDiffNamespaces[0].Namespace(), nid, + blobsWithDiffNamespaces[1].Namespace(), + }, + ) return b, err }, expectedResult: func(res interface{}, err error) { @@ -141,17 +161,19 @@ 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())) + 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) { @@ -184,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) { @@ -211,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) @@ -233,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) @@ -253,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) }, @@ -267,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 } @@ -293,23 +341,26 @@ func TestBlobService_Get(t *testing.T) { }, }, { - name: "get all not found", + name: "empty result and err when blobs were 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(appns.NamespaceVersionZeroIDSize)) + 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) + assert.Nil(t, blobs) + 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) }, @@ -320,11 +371,48 @@ 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)) }, }, + { + name: "internal error", + doFn: func() (interface{}, error) { + ctrl := gomock.NewController(t) + shareService := service.shareGetter + shareGetterMock := shareMock.NewMockModule(ctrl) + shareGetterMock.EXPECT(). + GetSharesByNamespace(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn( + func(ctx context.Context, h *header.ExtendedHeader, ns share.Namespace) (share.NamespacedShares, error) { + 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{ + 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(), blobsWithSameNamespace[0].Namespace()) + assert.NotEmpty(t, blobs) + assert.Len(t, blobs, len(blobsWithSameNamespace)) + }, + }, } for _, tt := range test { @@ -522,8 +610,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) diff --git a/nodebuilder/tests/blob_test.go b/nodebuilder/tests/blob_test.go index 60af146f67..bd225d7612 100644 --- a/nodebuilder/tests/blob_test.go +++ b/nodebuilder/tests/blob_test.go @@ -127,6 +127,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) }, }, {