Load block roots from fork choice where possible when serving BlocksByRange requests

[jimmygchen]

Issue Addressed

Partially #7053.

Proposed Changes

Load block roots from fork choice where possible to avoid loading state from disk when serving block by range requests.

[michaelsproul]

Rather than attempting to access fork choice and seeing if it works, I think we should check if the start slot is newer than finalization (start_slot >= finalized_slot), and use fork choice in that case. Fork choice has to have every chain newer than finalization.

This will be more efficient, as it fixes the issue with skipped slots. If a peer requests a range of unfinalized slots that is empty, we can just return nothing. Currently, we will try fork choice, get None, and then try get_block_roots_from_store and get the block roots from the skipped slots. This is not actually helpful, as later in the function we'll drop the blocks that are out of range anyway:

lighthouse/beacon_node/network/src/network_beacon_processor/rpc_methods.rs

Lines 746 to 758 in 2cb71e2

	// Due to skip slots, blocks could be out of the range, we ensure they
	// are in the range before sending
	if block.slot() >= *req.start_slot()
	&& block.slot() < req.start_slot() + req.count()
	{
	blocks_sent += 1;
	self.send_network_message(NetworkMessage::SendResponse {
	peer_id,
	request_id,
	response: Response::BlocksByRange(Some(block.clone())),
	id: (connection_id, substream_id),
	});
	}

In other words, the new fork choice impl allows us to avoid looking up blocks from skipped slots and then discarding them.

Something that this code change is not currently handling is requests that span across the finalized slot. If start_slot < finalized_slot and end_slot > finalized_slot, then the current impl will return partial data (all the blocks newer than finalization). Instead of this, short-term I think it would be better to fallback on the old disk-based iterator, which is accomplished by the same code change I suggested above: only use fork choice if the start slot is newer than finalization.

Longer term, we should use some kind of hybrid iterator so we can easily span the finalized slot without any mucking around. For now I think we don't need to worry about this though, because these requests should be infrequent.

[jimmygchen]

Thanks for the explanation. I've applied the change to check for finalization in bd093d9

[michaelsproul]

Technically we should probably avoid locking fork choice from an async task, but atm I think we have bigger fish to fry. It's nowhere near as bad as doing 10s of state loads 😱