Better proposal RPC #11721

terencechain · 2022-12-04T16:42:13Z

This PR refactors GetBeaconBlock down for readability and clarity. As GetBeaconBlock is currently constructed, there are convoluted functions and redundant calls. It hurts readability, and it's no longer maintainable for future hard forks and block additions. This PR refactors GetBeaconBlock down to a single function. Please take a look at the attached mermaid diagram, which illustrates the intent of the code

graph TD

GetBeaconBlock --> C1{Returne err if syncing}
C1 --> C2{Returne err if optimsitic}
C2 --> A{Initialize empty `block` and `signed_block` based on request slot}
A -->B{Process head state to requested slot}
B -->C{Set `slot`, `graffiti`, `randao_reveal` and `parent_root` to block}
C -->D{Get `eth1_data` and set to block}
D -->E{Get `deposit` and `attestation` and set to block}
E -->F{Get `proposer_index` and set to block}
F -->G{Get `slashings` and set to block}
G -->H{Get `exit` and set to block}
H -->|If Altair| I{Get `sync_aggregate` and set to block}
I -->|If Bellatrix| J1{If validator registered to use mev-boost and no error}
J1 -->J2{If circuit breaker is not active and no error}
J2 -->J3{Get `execution_data` from builder} 
J1 -->|error|J{Get `execution_data` from local EE and set to block}
J1 -->|not_registered|J
J2 -->|error|J
J2 -->|active|J
J3 -->|error|J
J3 -->|If Capella| K{Get `bls_to_exec` and set to block}
J -->|If Capella|K
K -->L{Compute state root and set state root}
L -->M{Return}

rauljordan · 2022-12-06T16:59:51Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	var blk interfaces.BeaconBlock
+	var sBlk interfaces.SignedBeaconBlock
+	var err error
+	switch {


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

rauljordan · 2022-12-06T17:01:48Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+
+	// 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))


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

nice catch!

rauljordan · 2022-12-06T17:02:35Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+		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 {
+			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)
+				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")
+			}
+		}


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

potuz · 2022-12-07T11:37:35Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	// Set eth1 data.
+	eth1Data, err := vs.eth1DataMajorityVote(ctx, head)
+	if err != nil {
+		return nil, fmt.Errorf("could not get ETH1 data: %v", err)


Let's continue here

potuz · 2022-12-07T11:37:49Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	// Set deposit and attestation.
+	deposits, atts, err := vs.packDepositsAndAttestations(ctx, head, eth1Data)
+	if err != nil {
+		return nil, err


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

potuz · 2022-12-07T11:47:15Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	// 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)


This should be its own tested helper

potuz · 2022-12-07T11:48:33Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+
+	// 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))


nice catch!

potuz · 2022-12-07T11:49:14Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	// 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)
+


This should be its own tested helper

potuz · 2022-12-07T11:55:07Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+		}
+		if canUseBuilder && err != nil {


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

Suggested change

}

if canUseBuilder && err != nil {

} else if canUseBuilder {

potuz · 2022-12-07T11:57:49Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	// 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")
+		}
+		if err := blk.Body().SetBLSToExecutionChanges(changes); err != nil {
+			return nil, errors.Wrap(err, "could not set BLSToExecutionChanges")
+		}
+	}


This should be its own tested helper

potuz · 2022-12-07T11:58:26Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	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")


We better return the empty slice instead of erroring here.

potuz · 2022-12-07T11:58:37Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

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


potuz · 2022-12-07T12:01:49Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+	if err != nil {
+		return nil, err
+	}
+	switch {


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 { ...

…validator-rpcs

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

rkapka · 2022-12-07T16:24:31Z

consensus-types/interfaces/beacon_block.go

@@ -51,25 +56,37 @@ type BeaconBlock interface {
 	ssz.HashRoot
 	Version() int
 	AsSignRequestObject() (validatorpb.SignRequestObject, error)
+	SetBlinded(bool)


Move next to IsBlinded() bool

rkapka · 2022-12-07T16:27:10Z

consensus-types/blocks/getters.go

+		return ErrNotSupported("Execution", b.version)
+	}
+	if b.isBlinded {
+		b.executionPayloadHeader = e // TODO: Copy?


You copy everything else, so I would say yes.

rkapka · 2022-12-07T16:33:40Z

consensus-types/blocks/getters.go

 // Block returns the underlying beacon block object.
 func (b *SignedBeaconBlock) Block() interfaces.BeaconBlock {
 	return b.block
 }

+// SetBlock sets the underlying beacon block object.
+func (b *SignedBeaconBlock) SetBlock(blk interfaces.BeaconBlock) error {


We want a copy here, right? You can create a Copy() method on type BeaconBlock interface and call it at the beginning here.

rkapka · 2022-12-07T16:38:30Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+			if err != nil {
+				log.WithError(err).Warn("Proposer: failed to get payload header from builder")
+			} else {
+				blk.SetBlinded(true)


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

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)

Co-authored-by: Radosław Kapka <[email protected]>

terencechain · 2022-12-08T15:13:04Z

Merging this in the next hour. Thanks for the review everyone

terencechain added 3 commits December 2, 2022 16:13

Refactor block proposal path

279cee4

Add builder paths

d9646a9

Fix interop

468cc23

terencechain requested a review from a team as a code owner December 4, 2022 16:42

terencechain requested review from saolyn, james-prysm and symbolpunk and removed request for a team December 4, 2022 16:42

rauljordan reviewed Dec 6, 2022

View reviewed changes

potuz reviewed Dec 7, 2022

View reviewed changes

terencechain added 2 commits December 7, 2022 07:04

Merge branch 'capella' of github.com:prysmaticlabs/prysm into better-…

3f068c4

…validator-rpcs

Feedback

a0c9d4b

rkapka reviewed Dec 7, 2022

View reviewed changes

terencechain and others added 2 commits December 7, 2022 13:05

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

321014b

Co-authored-by: Radosław Kapka <[email protected]>

Radek's feedback

4514c3a

Fix lint

99abb10

terencechain merged commit f397cba into capella Dec 8, 2022

delete-merged-branch bot deleted the better-validator-rpcs branch December 8, 2022 15:40

terencechain mentioned this pull request Dec 30, 2022

New prysm get block RPC #11834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better proposal RPC #11721

Better proposal RPC #11721

terencechain commented Dec 4, 2022 •

edited

Loading

rauljordan Dec 6, 2022

rauljordan Dec 6, 2022

potuz Dec 7, 2022

rauljordan Dec 6, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

potuz Dec 7, 2022

rkapka Dec 7, 2022

rkapka Dec 7, 2022

rkapka Dec 7, 2022

rkapka Dec 7, 2022

terencechain Dec 7, 2022

terencechain commented Dec 8, 2022

Better proposal RPC #11721

Better proposal RPC #11721

Conversation

terencechain commented Dec 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Dec 8, 2022

terencechain commented Dec 4, 2022 •

edited

Loading