Skip to content

Make SingleLookupRequestState fields private#5563

Merged
mergify[bot] merged 2 commits intosigp:unstablefrom
dapplion:single-lookup-state
Apr 15, 2024
Merged

Make SingleLookupRequestState fields private#5563
mergify[bot] merged 2 commits intosigp:unstablefrom
dapplion:single-lookup-state

Conversation

@dapplion
Copy link
Collaborator

@dapplion dapplion commented Apr 11, 2024 •
edited
Loading

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:

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

@dapplion dapplion mentioned this pull request Apr 11, 2024
Closed
@dapplion dapplion requested a review from realbigsean April 11, 2024 06:23
@dapplion dapplion force-pushed the single-lookup-state branch from fa6c8f1 to 22dba0c Compare April 13, 2024 05:27
@dapplion dapplion mentioned this pull request Apr 13, 2024
Merged
@realbigsean
Copy link
Member

realbigsean commented Apr 15, 2024

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
Copy link
Member

realbigsean commented Apr 15, 2024

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

@realbigsean realbigsean added the ready-for-merge This PR is ready to merge. label Apr 15, 2024
@realbigsean
Copy link
Member

realbigsean commented Apr 15, 2024

@Mergifyio queue

@mergify
Copy link

mergify bot commented Apr 15, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at fee2ee9

@mergify mergify bot merged commit fee2ee9 into sigp:unstable Apr 15, 2024
@dapplion
Copy link
Collaborator Author

dapplion commented Apr 15, 2024 •
edited
Loading

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

@dapplion dapplion deleted the single-lookup-state branch April 16, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@realbigsean realbigsean realbigsean approved these changes

Assignees

No one assigned

Labels

ready-for-merge This PR is ready to merge.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dapplion @realbigsean