When fetching non-existing block using eth namespace APIs, it must return null

[aeharvlee]

Proposed changes

eth namespace APIs fetching non-existing block must returns null based on its spec.

As-Is

Is must returns null, but currently it returns error like below.

> eth.getBlockByNumber(50000000)
Error: the block does not exist (block number: 50000000)
    at web3.js:3278:20
    at web3.js:6805:15
    at web3.js:5216:36
    at <anonymous>:1:1

To-be (If this PR is merged.)

Geth

> eth.getBlockByNumber(99999999)
null

Klaytn

• Returns null for non-existing block.

> eth.getBlockByNumber(9999999999)
null

Types of changes

Please put an x in the boxes related to your change.

• Bugfix
• New feature or enhancement
• Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• I have read the CONTRIBUTING GUIDELINES doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes ($ make test)
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in downstream modules

Related issues

• Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

[kjhman21]

@aeharvlee What if other error occurs? There is no way to return err?!

[aeharvlee]

@kjhman21
Currently only one error which is fmt.Errorf("the block does not exist (block number: %d)", blockNr) can be returned from CNAPIBackend.BlockByNumber.

Ethereum treats that case(= The block does not exist) as nil.

By the way, there is the case when error can occur during EthereumAPI.GetBlockByNumber and EthereumAPI.GetBlockByHash.
Both APIs use rpcMarshalBlock to marshal Block data structure to map[string]interface{}.
rpcMarshalBlock calls rpcMarshalHeader internally and rpcMarshalHeader try to fetch block proposer using backend.Author at istanbul/backend/engine.go.
If error occurs during backend.Author, it returns error so EthereumAPI.GetBlockByNumber and EthereumAPI.GetBlockByHash returns error in that case. This is the case when other error can occur during those APIs and this it the different part from Ethereum.

cc. @aidan-kwon

[aidan-kwon]

Either one is okay for now. But, I think error string comparison increases the complexity of these APIs.

[kjhman21]

What about this?

klaytnBlock, err := api.publicBlockChainAPI.b.BlockByNumber(ctx, number)
if err != nil {
  if err == errBlockNotFound {
    return nil, nil
  }
  return nil, err
}
		response, err := api.rpcMarshalBlock(klaytnBlock, true, fullTx)
		if err == nil && number == rpc.PendingBlockNumber {
			// Pending blocks need to nil out a few fields
			for _, field := range []string{"hash", "nonce", "miner"} {
				response[field] = nil
			}
		}
		return response, err

[aeharvlee]

@kjhman21
Looks great to me.
I'll update this PR.

[aeharvlee]

@kjhman21

PTAL this commit.

I considered defining not existing err as separate error variables in cn package and try to pack with that error like below, but there is a import cycle problem between cn package and api package.

// node/cn/api_backend.go
var ErrBlockNotFound = errors.New("the block does not exist")

func (b *CNAPIBackend) BlockByHash(ctx context.Context, hash common.Hash) (*types.Block, error) {
	block := b.cn.blockchain.GetBlockByHash(hash)
	if block == nil {
		return nil, fmt.Errorf("%w (block hash: %s)", ErrBlockNotFound, hash.String())
	}
	return block, nil
}

// api/api_ethereum.go
func (api *EthereumAPI) GetBlockByNumber(ctx context.Context, number rpc.BlockNumber) (map[string]interface{}, error) {
	klaytnHeader, err := api.publicBlockChainAPI.b.BlockByNumber(ctx, number)
	if err != nil {
		if errors.Is(err, cn.ErrBlockNotFound) {
			return nil, nil
		}
		return nil, err
	}
        // ...
}

So I checked the error whether it is containing "does not exist" text or not in its error message.

And I also updated other apis using similar logics too. (e.g. GetHeaderByNumber)

[kjhman21]

For now, it is okay to me, but it would be better to more specify the exact error string in the if statement like Ethereum.