Make SingleLookupRequestState fields private

[dapplion]

Issue Addressed

SingleLookupRequestState fields are public and mutated all over the place. This makes the code unnecessarily difficult to follow. Instead, its fields should be private and mutated through setters such that looking the the implementation of SingleLookupRequestState is sufficient to view and understand all possible state transitions.

Part of:

• Block lookup sync technical debt #5549

Proposed Changes

• Make SingleLookupRequestState fields private
• Move state transition to download result from verify_response_inner (specific per request) to verify_response (generic for the trait)
• In verify_response use the actual response sending peer instead of the one being tracked in the state. See code comment about potential inconsistencies.

A future PR should add more checks in setters like on_download_success such that a state transition from Processed -> Downloading is not possible

[realbigsean]

definitely support consolidating where these state changes are made. Not tracking this led to one of our recent bugs in duplicate parent lookups

A future PR should add more checks in setters like on_download_success such that a state transition from Processed -> Downloading is not possible

Agreed

[realbigsean]

@dapplion mentioned offline the TODOs here will be fixed in #5583

[realbigsean]

@Mergifyio queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at fee2ee9

[dapplion]

@realbigsean about this todo

lighthouse/beacon_node/network/src/sync/block_lookups/common.rs

Lines 169 to 171 in 6cc98a7

	// TODO: We requested a download from Downloading { peer_id }, but the network
	// injects a response from a different peer_id. What should we do? The peer_id to
	// track for scoring is the one that actually sent the response, not the state's

not sure if this was just overlooked in the original design. The peer returned from the network is decided by internal logic. If there are not bugs in routing the correct request_id the peer_id should always match. Then, should we even track the downloading peer?