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

Fix Bellatrix Helpers to Work With Both Blinded and Full Blocks #11019

Closed
wants to merge 5 commits into from

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Jul 11, 2022

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

As part of #11010, we noticed there are several helper functions in the codebase and uses of ExecutionPayload that ignore blinded beacon blocks. For example, Optimistic Sync has a critical helper function that checks if a block is execution enabled. This function only works on full, Bellatrix beacon blocks and fails on blinded ones. This PR audits all uses of execution payloads in the codebase and ensures everything is updated to work with blinded beacon blocks as well.

In some previous code review from #11010, it was noted how we should differentiate between blocks that have an execution payload and those that have an execution payload header. For example, let's say we want to write a function that takes in a beacon block and tells us the BlockHash of the execution payload contained within. Here's how we do it today:

func blockHashFromExecutionPayload(blk interfaces.BeaconBlock) ([]byte, error) {
  payload, err := blk.ExecutionPayload()
  if err != nil {
    return nil, err
  }
  return payload.BlockHash
}

This only works if the block has an ExecutionPayload field. However, Bellatrix blinded beacon blocks have ExecutionPayloadHeader which should work just fine. We have a few options available to get both full Bellatrix and blinded Bellatrix blocks to work with this function.

Option A: Duplication

The first option is to duplicate our code like this:

func blockHashFromExecutionPayloadBellatrix(blk interfaces.BeaconBlock) ([]byte, error) {
  payload, err := blk.ExecutionPayload()
  if err != nil {
    return nil, err
  }
  return payload.BlockHash
}

func blockHashFromExecutionPayloadBellatrixBlinded(blk interfaces.BeaconBlock) ([]byte, error) {
  payload, err := blk.ExecutionPayloadHeader()
  if err != nil {
    return nil, err
  }
  return payload.BlockHash
}

Option B: Use block versioning schemes

func blockHashFromExecutionPayloadBellatrix(blk interfaces.BeaconBlock) ([]byte, error) {
  switch blk.Version() {
    case version.Bellatrix:
    payload, err := blk.ExecutionPayload()
    if err != nil {
      return nil, err
    }
    return payload.BlockHash
    case version.BellatrixBlind:
    payload, err := blk.ExecutionPayloadHeader()
    if err != nil {
      return nil, err
    }
    return payload.BlockHash
    default:
      return nil, errors.New("unsupported version")
  }
}

Option C: Use type assertions

func blockHashFromExecutionPayloadBellatrix(blk interfaces.BeaconBlock) ([]byte, error) {
  switch blk.(type) {
    case *ethpb.BeaconBlockBellatrix:
      return blk.ExecutionPayload.BlockHash
    case version.BellatrixBlind:
      return blk.ExecutionPayloadHeader.BlockHash
    default:
      return nil, errors.New("unsupported version")
  }
}

Option D: Turn the execution payload field of a block body into an interface

type ExecutionData interface {
  BlockHash() common.Hash
  Transactions ([]*gethtypes.Transaction, error) // Exists in full payloads, not in header
  TransactionsRoot ([32]byte, error) // Exists in header, not in full payloads
}

Option E: Use the Golang Error Pattern

func blockHashFromExecutionPayload(blk interfaces.BeaconBlock) ([]byte, error) {
  payload, err := blk.ExecutionPayload()
  switch {
  case errors.Is(err, ErrUnsupportedField):
    payloadHeader, err := blk.ExecutionPayloadHeader()
    if err != nil { ... }
    return payloadHeader.BlockHash, nil
  case err != nil:
    return nil, err
  }
  return payload.BlockHash
}

Our goals are:

  1. To make our code future-proof: that is, the code does not need to be aware of beacon block versions and instead leverage interfaces to the best of their ability
  2. To make our code maintainable: we should not need to informally remember to update x.go file after y thing happens. Instead, we should minimize maintainability costs. If a future hard-fork changes some fields in a beacon block, our unit tests should fail and that should be how we figure out that we need to upgrade a certain file. We should not let this happen at runtime or in production

We argue options A, B, C do not meet these goals because they require significant maintenance, awareness of future block versions, and can manifest bugs at runtime rather than in CI / development pipelines.

We suggest Option D because it is

  1. Agnostic to versioning schemes: developers do not need to go back after every new version and add a new line to a switch statement. Instead, if the field is no longer supported in a future fork, tests will fail, and developers will know to update the code. This shifts responsibility to our CI suite rather than remembering things before they break in production
  2. Is more idiomatic: using ErrNotFound or other named, package errors as control variables is a well-known Go language pattern that is used extensively by projects. The database/sql package from the standard library is a good example

We also say that Option E is a solid direction, as it helps keep things clean. However, this introduces a lot more error checks and dealing with interfaces in our consensus-types package.

Which issues(s) does this PR fix?

Part of #10589

@rauljordan rauljordan requested a review from a team as a code owner July 11, 2022 16:22
@rauljordan rauljordan marked this pull request as draft July 11, 2022 16:23
@rauljordan rauljordan marked this pull request as ready for review July 11, 2022 17:58
@rauljordan rauljordan closed this Jul 11, 2022
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.

1 participant