celestia-node
celestia-node copied to clipboard
core/fetcher: Change `GetBlockInfo` implementation to take an int64 instead of *int64 to restrict potential `nil` case
Just noting that we always call this method with a concrete &block.Height
taken from a block that was received via a new block event. So this will never be called with latest/nil
as far as I understand but always with a concrete height here. If that is actually the expectation of how GetBlockInfo is to be used, it would be even better by making this explicit and to error on nil
instead.
Originally posted by @liamsi in https://github.com/celestiaorg/celestia-node/pull/327#discussion_r781086374
It seems not right, because in header/core/exchange.go Line 85: return ce.getExtendedHeaderByHeight(ctx, nil)
and in the definition of getExtendedHeaderByHeight, it call GetBlockInfo without check height.
and in the definition of GetBlockInfo it calls: commit, err := f.Commit(ctx, height)
and in the definition of Commit, it calls: res, err := f.client.Commit(ctx, &height)
the implementation of client is provided by tendermint and it requires *int64, then it's the responsibility of tendermint to handle the nil.
or we should not allow call of getExtendedHeaderByHeight with nil height.
@vgonkivs, can you help to figure out this issue?
We can change the argument of getExtendedHeaderByHeight to accept the int64 instead of the pointer, but in case of requesting head, we should agree on some specific value -> 0 or -1.
@vgonkivs I guess here is misunderstand. What I mean is, all these functions never touched the parameter but simply pass it to the SDK, and the function 'Commit', which is come from the SDK, accept a pointer as input but it doesn't handle the nil pointer, then this is a bug of the 'Commit' function. Yes, we could handle the problem on client side. but it's better push the patch to uperstream since the SDK is the source of the problem.