Skip to content

Refactor getLocalPayloadAndBlobs with New Helper getParentBlockHash#12951

Merged
prylabs-bulldozer[bot] merged 2 commits intodevelopfrom
proposer-get-parent-hash
Sep 27, 2023
Merged

Refactor getLocalPayloadAndBlobs with New Helper getParentBlockHash#12951
prylabs-bulldozer[bot] merged 2 commits intodevelopfrom
proposer-get-parent-hash

Conversation

@terencechain
Copy link
Collaborator

Description

This pull request aims to improve the maintainability and readability of the getLocalPayloadAndBlobs function. The refactor introduces a new helper function, getParentBlockHash, which streamlines the logic for fetching the parent block hash based on various conditions. This change aligns with the consensus spec as outlined in PR #3350.

Key Changes

  • New Helper Function: Adds getParentBlockHash to encapsulate the logic for obtaining the parent block hash.
  • Optimization for Capella or Older: If the beacon state is Capella or older, the function optimizes the IsMergeTransitionComplete check and fetches the parent block hash directly from st.LatestExecutionPayloadHeader().
  • Consistent Checks: Retains the existing checks for activationEpochNotReached and getTerminalBlockHashIfExists when necessary.

func getParentBlockHashPostCapella(st state.BeaconState) ([]byte, error) {
header, err := st.LatestExecutionPayloadHeader()
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only errors that can ever be returned by LatestExecutionPayloadHeader() is either errNotSupported+funcname or ErrNilObjectWrapped as there are two instances of the same call it may be better to add some additional information as to whether the failure is when retrieving the hash post Capella or below post merge.

Copy link
Contributor

@saolyn saolyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
var attr payloadattribute.Attributer
switch st.Version() {
case version.Deneb:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, there don't appear to be any tests covering this case here.

@terencechain terencechain force-pushed the proposer-get-parent-hash branch from 5a25e9f to fbb9680 Compare September 27, 2023 02:50
@prylabs-bulldozer prylabs-bulldozer bot merged commit b0caea3 into develop Sep 27, 2023
@prylabs-bulldozer prylabs-bulldozer bot deleted the proposer-get-parent-hash branch September 27, 2023 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants