Proposer use highest value payload vs header #11967

terencechain · 2023-02-03T00:01:45Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

With engine_getpayloadv2, blockValue is returned, and a rational validator can compare the local block and builder block (when choosing to use mev-boost) and use the higher value block. This ensures builder can't screw over the validator by constantly bidding over the second-highest bid whilst local mempool has higher value transactions are getting censored

To achieve this, we add Value() (*big.Int, error) to the execution interface and as we construct the interface during block proposal, the value from engine/builder-api gets passed in subsequently
Note: in any other instances, the value is just big.Int(0) which is fair IMO given this is only used during the proposal. The alternate method is to pass values as arguments through the proposal and that's messier

Which issues(s) does this PR fix?

Fixes #

Other notes for review

prestonvanloon · 2023-02-07T19:47:07Z

Build fails

…r-compare-values

prestonvanloon · 2023-02-08T18:20:38Z

conflicts, build fails

nisdas · 2023-02-09T12:05:16Z

beacon-chain/execution/engine_client.go

@@ -211,12 +211,13 @@ func (s *Service) GetPayload(ctx context.Context, payloadId [8]byte, slot primit
 	defer cancel()

 	if slots.ToEpoch(slot) >= params.BeaconConfig().CapellaForkEpoch {
-		result := &pb.ExecutionPayloadCapella{}
+		result := &pb.ExecutionPayloadCapellaWithValue{}


if all capella payloads are now going to have block value in them, why do we need to define a separate type ? Cant we reuse the same capella type here

Capella payload doesn't have block value in them. It's the GetPayloadV2 response that has a block value in them. Hence we are creating a different type that encompasses both the payload itself and the block value

nisdas · 2023-02-09T13:03:30Z

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

-				log.WithError(err).Warn("Proposer: failed to set execution payload")
-			} else {
-				return nil
+			localPayload, err := vs.getExecutionPayload(ctx, slot, idx, blk.ParentRoot(), headState)


what happens if this is still bellatrix. There is no block value, then this call will fail.

yeah, I still need to add coverage here. Thanks for pointing this out!

…r-compare-values

rauljordan · 2023-02-10T03:37:49Z

api/client/builder/bid.go

@@ -156,7 +159,7 @@ func WrappedBuilderBidCapella(p *ethpb.BuilderBidCapella) (Bid, error) {

 // Header --
 func (b builderBidCapella) Header() (interfaces.ExecutionData, error) {
-	return blocks.WrappedExecutionPayloadHeaderCapella(b.p.Header)
+	return blocks.WrappedExecutionPayloadHeaderCapella(b.p.Header, big.NewInt(0).SetBytes(bytesutil.ReverseByteOrder(b.p.Value)))


When I look at this line in a vacuum, there exists a hidden coupling because you're tied to expecting a big endian ordering from b.p.Value. What if this changes in the future? then you'll be reversing into an ordering you do not want. Instead, I would be explicit here about the byte ordering you want

Adding a comment for clarity. If this change in the future then it'll be a dramatic hard fork change then we'll have to sweep the entire client codebase for all the instances. Things will fail abruptly and I argue that's ok. This is the biggest technique ethereum debt where we chose little endian for consensus layer

rauljordan · 2023-02-10T03:38:44Z

beacon-chain/builder/testing/mock.go

 	if err != nil {
 		return nil, errors.Wrap(err, "could not wrap capella payload")
 	}
 	return w, s.ErrSubmitBlindedBlock
 }

 // GetHeader for mocking.
-func (s *MockBuilderService) GetHeader(context.Context, primitives.Slot, [32]byte, [48]byte) (builder.SignedBid, error) {
+func (s *MockBuilderService) GetHeader(ctx context.Context, slot primitives.Slot, hr [32]byte, pb [48]byte) (builder.SignedBid, error) {
+	if slots.ToEpoch(slot) >= params.BeaconConfig().CapellaForkEpoch {


Don't we have helpers for this stuff instead of manual conditionals?

We do not. We use these manual conditionals across the codebase. We certain can clean up but I prefer to do this in another PR

rauljordan · 2023-02-10T03:39:42Z

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

-			blk.SetBlinded(true)
-			if err := blk.SetExecution(h); err != nil {
-				log.WithError(err).Warn("Proposer: failed to set execution payload")
+			// Can only check values if block is Capella or later.


This logic is complex and deeply nested. Would recommend splitting up into a smaller helper and testing it independently

I used a switch case instead of if else. Other than that, it's really hard to split this function up. The concept of it is fairly straightforward. All the local block failures, return. All the remote block failures. cont to use local block

rauljordan · 2023-02-10T03:40:13Z

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

+				}
+				localValue, err := localPayload.Value()
+				if err != nil {
+					return errors.Wrap(err, "failed to get local payload value")


Do we want to error and return in these examples? Why can't we fallback to a value of 0 and continue?

Because this is a local block payload. If it fails then I argue we should abort the ship.
The ones that are ok to log and continue are builder block payload failures

rauljordan · 2023-02-10T03:42:04Z

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

+				builderValue, err := builderPayload.Value()
+				if err != nil {
+					log.WithError(err).Warn("Proposer: failed to get builder payload value") // Default to local if can't get builder value.
+				} else if builderValue.Cmp(localValue) > 0 {


To me, big int comparison methods are always unintuitive and I wish I could remember the < and > conditions. Maybe a comment ? Perhaps we could benefit instead from a Max method that acts on big ints because thats what we want

I'll add a comment

…r-compare-values

Proposer use highest value payload vs header

daf91a1

terencechain requested a review from a team as a code owner February 3, 2023 00:01

terencechain requested review from prestonvanloon, saolyn and james-prysm February 3, 2023 00:01

terencechain added 3 commits February 8, 2023 07:34

Default to interface for changes

4538ba7

Clean up

9d3ea5f

Clean up

d679982

terencechain self-assigned this Feb 8, 2023

terencechain added 2 commits February 8, 2023 07:43

Clean up

896d10d

Merge branch 'develop' of github.com:prysmaticlabs/prysm into propose…

cc79bb1

…r-compare-values

nisdas reviewed Feb 9, 2023

View reviewed changes

terencechain added 6 commits February 9, 2023 07:53

Capella condition

0ed93f3

Merge branch 'develop' of github.com:prysmaticlabs/prysm into propose…

f79186b

…r-compare-values

Tests

461e6f7

Add test for set execution data

a66a195

Gazelle

631292e

Fix panic

637ff08

terencechain added Ready For Review A pull request ready for code review Priority: High High priority item labels Feb 9, 2023

rauljordan reviewed Feb 10, 2023

View reviewed changes

terencechain added 2 commits February 10, 2023 07:13

Merge branch 'develop' of github.com:prysmaticlabs/prysm into propose…

70bdeb9

…r-compare-values

Clean ups, fix tests

dd94110

rauljordan previously approved these changes Feb 10, 2023

View reviewed changes

terencechain added 2 commits February 10, 2023 08:33

Merge branch 'develop' of github.com:prysmaticlabs/prysm into propose…

e364426

…r-compare-values

Fix conflict test

eafefa3

terencechain dismissed rauljordan’s stale review via eafefa3 February 10, 2023 16:44

Fix rest of tests

25d2ac4

rkapka approved these changes Feb 10, 2023

View reviewed changes

rauljordan approved these changes Feb 10, 2023

View reviewed changes

terencechain merged commit e136e10 into develop Feb 10, 2023

delete-merged-branch bot deleted the proposer-compare-values branch February 10, 2023 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposer use highest value payload vs header #11967

Proposer use highest value payload vs header #11967

terencechain commented Feb 3, 2023 •

edited

Loading

prestonvanloon commented Feb 7, 2023

prestonvanloon commented Feb 8, 2023

nisdas Feb 9, 2023

terencechain Feb 9, 2023

nisdas Feb 9, 2023

terencechain Feb 9, 2023

rauljordan Feb 10, 2023

terencechain Feb 10, 2023 •

edited

Loading

rauljordan Feb 10, 2023

terencechain Feb 10, 2023

rauljordan Feb 10, 2023

terencechain Feb 10, 2023

rauljordan Feb 10, 2023

terencechain Feb 10, 2023

rauljordan Feb 10, 2023

terencechain Feb 10, 2023

Proposer use highest value payload vs header #11967

Proposer use highest value payload vs header #11967

Conversation

terencechain commented Feb 3, 2023 • edited Loading

prestonvanloon commented Feb 7, 2023

prestonvanloon commented Feb 8, 2023

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 Feb 10, 2023 • 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

terencechain commented Feb 3, 2023 •

edited

Loading

terencechain Feb 10, 2023 •

edited

Loading