eth_getBlockByNumber should return error for null round

[rvagg]

eth_getBlockByNumber uniquely handles null rounds by returning a nil response (null via RPC), whereas all the other eth_ APIs that take a block number will return an error.

go-ethereum's behaviour seems to be that null indicates that the number isn't available, or at least can't be found in the number<>block mapping, which sounds like it'd be the right thing for us to do for null rounds, but users take this to mean that the block isn't yet available but will become at some point if they keep on polling for it because they just got ahead of the chain.

From @ilyalukyanov:

In EVM null means not found. Given all block numbers are sequential, that specifically means that a block in the future was requested. Therefore:

• If a block isn't found, any block after it would also be not found
• If a block isn't found, but you poll it for long enough, eventually the chain will arrive at it and it will be found.

We have special handling for null rounds in this API that returns nil:

lotus/node/impl/full/eth.go

Lines 346 to 354 in 02a8b97

	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)
	}
	// Create an Ethereum block from the Filecoin tipset

We should return a consistent error for all of these cases, currently we don't. All cases of getTipsetByBlockNumber:

• EthGetBlockByNumber: return nil
• EthFeeHistory: return error: bad block parameter X: requested epoch was a null round
• FilecoinAddressToEthAddress: failed to get tipset for block X: requested epoch was a null round
• EthTraceBlock: failed to get tipset: requested epoch was a null round
• EthTraceReplayBlockTransactions: failed to get tipset: requested epoch was a null round

We should make these consistent.

1. Make ErrNullRound dynamic and let it include the requested block number/string: "block number 0x1234 was a null round"
2. In each of these cases, errors.Is for ErrNullRound and return the err directly (or errors.As and return the instance?)
3. Where it's not a ErrNullRound return a nested error description, but let's make it consistent eh?

[rvagg]

Thanks to @eshon to alerting me to graphprotocol/graph-node#4729 (comment)

It turns out that the ErrNullRound string is a hard-wired expectation in TheGraph: https://github.com/graphprotocol/graph-node/pull/5294/files#diff-b31a71a95f48aa7d0f11b5ea3501befc2560d749d73872e9eebe8003de85d921R1090-R1099

So we just need to make sure we don't mess with that, and it also might be worth putting a note in there for that error // don't change this error string, it's used by downstream tools to detect a null round.

[rvagg]

I'm only realising that this comes from the fix in #10909, and before that we returned an error and I'm now advocating that we return an error. Sorry @virajbhartiya and @aarshkshah1992 I know you've been all over that issue and its fix @ #12529 but I think we need to revisit that because null isn't going to work as per the above reasons. I think we should just error with a very clear reason.

[aarshkshah1992]

I think reverting that change is the right answer here.

@virajbhartiya -> Let's just ensure that we return ErrNullRound like we used to for that API and also make this change in all the ETH RPC APIs listed on this issue.

While it is unfortunate -> looks like this Filecoin specific behaviour wrt null rounds(ETH does not have null rounds) has now been OSSified and ETH tooling has come to depend on it.

[rvagg]

I'm assuming #12641 will make it in, so my updated proposal for this issue is as follows:

1. Turn ErrNullRound into a parameterised error that can take an epoch number, and make the error say: requested epoch was a null round (XXX) where XXX is the number. It's important that we retain the precise text "requested epoch was a null round" in this process because that's being matched against. I think I'd prefer just a plain integer, don't bother returning the blkParam string, return the epoch number it converts to (num in the code at the point where it's generated).
2. For each API that calls in to getTipsetByBlockNumber which may return this error, make sure that the err returned from the call in to getTipsetByBlockNumber is returned as-is. It's already got a limited set of error cases and none of them are currently chained so I don't think we do any service by decorating the errors further. We want the error returned to just be what comes out of there, not some chained nonsense: failed to get tipset for block 0x1234: requested epoch was a null round (4660). We just want requested epoch was a null round (4660).
3. Make this error type propagate through the JSONRPC layer, same as we recently did for ErrExecutionReverted.
   a. Move it in to api/api_errors.go and register it with RPCErrors.
   b. Make a constructor for it that takes an epoch, similar to ErrExecutionReverted, but all it needs to do is build the custom message.
   c. When it's used, it always needs to be a pointer because that's how it gets registered, the constructor can just return that.
   d. Add some tests for it in itests to make sure errors.Is or errors.As gives you what you expect and the message is correct. The existing test in fevm_tests.go named TestEthGetBlockByNumber could be renamed to TestEthNullRoundHandling and just make it test all of the APIs that can return this error to make sure they all do a consistent thing and have the right error type and message.

How does that sound?

[rvagg]

I think also we'll put a note in #10909 after this all done to summarise all of this and that it's a WONTFIX and instead we're returning a consistent error as the best way we can deal with this discrepancy. To date it's proven to be good enough for people to work around and we're just affirming that this is the standard that we're adopting.

[rvagg]

Copying in text I wrote in Slack about the true/false strictness check:

---

I would say this: if the API is specifically about a tipset, then it should be strict; but if the API just happens to need a tipset for some other primary operation then it should be non-strict.

• FilecoinAddressToEthAddress - in this case, we just want the tipset so we know which state tree to use to do the conversion, in most cases the user is just going to want the latest but they may want to be specific. But, the pattern in all the other APIs that do this (a majority of Lotus APIs take a tipset so they can decide which state tree to use), we do near-enough, specifically tipset-before null rounds. So this should be non-strict.
• eth_getBlockByNumber - you care about the block, specifically, so it should be strict. If we don’t find the block you want, near enough isn’t good enough.
• eth_getTransactionByBlockNumberAndIndex - same, you care about the specific block, it should be strict.
• eth_feeHistory - in this case it’s for newestBlock argument for a range of blocks, I would say that for this one that it doesn’t matter so much if that happens to fall on a null round, the one before it is fine, so it should be non-strict and not error on null rounds.
• trace_block - in this case you’re asking for a specific block to trace, so near enough isn’t good enough, it should be strict and error if it’s a null round.
• trace_replayBlockTransactions - same deal, you care about a specific block, it should be strict.

So, for those that aren’t strict, I think you should adjust your tests to show that they don’t error.