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

This reverts commit c674554.
filecoin-project · Oct 28, 2024 · 8feaa02 · 8feaa02
1 parent 29a131c
commit 8feaa02
Show file tree

Hide file tree

Showing 16 changed files with 42 additions and 63 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,7 +3,6 @@
 # UNRELEASED
 
 ## New features
-- 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))
 - `lotus chain head` now supports a `--height` flag to print just the epoch number of the current chain head ([filecoin-project/lotus#12609](https://github.com/filecoin-project/lotus/pull/12609))

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 nil, 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 *new(ethtypes.EthBlock), 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 nil, 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 *new(ethtypes.EthBlock), 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 nil, err
+		return ethtypes.EthBlock{}, err
 	}
 
 	if err := gw.checkBlkParam(ctx, blkNum, 0); err != nil {
-		return nil, err
+		return ethtypes.EthBlock{}, 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,24 +65,18 @@ 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
 		}
-
-		// 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
-		}
+		require.NoError(t, err)
+		require.Equal(t, ts.Height(), abi.ChainEpoch(i), "expected a tipset at epoch %i", i)
 
 		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,9 +454,8 @@ func TestGetBlockByNumber(t *testing.T) {
 	}
 
 	// Fail when trying to fetch a null round.
-	block, err := client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
-	require.NoError(t, err)
-	require.Nil(t, block)
+	_, err = client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
+	require.Error(t, err)
 
 	// 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
@@ -1396,9 +1396,8 @@ func TestEthGetBlockByNumber(t *testing.T) {
 	require.NotNil(t, specificBlock)
 
 	// Test getting a future block (should fail)
-	futureBlock, err := client.EthGetBlockByNumber(ctx, (latest + 10000).Hex(), true)
+	_, err = client.EthGetBlockByNumber(ctx, (latest + 10000).Hex(), true)
 	require.Error(t, err)
-	require.Nil(t, futureBlock)
 
 	// Inject 10 null rounds
 	bms[0].InjectNulls(10)
@@ -1427,9 +1426,8 @@ func TestEthGetBlockByNumber(t *testing.T) {
 	}
 
 	// 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)
+	_, err = client.EthGetBlockByNumber(ctx, (ethtypes.EthUint64(nullHeight)).Hex(), true)
+	require.ErrorContains(t, err, "requested epoch was a null round")
 
 	// Test getting balance on a null round
 	bal, err := client.EthGetBalance(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromNumber(ethtypes.EthUint64(nullHeight)))

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 nil, ErrModuleDisabled
+func (e *EthModuleDummy) EthGetBlockByNumber(ctx context.Context, blkNum string, fullTxInfo bool) (ethtypes.EthBlock, error) {
+	return ethtypes.EthBlock{}, 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,23 +341,12 @@ 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) {
-	// Get the tipset for the specified block parameter
+func (a *EthModule) EthGetBlockByNumber(ctx context.Context, blkParam string, fullTxInfo bool) (ethtypes.EthBlock, error) {
 	ts, err := getTipsetByBlockNumber(ctx, a.Chain, blkParam, true)
 	if err != nil {
-		if err == ErrNullRound {
-			// Return nil for null rounds
-			return nil, nil
-		}
-		return nil, xerrors.Errorf("failed to get tipset: %w", err)
+		return ethtypes.EthBlock{}, err
 	}
-	// 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
+	return newEthBlockFromFilecoinTipSet(ctx, ts, fullTxInfo, a.Chain, a.StateAPI)
 }
 
 func (a *EthModule) EthGetTransactionByHash(ctx context.Context, txHash *ethtypes.EthHash) (*ethtypes.EthTx, error) {