Use begin/end block instead of query to fix the height indexing…

… issue with commitments (#838) * adds heights validation when generating data commtiments * better data commitment validation * changes query to begin/end block for data commitment * add docs * fix test * cosmetics * Update light/rpc/client.go * Update rpc/client/http/http.go * Update rpc/client/http/http.go * Update rpc/client/http/http.go * Update rpc/client/http/http.go
celestiaorg · Aug 17, 2022 · 5c84468 · 5c84468
1 parent 100cf0a
commit 5c84468
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 33 deletions.
diff --git a/light/proxy/routes.go b/light/proxy/routes.go
@@ -28,7 +28,7 @@ func RPCRoutes(c *lrpc.Client) map[string]*rpcserver.RPCFunc {
 		"block_by_hash":        rpcserver.NewRPCFunc(makeBlockByHashFunc(c), "hash"),
 		"block_results":        rpcserver.NewRPCFunc(makeBlockResultsFunc(c), "height"),
 		"commit":               rpcserver.NewRPCFunc(makeCommitFunc(c), "height"),
-		"data_commitment":      rpcserver.NewRPCFunc(makeDataCommitmentFunc(c), "query"),
+		"data_commitment":      rpcserver.NewRPCFunc(makeDataCommitmentFunc(c), "beginBlock,endBlock"),
 		"tx":                   rpcserver.NewRPCFunc(makeTxFunc(c), "hash,prove"),
 		"tx_search":            rpcserver.NewRPCFunc(makeTxSearchFunc(c), "query,prove,page,per_page,order_by"),
 		"block_search":         rpcserver.NewRPCFunc(makeBlockSearchFunc(c), "query,page,per_page,order_by"),
@@ -136,15 +136,17 @@ func makeCommitFunc(c *lrpc.Client) rpcCommitFunc {
 
 type rpcDataCommitmentFunc func(
 	ctx *rpctypes.Context,
-	query string,
+	beginBlock uint64,
+	endBlock uint64,
 ) (*ctypes.ResultDataCommitment, error)
 
 func makeDataCommitmentFunc(c *lrpc.Client) rpcDataCommitmentFunc {
 	return func(
 		ctx *rpctypes.Context,
-		query string,
+		beginBlock uint64,
+		endBlock uint64,
 	) (*ctypes.ResultDataCommitment, error) {
-		return c.DataCommitment(ctx.Context(), query)
+		return c.DataCommitment(ctx.Context(), beginBlock, endBlock)
 	}
 }
 

diff --git a/light/rpc/client.go b/light/rpc/client.go
@@ -454,8 +454,12 @@ func (c *Client) Commit(ctx context.Context, height *int64) (*ctypes.ResultCommi
 	}, nil
 }
 
-func (c *Client) DataCommitment(ctx context.Context, query string) (*ctypes.ResultDataCommitment, error) {
-	return c.next.DataCommitment(ctx, query)
+func (c *Client) DataCommitment(
+	ctx context.Context,
+	beginBlock uint64,
+	endBlock uint64,
+) (*ctypes.ResultDataCommitment, error) {
+	return c.next.DataCommitment(ctx, beginBlock, endBlock)
 }
 
 // Tx calls rpcclient#Tx method and then verifies the proof if such was

diff --git a/rpc/client/http/http.go b/rpc/client/http/http.go
@@ -457,10 +457,15 @@ func (c *baseRPCClient) Commit(ctx context.Context, height *int64) (*ctypes.Resu
 	return result, nil
 }
 
-func (c *baseRPCClient) DataCommitment(ctx context.Context, query string) (*ctypes.ResultDataCommitment, error) {
+func (c *baseRPCClient) DataCommitment(
+	ctx context.Context,
+	beginBlock uint64,
+	endBlock uint64,
+) (*ctypes.ResultDataCommitment, error) {
 	result := new(ctypes.ResultDataCommitment)
 	params := map[string]interface{}{
-		"query": query,
+		"beginBlock": beginBlock,
+		"endBlock":   endBlock,
 	}
 
 	_, err := c.caller.Call(ctx, "data_commitment", params, result)

diff --git a/rpc/client/interface.go b/rpc/client/interface.go
@@ -69,7 +69,7 @@ type SignClient interface {
 	BlockResults(ctx context.Context, height *int64) (*ctypes.ResultBlockResults, error)
 	Commit(ctx context.Context, height *int64) (*ctypes.ResultCommit, error)
 
-	DataCommitment(ctx context.Context, query string) (*ctypes.ResultDataCommitment, error)
+	DataCommitment(ctx context.Context, beginBlock uint64, endBlock uint64) (*ctypes.ResultDataCommitment, error)
 
 	Validators(ctx context.Context, height *int64, page, perPage *int) (*ctypes.ResultValidators, error)
 	Tx(ctx context.Context, hash []byte, prove bool) (*ctypes.ResultTx, error)

diff --git a/rpc/client/local/local.go b/rpc/client/local/local.go
@@ -173,8 +173,12 @@ func (c *Local) Commit(ctx context.Context, height *int64) (*ctypes.ResultCommit
 	return core.Commit(c.ctx, height)
 }
 
-func (c *Local) DataCommitment(_ context.Context, query string) (*ctypes.ResultDataCommitment, error) {
-	return core.DataCommitment(c.ctx, query)
+func (c *Local) DataCommitment(
+	_ context.Context,
+	beginBlock uint64,
+	endBlock uint64,
+) (*ctypes.ResultDataCommitment, error) {
+	return core.DataCommitment(c.ctx, beginBlock, endBlock)
 }
 
 func (c *Local) Validators(ctx context.Context, height *int64, page, perPage *int) (*ctypes.ResultValidators, error) {

diff --git a/rpc/client/mock/client.go b/rpc/client/mock/client.go
@@ -47,7 +47,6 @@ var _ client.Client = Client{}
 
 // Call is used by recorders to save a call and response.
 // It can also be used to configure mock responses.
-//
 type Call struct {
 	Name     string
 	Args     interface{}
@@ -170,8 +169,12 @@ func (c Client) Commit(ctx context.Context, height *int64) (*ctypes.ResultCommit
 	return core.Commit(&rpctypes.Context{}, height)
 }
 
-func (c Client) DataCommitment(ctx context.Context, query string) (*ctypes.ResultDataCommitment, error) {
-	return core.DataCommitment(&rpctypes.Context{}, query)
+func (c Client) DataCommitment(
+	ctx context.Context,
+	beginBlock uint64,
+	endBlock uint64,
+) (*ctypes.ResultDataCommitment, error) {
+	return core.DataCommitment(&rpctypes.Context{}, beginBlock, endBlock)
 }
 
 func (c Client) Validators(ctx context.Context, height *int64, page, perPage *int) (*ctypes.ResultValidators, error) {

diff --git a/rpc/client/rpc_test.go b/rpc/client/rpc_test.go
@@ -648,7 +648,7 @@ func TestDataCommitment(t *testing.T) {
 
 	// check if data commitment is not nil.
 	// Checking if the commitment is correct is done in `core/blocks_test.go`.
-	dataCommitment, err := c.DataCommitment(ctx, fmt.Sprintf("block.height <= %d", expectedHeight))
+	dataCommitment, err := c.DataCommitment(ctx, 0, uint64(expectedHeight))
 	require.NotNil(t, dataCommitment, "data commitment shouldn't be nul.")
 	require.Nil(t, err, "%+v when creating data commitment.", err)
 }

diff --git a/rpc/core/blocks.go b/rpc/core/blocks.go
@@ -136,28 +136,59 @@ func Commit(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultCommit, erro
 
 // DataCommitment collects the data roots over a provided ordered range of blocks,
 // and then creates a new Merkle root of those data roots.
-func DataCommitment(ctx *rpctypes.Context, query string) (*ctypes.ResultDataCommitment, error) {
-	heights, err := heightsByQuery(ctx, query)
+func DataCommitment(ctx *rpctypes.Context, beginBlock uint64, endBlock uint64) (*ctypes.ResultDataCommitment, error) {
+	err := validateDataCommitmentRange(beginBlock, endBlock)
 	if err != nil {
 		return nil, err
 	}
+	heights := generateHeightsList(beginBlock, endBlock)
+	blockResults := fetchBlocks(heights, len(heights), 0)
+	root := hashDataRoots(blockResults)
+	// Create data commitment
+	return &ctypes.ResultDataCommitment{DataCommitment: root}, nil
+}
 
-	if len(heights) > consts.DataCommitmentBlocksLimit {
-		return nil, fmt.Errorf("the query exceeds the limit of allowed blocks %d", consts.DataCommitmentBlocksLimit)
-	} else if len(heights) == 0 {
-		return nil, fmt.Errorf("cannot create the data commitments for an empty set of blocks")
+// generateHeightsList takes a begin and end block, then generates a list of heights
+// containing the elements of the range [beginBlock, endBlock].
+func generateHeightsList(beginBlock uint64, endBlock uint64) []int64 {
+	heights := make([]int64, endBlock-beginBlock+1)
+	for i := beginBlock; i <= endBlock; i++ {
+		heights[i-beginBlock] = int64(i)
 	}
+	return heights
+}
 
-	err = sortBlocks(heights, "asc")
+// validateDataCommitmentRange runs basic checks on the asc sorted list of heights
+// that will be used subsequently in generating data commitments over the defined set of heights.
+func validateDataCommitmentRange(beginBlock uint64, endBlock uint64) error {
+	heightsRange := endBlock - beginBlock + 1
+	if heightsRange > uint64(consts.DataCommitmentBlocksLimit) {
+		return fmt.Errorf("the query exceeds the limit of allowed blocks %d", consts.DataCommitmentBlocksLimit)
+	}
+	if heightsRange == 0 {
+		return fmt.Errorf("cannot create the data commitments for an empty set of blocks")
+	}
+	if beginBlock > endBlock {
+		return fmt.Errorf("end block is smaller than begin block")
+	}
+	if endBlock > uint64(env.BlockStore.Height()) {
+		return fmt.Errorf(
+			"end block %d is higher than current chain height %d",
+			endBlock,
+			env.BlockStore.Height(),
+		)
+	}
+	has, err := env.BlockIndexer.Has(int64(endBlock))
 	if err != nil {
-		return nil, err
+		return err
 	}
-
-	blockResults := fetchBlocks(heights, len(heights), 0)
-	root := hashDataRoots(blockResults)
-
-	// Create data commitment
-	return &ctypes.ResultDataCommitment{DataCommitment: root}, nil
+	if !has {
+		return fmt.Errorf(
+			"end block %d is still not indexed",
+			endBlock,
+		)
+	}
+	return nil
 }
 
 // hashDataRoots hashes a list of blocks data hashes and returns their merkle root.

diff --git a/rpc/core/blocks_test.go b/rpc/core/blocks_test.go
@@ -122,7 +122,7 @@ func TestBlockResults(t *testing.T) {
 
 func TestDataCommitmentResults(t *testing.T) {
 	env = &Environment{}
-	height := int64(100)
+	height := int64(2826)
 
 	blocks := randomBlocks(height)
 	blockStore := mockBlockStore{
@@ -137,19 +137,20 @@ func TestDataCommitmentResults(t *testing.T) {
 		expectPass bool
 	}{
 		{10, 15, true},
+		{2727, 2828, false},
 		{10, 9, false},
 		{0, 1000, false},
+		{10, 8, false},
 	}
 
 	for _, tc := range testCases {
-		mockedQuery := fmt.Sprintf("block.height >= %d AND block.height <= %d", tc.beginQuery, tc.endQuery)
 		env.BlockIndexer = mockBlockIndexer{
 			height:          height,
 			beginQueryBlock: tc.beginQuery,
 			endQueryBlock:   tc.endQuery,
 		}
 
-		actualCommitment, err := DataCommitment(&rpctypes.Context{}, mockedQuery)
+		actualCommitment, err := DataCommitment(&rpctypes.Context{}, uint64(tc.beginQuery), uint64(tc.endQuery))
 		if tc.expectPass {
 			require.Nil(t, err, "should generate the needed data commitment.")
 

diff --git a/rpc/core/routes.go b/rpc/core/routes.go
@@ -24,7 +24,7 @@ var Routes = map[string]*rpc.RPCFunc{
 	"block_by_hash":        rpc.NewRPCFunc(BlockByHash, "hash"),
 	"block_results":        rpc.NewRPCFunc(BlockResults, "height"),
 	"commit":               rpc.NewRPCFunc(Commit, "height"),
-	"data_commitment":      rpc.NewRPCFunc(DataCommitment, "query"),
+	"data_commitment":      rpc.NewRPCFunc(DataCommitment, "beginBlock,endBlock"),
 	"check_tx":             rpc.NewRPCFunc(CheckTx, "tx"),
 	"tx":                   rpc.NewRPCFunc(Tx, "hash,prove"),
 	"tx_search":            rpc.NewRPCFunc(TxSearch, "query,prove,page,per_page,order_by"),