From cfb363cc44c81eb7964839b70d77ec2bcd5a51bd Mon Sep 17 00:00:00 2001 From: Vlad <13818348+walldiss@users.noreply.github.com> Date: Wed, 23 Oct 2024 11:52:02 +0200 Subject: [PATCH] !refactor(api/share): use height in share module (#3870) --- api/gateway/availability.go | 8 +-- api/gateway/share.go | 7 +-- nodebuilder/node_test.go | 33 ------------ nodebuilder/share/cmd/share.go | 34 +++---------- nodebuilder/share/mocks/api.go | 9 ++-- nodebuilder/share/share.go | 73 +++++++++++++++++---------- nodebuilder/tests/nd_test.go | 12 +++-- nodebuilder/tests/prune_test.go | 6 +-- nodebuilder/tests/reconstruct_test.go | 12 ++--- nodebuilder/tests/sync_test.go | 8 +-- 10 files changed, 80 insertions(+), 122 deletions(-) diff --git a/api/gateway/availability.go b/api/gateway/availability.go index e8e96083d5..b399b66483 100644 --- a/api/gateway/availability.go +++ b/api/gateway/availability.go @@ -27,13 +27,7 @@ func (h *Handler) handleHeightAvailabilityRequest(w http.ResponseWriter, r *http return } - header, err := h.header.GetByHeight(r.Context(), uint64(height)) - if err != nil { - writeError(w, http.StatusInternalServerError, heightAvailabilityEndpoint, err) - return - } - - err = h.share.SharesAvailable(r.Context(), header) + err = h.share.SharesAvailable(r.Context(), uint64(height)) switch { case err == nil: resp, err := json.Marshal(&AvailabilityResponse{Available: true}) diff --git a/api/gateway/share.go b/api/gateway/share.go index 25fcac8480..0927747765 100644 --- a/api/gateway/share.go +++ b/api/gateway/share.go @@ -93,12 +93,7 @@ func (h *Handler) getShares( height uint64, namespace libshare.Namespace, ) ([]libshare.Share, error) { - header, err := h.header.GetByHeight(ctx, height) - if err != nil { - return nil, err - } - - shares, err := h.share.GetSharesByNamespace(ctx, header, namespace) + shares, err := h.share.GetSharesByNamespace(ctx, height, namespace) if err != nil { return nil, err } diff --git a/nodebuilder/node_test.go b/nodebuilder/node_test.go index 8155d7e081..bcf5f01942 100644 --- a/nodebuilder/node_test.go +++ b/nodebuilder/node_test.go @@ -15,9 +15,7 @@ import ( collectormetricpb "go.opentelemetry.io/proto/otlp/collector/metrics/v1" "google.golang.org/protobuf/proto" - "github.com/celestiaorg/celestia-node/header/headertest" "github.com/celestiaorg/celestia-node/nodebuilder/node" - "github.com/celestiaorg/celestia-node/share" ) func TestLifecycle(t *testing.T) { @@ -125,34 +123,3 @@ func StartMockOtelCollectorHTTPServer(t *testing.T) (string, func()) { server.EnableHTTP2 = true return server.URL, server.Close } - -func TestEmptyBlockExists(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - test := []struct { - tp node.Type - }{ - {tp: node.Bridge}, - {tp: node.Full}, - // technically doesn't need to be tested as a SharesAvailable call to - // light node short circuits on an empty EDS - {tp: node.Light}, - } - for i, tt := range test { - t.Run(strconv.Itoa(i), func(t *testing.T) { - node := TestNode(t, tt.tp) - err := node.Start(ctx) - require.NoError(t, err) - - // ensure an empty block exists in store - - eh := headertest.RandExtendedHeaderWithRoot(t, share.EmptyEDSRoots()) - err = node.ShareServ.SharesAvailable(ctx, eh) - require.NoError(t, err) - - err = node.Stop(ctx) - require.NoError(t, err) - }) - } -} diff --git a/nodebuilder/share/cmd/share.go b/nodebuilder/share/cmd/share.go index 41fb9bd8f8..4bb68d497a 100644 --- a/nodebuilder/share/cmd/share.go +++ b/nodebuilder/share/cmd/share.go @@ -1,18 +1,14 @@ package cmd import ( - "context" "encoding/hex" - "fmt" "strconv" "github.com/spf13/cobra" libshare "github.com/celestiaorg/go-square/v2/share" - rpc "github.com/celestiaorg/celestia-node/api/rpc/client" cmdnode "github.com/celestiaorg/celestia-node/cmd" - "github.com/celestiaorg/celestia-node/header" ) func init() { @@ -42,12 +38,12 @@ var sharesAvailableCmd = &cobra.Command{ } defer client.Close() - eh, err := getExtendedHeaderFromCmdArg(cmd.Context(), client, args[0]) + height, err := strconv.ParseUint(args[0], 10, 64) if err != nil { return err } - err = client.Share.SharesAvailable(cmd.Context(), eh) + err = client.Share.SharesAvailable(cmd.Context(), height) formatter := func(data interface{}) interface{} { err, ok := data.(error) available := false @@ -79,7 +75,7 @@ var getSharesByNamespaceCmd = &cobra.Command{ } defer client.Close() - eh, err := getExtendedHeaderFromCmdArg(cmd.Context(), client, args[0]) + height, err := strconv.ParseUint(args[0], 10, 64) if err != nil { return err } @@ -89,7 +85,7 @@ var getSharesByNamespaceCmd = &cobra.Command{ return err } - shares, err := client.Share.GetSharesByNamespace(cmd.Context(), eh, ns) + shares, err := client.Share.GetSharesByNamespace(cmd.Context(), height, ns) return cmdnode.PrintOutput(shares, err, nil) }, } @@ -105,7 +101,7 @@ var getShare = &cobra.Command{ } defer client.Close() - eh, err := getExtendedHeaderFromCmdArg(cmd.Context(), client, args[0]) + height, err := strconv.ParseUint(args[0], 10, 64) if err != nil { return err } @@ -120,7 +116,7 @@ var getShare = &cobra.Command{ return err } - s, err := client.Share.GetShare(cmd.Context(), eh, int(row), int(col)) + s, err := client.Share.GetShare(cmd.Context(), height, int(row), int(col)) formatter := func(data interface{}) interface{} { sh, ok := data.(libshare.Share) @@ -153,26 +149,12 @@ var getEDS = &cobra.Command{ } defer client.Close() - eh, err := getExtendedHeaderFromCmdArg(cmd.Context(), client, args[0]) + height, err := strconv.ParseUint(args[0], 10, 64) if err != nil { return err } - shares, err := client.Share.GetEDS(cmd.Context(), eh) + shares, err := client.Share.GetEDS(cmd.Context(), height) return cmdnode.PrintOutput(shares, err, nil) }, } - -func getExtendedHeaderFromCmdArg(ctx context.Context, client *rpc.Client, arg string) (*header.ExtendedHeader, error) { - height, err := strconv.ParseUint(arg, 10, 64) - if err == nil { - return client.Header.GetByHeight(ctx, height) - } - - hash, err := hex.DecodeString(arg) - if err != nil { - return nil, fmt.Errorf("can't parse the height/hash argument: %w", err) - } - - return client.Header.GetByHash(ctx, hash) -} diff --git a/nodebuilder/share/mocks/api.go b/nodebuilder/share/mocks/api.go index 64a607d495..cccc81a452 100644 --- a/nodebuilder/share/mocks/api.go +++ b/nodebuilder/share/mocks/api.go @@ -8,7 +8,6 @@ import ( context "context" reflect "reflect" - header "github.com/celestiaorg/celestia-node/header" share "github.com/celestiaorg/celestia-node/nodebuilder/share" shwap "github.com/celestiaorg/celestia-node/share/shwap" share0 "github.com/celestiaorg/go-square/v2/share" @@ -40,7 +39,7 @@ func (m *MockModule) EXPECT() *MockModuleMockRecorder { } // GetEDS mocks base method. -func (m *MockModule) GetEDS(arg0 context.Context, arg1 *header.ExtendedHeader) (*rsmt2d.ExtendedDataSquare, error) { +func (m *MockModule) GetEDS(arg0 context.Context, arg1 uint64) (*rsmt2d.ExtendedDataSquare, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetEDS", arg0, arg1) ret0, _ := ret[0].(*rsmt2d.ExtendedDataSquare) @@ -70,7 +69,7 @@ func (mr *MockModuleMockRecorder) GetRange(arg0, arg1, arg2, arg3 interface{}) * } // GetShare mocks base method. -func (m *MockModule) GetShare(arg0 context.Context, arg1 *header.ExtendedHeader, arg2, arg3 int) (share0.Share, error) { +func (m *MockModule) GetShare(arg0 context.Context, arg1 uint64, arg2, arg3 int) (share0.Share, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetShare", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(share0.Share) @@ -85,7 +84,7 @@ func (mr *MockModuleMockRecorder) GetShare(arg0, arg1, arg2, arg3 interface{}) * } // GetSharesByNamespace mocks base method. -func (m *MockModule) GetSharesByNamespace(arg0 context.Context, arg1 *header.ExtendedHeader, arg2 share0.Namespace) (shwap.NamespaceData, error) { +func (m *MockModule) GetSharesByNamespace(arg0 context.Context, arg1 uint64, arg2 share0.Namespace) (shwap.NamespaceData, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetSharesByNamespace", arg0, arg1, arg2) ret0, _ := ret[0].(shwap.NamespaceData) @@ -100,7 +99,7 @@ func (mr *MockModuleMockRecorder) GetSharesByNamespace(arg0, arg1, arg2 interfac } // SharesAvailable mocks base method. -func (m *MockModule) SharesAvailable(arg0 context.Context, arg1 *header.ExtendedHeader) error { +func (m *MockModule) SharesAvailable(arg0 context.Context, arg1 uint64) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "SharesAvailable", arg0, arg1) ret0, _ := ret[0].(error) diff --git a/nodebuilder/share/share.go b/nodebuilder/share/share.go index c2eaf28137..ac348c5373 100644 --- a/nodebuilder/share/share.go +++ b/nodebuilder/share/share.go @@ -8,7 +8,6 @@ import ( libshare "github.com/celestiaorg/go-square/v2/share" "github.com/celestiaorg/rsmt2d" - "github.com/celestiaorg/celestia-node/header" headerServ "github.com/celestiaorg/celestia-node/nodebuilder/header" "github.com/celestiaorg/celestia-node/share" "github.com/celestiaorg/celestia-node/share/eds" @@ -43,15 +42,15 @@ type GetRangeResult struct { type Module interface { // SharesAvailable subjectively validates if Shares committed to the given // ExtendedHeader are available on the Network. - SharesAvailable(context.Context, *header.ExtendedHeader) error + SharesAvailable(ctx context.Context, height uint64) error // GetShare gets a Share by coordinates in EDS. - GetShare(ctx context.Context, header *header.ExtendedHeader, row, col int) (libshare.Share, error) + GetShare(ctx context.Context, height uint64, row, col int) (libshare.Share, error) // GetEDS gets the full EDS identified by the given extended header. - GetEDS(ctx context.Context, header *header.ExtendedHeader) (*rsmt2d.ExtendedDataSquare, error) + GetEDS(ctx context.Context, height uint64) (*rsmt2d.ExtendedDataSquare, error) // GetSharesByNamespace gets all shares from an EDS within the given namespace. // Shares are returned in a row-by-row order if the namespace spans multiple rows. GetSharesByNamespace( - ctx context.Context, header *header.ExtendedHeader, namespace libshare.Namespace, + ctx context.Context, height uint64, namespace libshare.Namespace, ) (shwap.NamespaceData, error) // GetRange gets a list of shares and their corresponding proof. GetRange(ctx context.Context, height uint64, start, end int) (*GetRangeResult, error) @@ -60,19 +59,19 @@ type Module interface { // API is a wrapper around Module for the RPC. type API struct { Internal struct { - SharesAvailable func(context.Context, *header.ExtendedHeader) error `perm:"read"` + SharesAvailable func(ctx context.Context, height uint64) error `perm:"read"` GetShare func( ctx context.Context, - header *header.ExtendedHeader, + height uint64, row, col int, ) (libshare.Share, error) `perm:"read"` GetEDS func( ctx context.Context, - header *header.ExtendedHeader, + height uint64, ) (*rsmt2d.ExtendedDataSquare, error) `perm:"read"` GetSharesByNamespace func( ctx context.Context, - header *header.ExtendedHeader, + height uint64, namespace libshare.Namespace, ) (shwap.NamespaceData, error) `perm:"read"` GetRange func( @@ -83,16 +82,16 @@ type API struct { } } -func (api *API) SharesAvailable(ctx context.Context, header *header.ExtendedHeader) error { - return api.Internal.SharesAvailable(ctx, header) +func (api *API) SharesAvailable(ctx context.Context, height uint64) error { + return api.Internal.SharesAvailable(ctx, height) } -func (api *API) GetShare(ctx context.Context, header *header.ExtendedHeader, row, col int) (libshare.Share, error) { - return api.Internal.GetShare(ctx, header, row, col) +func (api *API) GetShare(ctx context.Context, height uint64, row, col int) (libshare.Share, error) { + return api.Internal.GetShare(ctx, height, row, col) } -func (api *API) GetEDS(ctx context.Context, header *header.ExtendedHeader) (*rsmt2d.ExtendedDataSquare, error) { - return api.Internal.GetEDS(ctx, header) +func (api *API) GetEDS(ctx context.Context, height uint64) (*rsmt2d.ExtendedDataSquare, error) { + return api.Internal.GetEDS(ctx, height) } func (api *API) GetRange(ctx context.Context, height uint64, start, end int) (*GetRangeResult, error) { @@ -101,28 +100,44 @@ func (api *API) GetRange(ctx context.Context, height uint64, start, end int) (*G func (api *API) GetSharesByNamespace( ctx context.Context, - header *header.ExtendedHeader, + height uint64, namespace libshare.Namespace, ) (shwap.NamespaceData, error) { - return api.Internal.GetSharesByNamespace(ctx, header, namespace) + return api.Internal.GetSharesByNamespace(ctx, height, namespace) } type module struct { - shwap.Getter - share.Availability - hs headerServ.Module + getter shwap.Getter + avail share.Availability + hs headerServ.Module } -func (m module) SharesAvailable(ctx context.Context, header *header.ExtendedHeader) error { - return m.Availability.SharesAvailable(ctx, header) +func (m module) GetShare(ctx context.Context, height uint64, row, col int) (libshare.Share, error) { + header, err := m.hs.GetByHeight(ctx, height) + if err != nil { + return libshare.Share{}, err + } + return m.getter.GetShare(ctx, header, row, col) } -func (m module) GetRange(ctx context.Context, height uint64, start, end int) (*GetRangeResult, error) { - extendedHeader, err := m.hs.GetByHeight(ctx, height) +func (m module) GetEDS(ctx context.Context, height uint64) (*rsmt2d.ExtendedDataSquare, error) { + header, err := m.hs.GetByHeight(ctx, height) if err != nil { return nil, err } - extendedDataSquare, err := m.GetEDS(ctx, extendedHeader) + return m.getter.GetEDS(ctx, header) +} + +func (m module) SharesAvailable(ctx context.Context, height uint64) error { + header, err := m.hs.GetByHeight(ctx, height) + if err != nil { + return err + } + return m.avail.SharesAvailable(ctx, header) +} + +func (m module) GetRange(ctx context.Context, height uint64, start, end int) (*GetRangeResult, error) { + extendedDataSquare, err := m.GetEDS(ctx, height) if err != nil { return nil, err } @@ -144,8 +159,12 @@ func (m module) GetRange(ctx context.Context, height uint64, start, end int) (*G func (m module) GetSharesByNamespace( ctx context.Context, - header *header.ExtendedHeader, + height uint64, namespace libshare.Namespace, ) (shwap.NamespaceData, error) { - return m.Getter.GetSharesByNamespace(ctx, header, namespace) + header, err := m.hs.GetByHeight(ctx, height) + if err != nil { + return nil, err + } + return m.getter.GetSharesByNamespace(ctx, header, namespace) } diff --git a/nodebuilder/tests/nd_test.go b/nodebuilder/tests/nd_test.go index 07d6fcc496..4cbe0315b7 100644 --- a/nodebuilder/tests/nd_test.go +++ b/nodebuilder/tests/nd_test.go @@ -72,9 +72,10 @@ func TestShrexNDFromLights(t *testing.T) { ns, err := libshare.NewNamespaceFromBytes(namespace) require.NoError(t, err) - expected, err := bridgeClient.Share.GetSharesByNamespace(reqCtx, h, ns) + height := h.Height() + expected, err := bridgeClient.Share.GetSharesByNamespace(reqCtx, height, ns) require.NoError(t, err) - got, err := lightClient.Share.GetSharesByNamespace(reqCtx, h, ns) + got, err := lightClient.Share.GetSharesByNamespace(reqCtx, height, ns) require.NoError(t, err) require.True(t, len(got[0].Shares) > 0) @@ -145,20 +146,21 @@ func TestShrexNDFromLightsWithBadFulls(t *testing.T) { // ensure to fetch random namespace (not the reserved namespace) namespace := h.DAH.RowRoots[1][:libshare.NamespaceSize] + height := h.Height() ns, err := libshare.NewNamespaceFromBytes(namespace) require.NoError(t, err) - expected, err := bridgeClient.Share.GetSharesByNamespace(reqCtx, h, ns) + expected, err := bridgeClient.Share.GetSharesByNamespace(reqCtx, height, ns) require.NoError(t, err) require.True(t, len(expected[0].Shares) > 0) // choose a random full to test fN := fulls[len(fulls)/2] fnClient := getAdminClient(ctx, fN, t) - gotFull, err := fnClient.Share.GetSharesByNamespace(reqCtx, h, ns) + gotFull, err := fnClient.Share.GetSharesByNamespace(reqCtx, height, ns) require.NoError(t, err) require.True(t, len(gotFull[0].Shares) > 0) - gotLight, err := lightClient.Share.GetSharesByNamespace(reqCtx, h, ns) + gotLight, err := lightClient.Share.GetSharesByNamespace(reqCtx, height, ns) require.NoError(t, err) require.True(t, len(gotLight[0].Shares) > 0) diff --git a/nodebuilder/tests/prune_test.go b/nodebuilder/tests/prune_test.go index 8541649f5a..64f0f0596a 100644 --- a/nodebuilder/tests/prune_test.go +++ b/nodebuilder/tests/prune_test.go @@ -132,15 +132,15 @@ func TestArchivalBlobSync(t *testing.T) { continue } - shr, err := archivalFN.ShareServ.GetShare(ctx, eh, 2, 2) + shr, err := archivalFN.ShareServ.GetShare(ctx, eh.Height(), 2, 2) require.NoError(t, err) - blobs, err := archivalFN.BlobServ.GetAll(ctx, uint64(i), []libshare.Namespace{shr.Namespace()}) + blobs, err := archivalFN.BlobServ.GetAll(ctx, eh.Height(), []libshare.Namespace{shr.Namespace()}) require.NoError(t, err) archivalBlobs = append(archivalBlobs, &archivalBlob{ blob: blobs[0], - height: uint64(i), + height: eh.Height(), root: eh.DAH.Hash(), }) diff --git a/nodebuilder/tests/reconstruct_test.go b/nodebuilder/tests/reconstruct_test.go index 70bcca05be..36cdf3a6da 100644 --- a/nodebuilder/tests/reconstruct_test.go +++ b/nodebuilder/tests/reconstruct_test.go @@ -75,7 +75,7 @@ func TestFullReconstructFromBridge(t *testing.T) { return err } - return fullClient.Share.SharesAvailable(bctx, h) + return fullClient.Share.SharesAvailable(bctx, h.Height()) }) } require.NoError(t, <-fillDn) @@ -214,10 +214,10 @@ func TestFullReconstructFromFulls(t *testing.T) { ctxErr, cancelErr := context.WithTimeout(ctx, time.Second*30) errg, errCtx = errgroup.WithContext(ctxErr) errg.Go(func() error { - return fullClient1.Share.SharesAvailable(errCtx, h) + return fullClient1.Share.SharesAvailable(errCtx, h.Height()) }) errg.Go(func() error { - return fullClient1.Share.SharesAvailable(errCtx, h) + return fullClient1.Share.SharesAvailable(errCtx, h.Height()) }) require.Error(t, errg.Wait()) cancelErr() @@ -230,10 +230,10 @@ func TestFullReconstructFromFulls(t *testing.T) { h, err := fullClient1.Header.WaitForHeight(bctx, uint64(i)) require.NoError(t, err) errg.Go(func() error { - return fullClient1.Share.SharesAvailable(bctx, h) + return fullClient1.Share.SharesAvailable(bctx, h.Height()) }) errg.Go(func() error { - return fullClient2.Share.SharesAvailable(bctx, h) + return fullClient2.Share.SharesAvailable(bctx, h.Height()) }) } @@ -352,7 +352,7 @@ func TestFullReconstructFromLights(t *testing.T) { return err } - return fullClient.Share.SharesAvailable(bctx, h) + return fullClient.Share.SharesAvailable(bctx, h.Height()) }) } require.NoError(t, <-fillDn) diff --git a/nodebuilder/tests/sync_test.go b/nodebuilder/tests/sync_test.go index 90baabd218..1a63975574 100644 --- a/nodebuilder/tests/sync_test.go +++ b/nodebuilder/tests/sync_test.go @@ -77,7 +77,7 @@ func TestSyncAgainstBridge_NonEmptyChain(t *testing.T) { assert.EqualValues(t, h.Commit.BlockID.Hash, sw.GetCoreBlockHashByHeight(ctx, numBlocks)) // check that the light node has also sampled over the block at height 20 - err = lightClient.Share.SharesAvailable(ctx, h) + err = lightClient.Share.SharesAvailable(ctx, h.Height()) assert.NoError(t, err) // wait until the entire chain (up to network head) has been sampled @@ -97,7 +97,7 @@ func TestSyncAgainstBridge_NonEmptyChain(t *testing.T) { assert.EqualValues(t, h.Commit.BlockID.Hash, sw.GetCoreBlockHashByHeight(ctx, numBlocks)) // check to ensure the full node can sync the 20th block's data - err = fullClient.Share.SharesAvailable(ctx, h) + err = fullClient.Share.SharesAvailable(ctx, h.Height()) assert.NoError(t, err) // wait for full node to sync up the blocks from genesis -> network head. @@ -167,7 +167,7 @@ func TestSyncAgainstBridge_EmptyChain(t *testing.T) { assert.EqualValues(t, h.Commit.BlockID.Hash, sw.GetCoreBlockHashByHeight(ctx, numBlocks)) // check that the light node has also sampled over the block at height 20 - err = lightClient.Share.SharesAvailable(ctx, h) + err = lightClient.Share.SharesAvailable(ctx, h.Height()) assert.NoError(t, err) // wait until the entire chain (up to network head) has been sampled @@ -187,7 +187,7 @@ func TestSyncAgainstBridge_EmptyChain(t *testing.T) { assert.EqualValues(t, h.Commit.BlockID.Hash, sw.GetCoreBlockHashByHeight(ctx, numBlocks)) // check to ensure the full node can sync the 20th block's data - err = fullClient.Share.SharesAvailable(ctx, h) + err = fullClient.Share.SharesAvailable(ctx, h.Height()) assert.NoError(t, err) // wait for full node to sync up the blocks from genesis -> network head.