Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Action in single_block_component_processed #5564

Merged

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Apr 11, 2024

Issue Addressed

Part of:

The job of single_block_component_processed is to decide what action to do in response to a block components processing result. The current code is correct, but IMO it is hard to audit to ensure that:

  • The branching logic results in some action
  • The lookup is not dropped un-intentionally

Proposed Changes

This PR changes the current match into returning a single Action value

enum Action {
    Retry,
    ParentUnknown,
    Drop,
}

This style achieves the goals:

  • Ensures branching logic results in exactly one action
  • Ensures the lookup is not dropped un-intentionally
  • Allows to convert lookup to a mutable reference as the match does not call into some self. method (not impl in this PR). We can switch from the current pattern of remove -> insert to retry / noop to drop, to get_mut -> noop to retry / remove to drop.

I'd recommend looking at the final file for a better read
https://github.com/dapplion/lighthouse/blob/540f87325f132b5065379117d92fba1e308c8a02/beacon_node/network/src/sync/block_lookups/mod.rs#L798

Next steps

There are 3 other functions that could adopt this pattern:

  • single_lookup_response
  • parent_block_processed
  • parent_chain_processed

But if this PR is well received I would prefer to tackle each latter and separately

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

@Mergifyio queue

Copy link

mergify bot commented Apr 16, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at e5b8d12

mergify bot added a commit that referenced this pull request Apr 16, 2024
@mergify mergify bot merged commit e5b8d12 into sigp:unstable Apr 16, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants