Check da_checker before doing a block lookup request

[dapplion]

Issue Addressed

Testing of v5.2.0 shows many more rpc_block than usual. This is due this sequence of events:

• Block is known to the da_checker but not yet imported into fork-choice
• The event UnknownBlockHashFromAttestation is sent at least once every block (this is likely a bug and should be fixed separately)
• A new lookup is created for the block, downloads block, and sends for processing

In current stable the single lookup checks against the da_checker if the block is known before sending a request, but after

• Merge parent and current sync lookups #5655

this check was dropped by mistake

Proposed Changes

• Add a check in SyncNetworkContext::block_lookup_request and skip the download if the block is already known

However, now we need to handle the case where a lookup goes directly from created -> completed. This takes a bit of refactoring, and to handle this gracefully and safely for all cases I have introduced the LookupResult with a must_use directive.

[realbigsean]

don't we want to use the reqresp_pre_import_cache? Otherwise we'll miss blocks that we're in the process of verifying execution

[realbigsean]

the check in stable is essentially this because data_availability_checker.has_block() used to check the processing cache

[realbigsean]

lion agreed offline, so I've pushed the change here: 5db3e1b

[realbigsean]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 01e4e35

[michaelsproul]

I think this is the cause of the recent de-sync issues.

Consider the following sequence of events:

1. Block arrives on gossip, we add it to the reqresp_pre_import_cache.
2. We send the block's payload to the EL for verification.
3. We receive blobs on gossip and add them to the DA cache.
4. The EL times out. Block processing is aborted as a result.
5. Subsequent attempts to look up the block and re-process it are blocked by the entry in the reqresp_pre_import_cache.
6. We stall and are unable to reprocess the block until range sync and/or a blocks by range request is made in spite of the check above.

I've confirmed that this happened for block 0xfc4a1afe39d84a410c61f213cf15cb405fdd15b96ca54f0c826d4054671b1275 at slot 1574856 on Holesky. We get the block and all the blobs, then the EL times out and everything gets stuck:

May 04 05:31:20.803 DEBG Failed to verify execution payload error: ExecutionPayloadError(RequestFailed(EngineError(Api { error: HttpClient(url: http://127.0.0.1:8551/, kind: timeout, detail: operation timed out) })))

[michaelsproul]

Actually scratch that, @jimmygchen pointed out that we remove from the reqresp_pre_import_cache on failure here:

lighthouse/beacon_node/beacon_chain/src/beacon_chain.rs

Lines 2945 to 2950 in 63fad7e

	let has_missing_components =
	matches!(r, Ok(AvailabilityProcessingStatus::MissingComponents(_, _)));
	if !has_missing_components {
	self.reqresp_pre_import_cache.write().remove(block_root);
	}
	r