Report

Osmosis team: "My understanding of this value is that this implies we should only be fetching blocks 100 blocks in advance from where we currently are. I added prints and saw that even when this value was set as low as 10, we were still fetching blocks 2000+ blocks out while being starved of blocks 2 away from where we currently were

And I might be wrong about this second part, but looking at where this value is used https://github.com/osmosis-labs/cometbft/blob/490dd752eb8a1a934542f70fdc8960594e06973b/blocksync/pool.go#L264-L268, it seems like because we are fetching blocks very far out, we just error and disregard them, so its completely wasted cycles AFAICT."

Preliminary discussion

@jmalicevic: "it seems that maxDiffBetweenCurrentAndReceivedBlockHeight is only checked when we receive blocks, it should probably also be checked when we request blocks. Otherwise it is not the peers fault we get blocks too far ahead. Perhaps avoid creating requesters too far ahead in makeRequestersRoutine() by adapting when then pool.makeNextRequester is called could help."

@cason: "Yes, but only for blocks for which we don't have a tracked request.
It is still to be understood if we don't have a tracked request because it is too "old" (i.e., we already committed this block) or too "new" (i.e., too far in the future). I still can't see how the latter could occur, as: (i) a correct peer only sends us a block upon receiving our request; (ii) we track all requests we send."

@jmalicevic: "So effectively this variable is never checked unless it is an error (we don’t have a requester for it ) or we removed it at some point. But we are never enforcing not requesting blocks too far ahead with this variable it seems"