search

filecoin-project / lotus

Reference implementation of the Filecoin protocol, written in Go
https://lotus.filecoin.io/
Other
2.84k stars 1.26k forks source link

eth_getBlockByNumber should return error for null round #12633

Open rvagg opened 3 days ago

rvagg commented 3 days ago

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: https://github.com/filecoin-project/lotus/blob/02a8b972db82eee0ff109366f57122caf7790128/node/impl/full/eth.go#L346-L354

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

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 commented 3 days ago

Thanks to @eshon to alerting me to https://github.com/graphprotocol/graph-node/issues/4729#issuecomment-1650257347

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 commented 1 day ago

I'm only realising that this comes from the fix in https://github.com/filecoin-project/lotus/issues/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 @ https://github.com/filecoin-project/lotus/pull/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 commented 1 day ago

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.