Skip to content

Use Action in single_block_component_processed#5564

Merged
mergify[bot] merged 4 commits intosigp:unstablefrom
dapplion:single_block_component_processed-action
Apr 16, 2024
Merged

Use Action in single_block_component_processed#5564
mergify[bot] merged 4 commits intosigp:unstablefrom
dapplion:single_block_component_processed-action

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

@mergify
Copy link

mergify bot commented Apr 16, 2024

queue

✅ The pull request has been merged automatically

Details

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
@dapplion dapplion deleted the single_block_component_processed-action branch January 24, 2025 17:57
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