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

Better proposal RPC #11721

Merged
merged 8 commits into from
Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions beacon-chain/execution/testing/mock_engine_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/prysmaticlabs/prysm/v3/consensus-types/blocks"
"github.com/prysmaticlabs/prysm/v3/consensus-types/interfaces"
payloadattribute "github.com/prysmaticlabs/prysm/v3/consensus-types/payload-attribute"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
pb "github.com/prysmaticlabs/prysm/v3/proto/engine/v1"
)
Expand Down Expand Up @@ -53,8 +54,12 @@ func (e *EngineClient) ForkchoiceUpdated(
}

// GetPayload --
func (e *EngineClient) GetPayload(_ context.Context, _ [8]byte) (*pb.ExecutionPayload, error) {
return e.ExecutionPayload, e.ErrGetPayload
func (e *EngineClient) GetPayload(_ context.Context, _ [8]byte, slot types.Slot) (interfaces.ExecutionData, error) {
p, err := blocks.WrappedExecutionPayload(e.ExecutionPayload)
if err != nil {
return nil, err
}
return p, e.ErrGetPayload
}

func (e *EngineClient) GetPayloadV2(_ context.Context, _ [8]byte) (*pb.ExecutionPayloadCapella, error) {
Expand Down
11 changes: 2 additions & 9 deletions beacon-chain/rpc/eth/validator/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -488,16 +488,9 @@ func (vs *Server) ProduceBlindedBlock(ctx context.Context, req *ethpbv1.ProduceB
if optimistic {
return nil, status.Errorf(codes.Unavailable, "The node is currently optimistic and cannot serve validators")
}
altairBlk, err := vs.V1Alpha1Server.BuildAltairBeaconBlock(ctx, v1alpha1req)
b, err := vs.V1Alpha1Server.GetBeaconBlock(ctx, v1alpha1req)
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not prepare beacon block: %v", err)
}
ok, b, err := vs.V1Alpha1Server.GetAndBuildBlindBlock(ctx, altairBlk)
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not prepare blind beacon block: %v", err)
}
if !ok {
return nil, status.Error(codes.Unavailable, "Builder is not available due to miss-config or circuit breaker")
return nil, err
}
blk, err := migration.V1Alpha1BeaconBlockBlindedBellatrixToV2Blinded(b.GetBlindedBellatrix())
if err != nil {
Expand Down
1 change: 0 additions & 1 deletion beacon-chain/rpc/prysm/v1alpha1/validator/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ go_library(
"proposer_deposits.go",
"proposer_eth1data.go",
"proposer_execution_payload.go",
"proposer_phase0.go",
"proposer_sync_aggregate.go",
"server.go",
"status.go",
Expand Down
215 changes: 204 additions & 11 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ import (
emptypb "github.com/golang/protobuf/ptypes/empty"
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/builder"
blocks2 "github.com/prysmaticlabs/prysm/v3/beacon-chain/core/blocks"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/feed"
blockfeed "github.com/prysmaticlabs/prysm/v3/beacon-chain/core/feed/block"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/transition"
v "github.com/prysmaticlabs/prysm/v3/beacon-chain/core/validators"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/db/kv"
"github.com/prysmaticlabs/prysm/v3/config/params"
"github.com/prysmaticlabs/prysm/v3/consensus-types/blocks"
Expand All @@ -41,25 +44,215 @@ func (vs *Server) GetBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (
ctx, span := trace.StartSpan(ctx, "ProposerServer.GetBeaconBlock")
defer span.End()
span.AddAttributes(trace.Int64Attribute("slot", int64(req.Slot)))
if slots.ToEpoch(req.Slot) < params.BeaconConfig().AltairForkEpoch {
blk, err := vs.getPhase0BeaconBlock(ctx, req)

// A syncing validator should not produce a block.
if vs.SyncChecker.Syncing() {
return nil, fmt.Errorf("syncing to latest head, not ready to respond")
}

// An optimistic validator MUST NOT produce a block (i.e., sign across the DOMAIN_BEACON_PROPOSER domain).
if err := vs.optimisticStatus(ctx); err != nil {
return nil, err
}

var blk interfaces.BeaconBlock
var sBlk interfaces.SignedBeaconBlock
var err error
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be its own helper func with tests? Should take in a slot and return the block interface. Maybe call it PrepareBeaconBlockToSign to make it clear it is a signed data structure but does not contain a sig

case slots.ToEpoch(req.Slot) < params.BeaconConfig().AltairForkEpoch:
blk, err = blocks.NewBeaconBlock(&ethpb.BeaconBlock{Body: &ethpb.BeaconBlockBody{}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
sBlk, err = blocks.NewSignedBeaconBlock(&ethpb.SignedBeaconBlock{Block: &ethpb.BeaconBlock{Body: &ethpb.BeaconBlockBody{}}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not fetch phase0 beacon block: %v", err)
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Phase0{Phase0: blk}}, nil
} else if slots.ToEpoch(req.Slot) < params.BeaconConfig().BellatrixForkEpoch {
blk, err := vs.getAltairBeaconBlock(ctx, req)
case slots.ToEpoch(req.Slot) < params.BeaconConfig().BellatrixForkEpoch:
blk, err = blocks.NewBeaconBlock(&ethpb.BeaconBlockAltair{Body: &ethpb.BeaconBlockBodyAltair{}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not fetch Altair beacon block: %v", err)
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
sBlk, err = blocks.NewSignedBeaconBlock(&ethpb.SignedBeaconBlockAltair{Block: &ethpb.BeaconBlockAltair{Body: &ethpb.BeaconBlockBodyAltair{}}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
case slots.ToEpoch(req.Slot) < params.BeaconConfig().CapellaForkEpoch:
blk, err = blocks.NewBeaconBlock(&ethpb.BeaconBlockBellatrix{Body: &ethpb.BeaconBlockBodyBellatrix{}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
sBlk, err = blocks.NewSignedBeaconBlock(&ethpb.SignedBeaconBlockBellatrix{Block: &ethpb.BeaconBlockBellatrix{Body: &ethpb.BeaconBlockBodyBellatrix{}}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
default:
blk, err = blocks.NewBeaconBlock(&ethpb.BeaconBlockCapella{Body: &ethpb.BeaconBlockBodyCapella{}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
sBlk, err = blocks.NewSignedBeaconBlock(&ethpb.SignedBeaconBlockCapella{Block: &ethpb.BeaconBlockCapella{Body: &ethpb.BeaconBlockBodyCapella{}}})
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not initialize block for proposal: %v", err)
}
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Altair{Altair: blk}}, nil
}

// An optimistic validator MUST NOT produce a block (i.e., sign across the DOMAIN_BEACON_PROPOSER domain).
if err := vs.optimisticStatus(ctx); err != nil {
parentRoot, err := vs.HeadFetcher.HeadRoot(ctx)
if err != nil {
return nil, fmt.Errorf("could not retrieve head root: %v", err)
}
head, err := vs.HeadFetcher.HeadState(ctx)
if err != nil {
return nil, fmt.Errorf("could not get head state %v", err)
}
head, err = transition.ProcessSlotsUsingNextSlotCache(ctx, head, parentRoot, req.Slot)
if err != nil {
return nil, fmt.Errorf("could not advance slots to calculate proposer index: %v", err)
}

// Set slot, graffiti, randao reveal, and parent root.
blk.SetSlot(req.Slot)
blk.Body().SetGraffiti(req.Graffiti)
blk.Body().SetRandaoReveal(req.RandaoReveal)
blk.SetParentRoot(parentRoot)
// Set eth1 data.
eth1Data, err := vs.eth1DataMajorityVote(ctx, head)
if err != nil {
return nil, fmt.Errorf("could not get ETH1 data: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's continue here

}
blk.Body().SetEth1Data(eth1Data)

// Set deposit and attestation.
deposits, atts, err := vs.packDepositsAndAttestations(ctx, head, eth1Data)
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.

Let's continue here, that is, let's not pack deposits and attestations if we fail above.

}
blk.Body().SetDeposits(deposits)
blk.Body().SetAttestations(atts)

// Set proposer index
idx, err := helpers.BeaconProposerIndex(ctx, head)
if err != nil {
return nil, fmt.Errorf("could not calculate proposer index %v", err)
}
blk.SetProposerIndex(idx)

// Set slashings
proposerSlashings := vs.SlashingsPool.PendingProposerSlashings(ctx, head, false /*noLimit*/)
validProposerSlashings := make([]*ethpb.ProposerSlashing, 0, len(proposerSlashings))
for _, slashing := range proposerSlashings {
_, err := blocks2.ProcessProposerSlashing(ctx, head, slashing, v.SlashValidator)
if err != nil {
log.WithError(err).Warn("Proposer: invalid proposer slashing")
continue
}
validProposerSlashings = append(validProposerSlashings, slashing)
}
attSlashings := vs.SlashingsPool.PendingAttesterSlashings(ctx, head, false /*noLimit*/)
validAttSlashings := make([]*ethpb.AttesterSlashing, 0, len(attSlashings))
for _, slashing := range attSlashings {
_, err := blocks2.ProcessAttesterSlashing(ctx, head, slashing, v.SlashValidator)
if err != nil {
log.WithError(err).Warn("Proposer: invalid attester slashing")
continue
}
validAttSlashings = append(validAttSlashings, slashing)
}
blk.Body().SetProposerSlashings(validProposerSlashings)
blk.Body().SetAttesterSlashings(validAttSlashings)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be its own tested helper


// Set exits
exits := vs.ExitPool.PendingExits(head, req.Slot, false /*noLimit*/)
validExits := make([]*ethpb.SignedVoluntaryExit, 0, len(exits))
for _, exit := range exits {
val, err := head.ValidatorAtIndexReadOnly(exit.Exit.ValidatorIndex)
if err != nil {
log.WithError(err).Warn("Proposer: invalid exit")
continue
}
if err := blocks2.VerifyExitAndSignature(val, head.Slot(), head.Fork(), exit, head.GenesisValidatorsRoot()); err != nil {
log.WithError(err).Warn("Proposer: invalid exit")
continue
}
validExits = append(validExits, exit)
}
blk.Body().SetVoluntaryExits(validExits)

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be its own tested helper

// Set sync aggregate. New in Altair
if slots.ToEpoch(req.Slot) >= params.BeaconConfig().AltairForkEpoch {
syncAggregate, err := vs.getSyncAggregate(ctx, req.Slot-1, bytesutil.ToBytes32(parentRoot))
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do a custom testnet where we start from altair or greater, there is nothing preventing req.Slot from being 0. Would be best to play defensive to not run into crazy issues in e2e later or in devnets

Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

if err != nil {
return nil, errors.Wrap(err, "could not compute the sync aggregate")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should continue here instead of failing

}
if err := blk.Body().SetSyncAggregate(syncAggregate); err != nil {
return nil, errors.Wrap(err, "could not set sync aggregate")
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, we should log an error and have another helper SetEmptySyncAggregate that attempts to set an empty aggregate

}
}

// Set execution data. New in Bellatrix
if slots.ToEpoch(req.Slot) >= params.BeaconConfig().BellatrixForkEpoch {
fallBackToLocal := true
canUseBuilder, err := vs.canUseBuilder(ctx, req.Slot, idx)
if err != nil {
log.WithError(err).Warn("Proposer: failed to check if builder can be used")
}
if canUseBuilder && err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only use the builder if there was an error in the previous call!

Suggested change
}
if canUseBuilder && err != nil {
} else if canUseBuilder {

h, err := vs.getPayloadHeaderFromBuilder(ctx, req.Slot, idx)
if err != nil {
log.WithError(err).Warn("Proposer: failed to get payload header from builder")
} else {
blk.SetBlinded(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set the block to blinded? Don't we always propose full blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

GetBlock is first part of the proposal. We are giving an unsigned block to the validator to sign and that can be full blocks or blind blocks (using mev-boost)

if err := blk.Body().SetExecution(h); err != nil {
log.WithError(err).Warn("Proposer: failed to set execution payload")
} else {
fallBackToLocal = false
}
}
}
if fallBackToLocal {
executionData, err := vs.getExecutionPayload(ctx, req.Slot, idx, bytesutil.ToBytes32(parentRoot), head)
if err != nil {
return nil, errors.Wrap(err, "could not get execution payload")
}
if err := blk.Body().SetExecution(executionData); err != nil {
return nil, errors.Wrap(err, "could not set execution payload")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nesting and complex conditional logic can be reduced by creating a function that returns an execution data. Then, the caller can set the block's execution value to it

}

// Set bls to execution change. New in Capella
if slots.ToEpoch(req.Slot) >= params.BeaconConfig().CapellaForkEpoch {
changes, err := vs.BLSChangesPool.BLSToExecChangesForInclusion(head)
if err != nil {
return nil, errors.Wrap(err, "could not pack BLSToExecutionChanges")
Copy link
Contributor

Choose a reason for hiding this comment

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

We better return the empty slice instead of erroring here.

}
if err := blk.Body().SetBLSToExecutionChanges(changes); err != nil {
return nil, errors.Wrap(err, "could not set BLSToExecutionChanges")
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be its own tested helper


if err := sBlk.SetBlock(blk); err != nil {
return nil, err
}
return vs.getBellatrixBeaconBlock(ctx, req)
sr, err := vs.computeStateRoot(ctx, sBlk)
if err != nil {
return nil, fmt.Errorf("could not compute state root: %v", err)
}
blk.SetStateRoot(sr)

pb, err := blk.Proto()
if err != nil {
return nil, err
}
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to use a switch statement here instead of a sequence of if checks? Also I would start with Capella

if slots.ToEpoch(req.Slot) >= params.BeaconConfig().CapellaForkEpoch {
    return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Capella{Capella: pb.(*ethpb.BeaconBlockCapella)}}, nil
} 
if slots.ToEpoch(req.Slot) >= params.BeaconConfig().BellatrixForkEpoch {    
...

case slots.ToEpoch(req.Slot) < params.BeaconConfig().AltairForkEpoch:
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Phase0{Phase0: pb.(*ethpb.BeaconBlock)}}, nil
case slots.ToEpoch(req.Slot) < params.BeaconConfig().BellatrixForkEpoch:
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Altair{Altair: pb.(*ethpb.BeaconBlockAltair)}}, nil
case slots.ToEpoch(req.Slot) < params.BeaconConfig().CapellaForkEpoch:
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Bellatrix{Bellatrix: pb.(*ethpb.BeaconBlockBellatrix)}}, nil
}
return &ethpb.GenericBeaconBlock{Block: &ethpb.GenericBeaconBlock_Capella{Capella: pb.(*ethpb.BeaconBlockCapella)}}, nil
}

// ProposeBeaconBlock is called by a proposer during its assigned slot to create a block in an attempt
Expand Down
59 changes: 0 additions & 59 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_altair.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@ package validator

import (
"context"
"fmt"

"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/transition/interop"
"github.com/prysmaticlabs/prysm/v3/config/params"
"github.com/prysmaticlabs/prysm/v3/consensus-types/blocks"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/crypto/bls"
"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
Expand All @@ -15,62 +12,6 @@ import (
"go.opencensus.io/trace"
)

func buildAltairBeaconBlockFromBlockData(blkData *blockData) *ethpb.BeaconBlockAltair {
// Use zero hash as stub for state root to compute later.
stateRoot := params.BeaconConfig().ZeroHash[:]

return &ethpb.BeaconBlockAltair{
Slot: blkData.Slot,
ParentRoot: blkData.ParentRoot,
StateRoot: stateRoot,
ProposerIndex: blkData.ProposerIdx,
Body: &ethpb.BeaconBlockBodyAltair{
Eth1Data: blkData.Eth1Data,
Deposits: blkData.Deposits,
Attestations: blkData.Attestations,
RandaoReveal: blkData.RandaoReveal,
ProposerSlashings: blkData.ProposerSlashings,
AttesterSlashings: blkData.AttesterSlashings,
VoluntaryExits: blkData.VoluntaryExits,
Graffiti: blkData.Graffiti[:],
SyncAggregate: blkData.SyncAggregate,
},
}
}

func (vs *Server) BuildAltairBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (*ethpb.BeaconBlockAltair, error) {
ctx, span := trace.StartSpan(ctx, "ProposerServer.BuildAltairBeaconBlock")
defer span.End()
blkData, err := vs.buildPhase0BlockData(ctx, req)
if err != nil {
return nil, fmt.Errorf("could not build block data: %v", err)
}
return buildAltairBeaconBlockFromBlockData(blkData), nil
}

func (vs *Server) getAltairBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (*ethpb.BeaconBlockAltair, error) {
ctx, span := trace.StartSpan(ctx, "ProposerServer.getAltairBeaconBlock")
defer span.End()
blk, err := vs.BuildAltairBeaconBlock(ctx, req)
if err != nil {
return nil, fmt.Errorf("could not build block data: %v", err)
}
// Compute state root with the newly constructed block.
wsb, err := blocks.NewSignedBeaconBlock(
&ethpb.SignedBeaconBlockAltair{Block: blk, Signature: make([]byte, 96)},
)
if err != nil {
return nil, err
}
stateRoot, err := vs.computeStateRoot(ctx, wsb)
if err != nil {
interop.WriteBlockToDisk(wsb, true /*failed*/)
return nil, fmt.Errorf("could not compute state root: %v", err)
}
blk.StateRoot = stateRoot
return blk, nil
}

// getSyncAggregate retrieves the sync contributions from the pool to construct the sync aggregate object.
// The contributions are filtered based on matching of the input root and slot then profitability.
func (vs *Server) getSyncAggregate(ctx context.Context, slot types.Slot, root [32]byte) (*ethpb.SyncAggregate, error) {
Expand Down
Loading