celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

core/fetcher: Change `GetBlockInfo` implementation to take an int64 instead of *int64 to restrict potential `nil` case

Open renaynay opened this issue 3 years ago • 1 comments

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

renaynay avatar Jan 10 '22 11:01 renaynay

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.

HoytRen avatar Oct 12 '22 09:10 HoytRen

@vgonkivs, can you help to figure out this issue?

Wondertan avatar Dec 07 '22 16:12 Wondertan

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 avatar Jan 12 '24 10:01 vgonkivs

@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.

HoytRen avatar Jan 12 '24 14:01 HoytRen