feat: fix GetBlock for null rounds by returning nil (#12529)

virajbhartiya · aarshkshah1992 · web-flow · commit c67455450419 · 2024-10-03T11:52:16.000Z
* update pb version

* docs: update changelog

* fix: remove redundant code in sectors.go

* feat: Update EthGetBlockByNumber to return pointer to EthBlock

* fix TestGetBlockByNumber

* fix: Remove unnecessary assertions tests

* Update eth_api_test.go

* feat: Refactor EthGetBlockByNumber test in fevm_test.go

---------

Co-authored-by: Aarsh Shah &lt;aarshkshah1992@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,7 +12,8 @@
 
 ## Improvements
 
-- Update `TestEthGetTransactionCount` to test for `earliest`, `pending` and `finalized`.  ([filecoin-project/lotus#12448](https://github.com/filecoin-project/lotus/pull/12448))
+
+- Update `EthGetBlockByNumber` to return a pointer to ethtypes.EthBlock or nil for null rounds. ([filecoin-project/lotus#12529](https://github.com/filecoin-project/lotus/pull/12529))
 - Reduce size of embedded genesis CAR files by removing WASM actor blocks and compressing with zstd. This reduces the `lotus` binary size by approximately 10 MiB. ([filecoin-project/lotus#12439](https://github.com/filecoin-project/lotus/pull/12439))
 - Add ChainSafe operated Calibration archival node to the bootstrap list ([filecoin-project/lotus#12517](https://github.com/filecoin-project/lotus/pull/12517))
 
diff --git a/api/api_full.go b/api/api_full.go
@@ -775,7 +775,7 @@ type FullNode interface {
 	EthGetBlockTransactionCountByHash(ctx context.Context, blkHash ethtypes.EthHash) (ethtypes.EthUint64, error) //perm:read
 
 	EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash, fullTxInfo bool) (ethtypes.EthBlock, error)                                //perm:read
-	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error)                                         //perm:read
+	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error)                                        //perm:read
 	EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error)                                             //perm:read
 	EthGetTransactionByHashLimited(ctx context.Context, txHash *ethtypes.EthHash, limit abi.ChainEpoch) (*ethtypes.EthTx, error)                //perm:read
 	EthGetTransactionHashByCid(ctx context.Context, cid cid.Cid) (*ethtypes.EthHash, error)                                                     //perm:read
diff --git a/api/api_gateway.go b/api/api_gateway.go
@@ -98,7 +98,7 @@ type Gateway interface {
 	EthGetBlockTransactionCountByNumber(ctx context.Context, blkNum ethtypes.EthUint64) (ethtypes.EthUint64, error)
 	EthGetBlockTransactionCountByHash(ctx context.Context, blkHash ethtypes.EthHash) (ethtypes.EthUint64, error)
 	EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash, fullTxInfo bool) (ethtypes.EthBlock, error)
-	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error)
+	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error)
 	EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error)
 	EthGetTransactionByHashLimited(ctx context.Context, txHash *ethtypes.EthHash, limit abi.ChainEpoch) (*ethtypes.EthTx, error)
 	EthGetTransactionHashByCid(ctx context.Context, cid cid.Cid) (*ethtypes.EthHash, error)
diff --git a/api/mocks/mock_full.go b/api/mocks/mock_full.go
diff --git a/api/proxy_gen.go b/api/proxy_gen.go
diff --git a/build/openrpc/full.json b/build/openrpc/full.json
@@ -2866,7 +2866,7 @@
         },
         {
             "name": "Filecoin.EthGetBlockByNumber",
-            "description": "```go\nfunc (s *FullNodeStruct) EthGetBlockByNumber(p0 context.Context, p1 string, p2 bool) (ethtypes.EthBlock, error) {\n\tif s.Internal.EthGetBlockByNumber == nil {\n\t\treturn *new(ethtypes.EthBlock), ErrNotSupported\n\t}\n\treturn s.Internal.EthGetBlockByNumber(p0, p1, p2)\n}\n```",
+            "description": "```go\nfunc (s *FullNodeStruct) EthGetBlockByNumber(p0 context.Context, p1 string, p2 bool) (*ethtypes.EthBlock, error) {\n\tif s.Internal.EthGetBlockByNumber == nil {\n\t\treturn nil, ErrNotSupported\n\t}\n\treturn s.Internal.EthGetBlockByNumber(p0, p1, p2)\n}\n```",
             "summary": "",
             "paramStructure": "by-position",
             "params": [
@@ -2902,8 +2902,8 @@
                 }
             ],
             "result": {
-                "name": "ethtypes.EthBlock",
-                "description": "ethtypes.EthBlock",
+                "name": "*ethtypes.EthBlock",
+                "description": "*ethtypes.EthBlock",
                 "summary": "",
                 "schema": {
                     "examples": [
diff --git a/build/openrpc/gateway.json b/build/openrpc/gateway.json
@@ -2213,7 +2213,7 @@
         },
         {
             "name": "Filecoin.EthGetBlockByNumber",
-            "description": "```go\nfunc (s *GatewayStruct) EthGetBlockByNumber(p0 context.Context, p1 string, p2 bool) (ethtypes.EthBlock, error) {\n\tif s.Internal.EthGetBlockByNumber == nil {\n\t\treturn *new(ethtypes.EthBlock), ErrNotSupported\n\t}\n\treturn s.Internal.EthGetBlockByNumber(p0, p1, p2)\n}\n```",
+            "description": "```go\nfunc (s *GatewayStruct) EthGetBlockByNumber(p0 context.Context, p1 string, p2 bool) (*ethtypes.EthBlock, error) {\n\tif s.Internal.EthGetBlockByNumber == nil {\n\t\treturn nil, ErrNotSupported\n\t}\n\treturn s.Internal.EthGetBlockByNumber(p0, p1, p2)\n}\n```",
             "summary": "There are not yet any comments for this method.",
             "paramStructure": "by-position",
             "params": [
@@ -2249,8 +2249,8 @@
                 }
             ],
             "result": {
-                "name": "ethtypes.EthBlock",
-                "description": "ethtypes.EthBlock",
+                "name": "*ethtypes.EthBlock",
+                "description": "*ethtypes.EthBlock",
                 "summary": "",
                 "schema": {
                     "examples": [
diff --git a/gateway/node.go b/gateway/node.go
@@ -119,7 +119,7 @@ type TargetAPI interface {
 	EthGetBlockTransactionCountByNumber(ctx context.Context, blkNum ethtypes.EthUint64) (ethtypes.EthUint64, error)
 	EthGetBlockTransactionCountByHash(ctx context.Context, blkHash ethtypes.EthHash) (ethtypes.EthUint64, error)
 	EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash, fullTxInfo bool) (ethtypes.EthBlock, error)
-	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error)
+	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error)
 	EthGetTransactionByHashLimited(ctx context.Context, txHash *ethtypes.EthHash, limit abi.ChainEpoch) (*ethtypes.EthTx, error)
 	EthGetTransactionHashByCid(ctx context.Context, cid cid.Cid) (*ethtypes.EthHash, error)
 	EthGetMessageCidByTransactionHash(ctx context.Context, txHash *ethtypes.EthHash) (*cid.Cid, error)
diff --git a/gateway/proxy_eth.go b/gateway/proxy_eth.go
@@ -188,13 +188,13 @@ func (gw *Node) EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash,
 	return gw.target.EthGetBlockByHash(ctx, blkHash, fullTxInfo)
 }
 
-func (gw *Node) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error) {
+func (gw *Node) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error) {
 	if err := gw.limit(ctx, stateRateLimitTokens); err != nil {
-		return ethtypes.EthBlock{}, err
+		return nil, err
 	}
 
 	if err := gw.checkBlkParam(ctx, blkNum, 0); err != nil {
-		return ethtypes.EthBlock{}, err
+		return nil, err
 	}
 
 	return gw.target.EthGetBlockByNumber(ctx, blkNum, fullTxInfo)
diff --git a/itests/eth_api_test.go b/itests/eth_api_test.go
@@ -408,7 +408,7 @@ func TestEthBlockNumberAliases(t *testing.T) {
 
 	head := client.WaitTillChain(ctx, kit.HeightAtLeast(policy.ChainFinality+100))
 	// latest should be head-1 (parents)
-	var latestEthBlk ethtypes.EthBlock
+	var latestEthBlk *ethtypes.EthBlock
 	for {
 		var err error
 		latestEthBlk, err = client.EVM().EthGetBlockByNumber(ctx, "latest", true)
diff --git a/itests/eth_block_hash_test.go b/itests/eth_block_hash_test.go
@@ -65,18 +65,24 @@ func TestEthBlockHashesCorrect_MultiBlockTipset(t *testing.T) {
 		require.NoError(t, err)
 
 		ethBlockA, err := n2.EthGetBlockByNumber(ctx, hex, true)
-		// Cannot use static ErrFullRound error for comparison since it gets reserialized as a JSON RPC error.
 		if err != nil && strings.Contains(err.Error(), "null round") {
 			require.Less(t, ts.Height(), abi.ChainEpoch(i), "did not expect a tipset at epoch %d", i)
 			continue
 		}
-		require.NoError(t, err)
-		require.Equal(t, ts.Height(), abi.ChainEpoch(i), "expected a tipset at epoch %i", i)
+
+		// Check if the tipset height is less than or equal to the current epoch
+		require.LessOrEqual(t, ts.Height(), abi.ChainEpoch(i), "tipset height should not exceed the current epoch")
+
+		// Skip Null rounds
+		if ethBlockA == nil {
+			continue
+		}
 
 		ethBlockB, err := n2.EthGetBlockByHash(ctx, ethBlockA.Hash, true)
 		require.NoError(t, err)
+		require.NotNil(t, ethBlockB)
 
-		require.Equal(t, ethBlockA, ethBlockB)
+		require.Equal(t, *ethBlockA, ethBlockB)
 
 		numBlocks := len(ts.Blocks())
 		expGasLimit := ethtypes.EthUint64(int64(numBlocks) * buildconstants.BlockGasLimit)
diff --git a/itests/eth_deploy_test.go b/itests/eth_deploy_test.go
@@ -206,7 +206,7 @@ func TestDeployment(t *testing.T) {
 	blkNum := strconv.FormatInt(int64(*chainTx.BlockNumber), 10)
 	block2, err := client.EthGetBlockByNumber(ctx, blkNum, false)
 	require.Nil(t, err)
-	require.True(t, reflect.DeepEqual(block1, block2))
+	require.True(t, reflect.DeepEqual(block1, *block2))
 
 	// verify that the block contains full tx objects
 	block3, err := client.EthGetBlockByHash(ctx, *chainTx.BlockHash, true)
@@ -235,7 +235,7 @@ func TestDeployment(t *testing.T) {
 	// make sure the _full_ block got from EthGetBlockByNumber is the same
 	block4, err := client.EthGetBlockByNumber(ctx, blkNum, true)
 	require.Nil(t, err)
-	require.True(t, reflect.DeepEqual(block3, block4))
+	require.True(t, reflect.DeepEqual(block3, *block4))
 
 	// Verify that the deployer is now an account.
 	client.AssertActorType(ctx, deployer, manifest.EthAccountKey)
diff --git a/itests/eth_transactions_test.go b/itests/eth_transactions_test.go
@@ -454,8 +454,9 @@ func TestGetBlockByNumber(t *testing.T) {
 	}
 
 	// Fail when trying to fetch a null round.
-	_, err = client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
-	require.Error(t, err)
+	block, err := client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
+	require.NoError(t, err)
+	require.Nil(t, block)
 
 	// Fetch balance on a null round; should not fail and should return previous balance.
 	bal, err := client.EthGetBalance(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromNumber(ethtypes.EthUint64(nullHeight)))
diff --git a/itests/fevm_test.go b/itests/fevm_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/filecoin-project/lotus/api"
 	"github.com/filecoin-project/lotus/build"
 	"github.com/filecoin-project/lotus/build/buildconstants"
+	"github.com/filecoin-project/lotus/chain/store"
 	"github.com/filecoin-project/lotus/chain/types"
 	"github.com/filecoin-project/lotus/chain/types/ethtypes"
 	"github.com/filecoin-project/lotus/itests/kit"
@@ -1308,3 +1309,78 @@ func TestEthGetTransactionCount(t *testing.T) {
 	require.NoError(t, err)
 	require.Zero(t, contractNonceAfterDestroy)
 }
+
+func TestEthGetBlockByNumber(t *testing.T) {
+	blockTime := 100 * time.Millisecond
+	client, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.ThroughRPC())
+
+	bms := ens.InterconnectAll().BeginMining(blockTime)
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+
+	// Create a new Ethereum account
+	_, ethAddr, filAddr := client.EVM().NewAccount()
+	// Send some funds to the f410 address
+	kit.SendFunds(ctx, t, client, filAddr, types.FromFil(10))
+
+	// Test getting the latest block
+	latest, err := client.EthBlockNumber(ctx)
+	require.NoError(t, err)
+	latestBlock, err := client.EthGetBlockByNumber(ctx, "latest", true)
+	require.NoError(t, err)
+	require.NotNil(t, latestBlock)
+
+	// Test getting a specific block by number
+	specificBlock, err := client.EthGetBlockByNumber(ctx, latest.Hex(), true)
+	require.NoError(t, err)
+	require.NotNil(t, specificBlock)
+
+	// Test getting a future block (should fail)
+	futureBlock, err := client.EthGetBlockByNumber(ctx, (latest + 10000).Hex(), true)
+	require.Error(t, err)
+	require.Nil(t, futureBlock)
+
+	// Inject 10 null rounds
+	bms[0].InjectNulls(10)
+
+	// Wait until we produce blocks again
+	tctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+	ch, err := client.ChainNotify(tctx)
+	require.NoError(t, err)
+	<-ch       // current
+	hc := <-ch // wait for next block
+	require.Equal(t, store.HCApply, hc[0].Type)
+
+	afterNullHeight := hc[0].Val.Height()
+
+	// Find the first null round
+	nullHeight := afterNullHeight - 1
+	for nullHeight > 0 {
+		ts, err := client.ChainGetTipSetByHeight(ctx, nullHeight, types.EmptyTSK)
+		require.NoError(t, err)
+		if ts.Height() == nullHeight {
+			nullHeight--
+		} else {
+			break
+		}
+	}
+
+	// Test getting a block for a null round
+	nullRoundBlock, err := client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
+	require.NoError(t, err)
+	require.Nil(t, nullRoundBlock)
+
+	// Test getting balance on a null round
+	bal, err := client.EthGetBalance(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromNumber(ethtypes.EthUint64(nullHeight)))
+	require.NoError(t, err)
+	require.NotEqual(t, big.Zero(), bal)
+	require.Equal(t, types.FromFil(10).Int, bal.Int)
+
+	// Test getting block by pending
+	pendingBlock, err := client.EthGetBlockByNumber(ctx, "pending", true)
+	require.NoError(t, err)
+	require.NotNil(t, pendingBlock)
+	require.True(t, pendingBlock.Number >= latest)
+}
diff --git a/node/impl/full/dummy.go b/node/impl/full/dummy.go
@@ -51,8 +51,8 @@ func (e *EthModuleDummy) EthGetBlockByHash(ctx context.Context, blkHash ethtypes
 	return ethtypes.EthBlock{}, ErrModuleDisabled
 }
 
-func (e *EthModuleDummy) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error) {
-	return ethtypes.EthBlock{}, ErrModuleDisabled
+func (e *EthModuleDummy) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error) {
+	return nil, ErrModuleDisabled
 }
 
 func (e *EthModuleDummy) EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error) {
diff --git a/node/impl/full/eth.go b/node/impl/full/eth.go
@@ -57,7 +57,7 @@ type EthModuleAPI interface {
 	EthGetBlockTransactionCountByNumber(ctx context.Context, blkNum ethtypes.EthUint64) (ethtypes.EthUint64, error)
 	EthGetBlockTransactionCountByHash(ctx context.Context, blkHash ethtypes.EthHash) (ethtypes.EthUint64, error)
 	EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash, fullTxInfo bool) (ethtypes.EthBlock, error)
-	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error)
+	EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (*ethtypes.EthBlock, error)
 	EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error)
 	EthGetTransactionByHashLimited(ctx context.Context, txHash *ethtypes.EthHash, limit abi.ChainEpoch) (*ethtypes.EthTx, error)
 	EthGetMessageCidByTransactionHash(ctx context.Context, txHash *ethtypes.EthHash) (*cid.Cid, error)
@@ -341,12 +341,23 @@ func (a *EthModule) EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthH
 	return blk, nil
 }
 
-func (a *EthModule) EthGetBlockByNumber(ctx context.Context, blkParam string, fullTxInfo bool) (ethtypes.EthBlock, error) {
+func (a *EthModule) EthGetBlockByNumber(ctx context.Context, blkParam string, fullTxInfo bool) (*ethtypes.EthBlock, error) {
+	// Get the tipset for the specified block parameter
 	ts, err := getTipsetByBlockNumber(ctx, a.Chain, blkParam, true)
 	if err != nil {
-		return ethtypes.EthBlock{}, err
+		if err == ErrNullRound {
+			// Return nil for null rounds
+			return nil, nil
+		}
+		return nil, xerrors.Errorf("failed to get tipset: %w", err)
 	}
-	return newEthBlockFromFilecoinTipSet(ctx, ts, fullTxInfo, a.Chain, a.StateAPI)
+	// Create an Ethereum block from the Filecoin tipset
+	block, err := newEthBlockFromFilecoinTipSet(ctx, ts, fullTxInfo, a.Chain, a.StateAPI)
+	if err != nil {
+		return nil, xerrors.Errorf("failed to create Ethereum block: %w", err)
+	}
+
+	return &block, nil
 }
 
 func (a *EthModule) EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error) {

Original file line number	Diff line number	Diff line change
`@@ -188,13 +188,13 @@ func (gw *Node) EthGetBlockByHash(ctx context.Context, blkHash ethtypes.EthHash,`
`188`	`188`	`return gw.target.EthGetBlockByHash(ctx, blkHash, fullTxInfo)`
`189`	`189`	`}`
`190`	`190`
`191`		`-func (gw *Node) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error) {`
	`191`	`+func (gw Node) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error) {`
`192`	`192`	`if err := gw.limit(ctx, stateRateLimitTokens); err != nil {`
`193`		`- return ethtypes.EthBlock{}, err`
	`193`	`+ return nil, err`
`194`	`194`	`}`
`195`	`195`
`196`	`196`	`if err := gw.checkBlkParam(ctx, blkNum, 0); err != nil {`
`197`		`- return ethtypes.EthBlock{}, err`
	`197`	`+ return nil, err`
`198`	`198`	`}`
`199`	`199`
`200`	`200`	`return gw.target.EthGetBlockByNumber(ctx, blkNum, fullTxInfo)`