Unwrap payload bid #14035

kasey · 2024-05-22T03:02:16Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR removes the need for types that satisfy ExecutionData to internally carry bid values in wei/gwei. The payload bid is a separate piece of data from the payload and coupling them together creates complexity outside of the small proposal code path where it applies.

Which issues(s) does this PR fix?

This PR is setting the stage for further simplification of the consesus-types package. By bringing ExecutionData closer to the spec types, we can begin to reduce the number of different representations for the type.

This is a big set of changes but I built it up in layers, so if you look at the individual commits that should make the review more managable. Feedback to break this up to a smaller commit is welcome - I can strip off the newest commits that remove helper methods from consensus-types without breaking the earlier changes in proposal or engine code.

terencechain · 2024-05-22T03:09:05Z

beacon-chain/execution/engine_client.go

+	//d := time.Now().Add(defaultEngineTimeout)
+	//ctx, cancel := context.WithDeadline(ctx, d)
+	//defer cancel()


What should we do about these commented code?

kasey · 2024-05-22T03:27:35Z

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

@@ -154,7 +155,7 @@ func TestConstructGenericBeaconBlock(t *testing.T) {
 	t.Run("phase0 block", func(t *testing.T) {
 		b, err := blocks.NewSignedBeaconBlock(util.NewBeaconBlock())
 		require.NoError(t, err)
-		result, err := vs.constructGenericBeaconBlock(b, nil)
+		result, err := vs.constructGenericBeaconBlock(b, nil, primitives.ZeroWei)


note to self: primitives.ZeroWei should be a func that returns a new value each call. Returning a pointer type risks some code mutating it accidentally.

I'm going to fix this in a follow-up PR.

kasey · 2024-05-22T03:32:42Z

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

+				log.WithError(err).Error("Could not get builder payload")
+			} else if builderBid == nil {
+				builderGetPayloadMissCount.Inc()
+				log.WithError(err).Error("Could not get builder payload")


This is wonky, I actually don't think we need to do anything if err == nil && builderBid == nil - this means getBuilderPayloadAndBlobs decided we didn't need to call the builder, because it's disabled or pre-bellatrix.

kasey · 2024-05-22T03:35:50Z

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

+		res, err := vs.ExecutionEngineCaller.GetPayload(ctx, pid, slot)
+		if err == nil {
+			//payload, bundle, overrideBuilder := res.ExecutionData, res.BlobsBundle, res.OverrideBuilder
+			//bundleCache.add(slot, bundle)


kasey · 2024-05-22T03:36:10Z

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

-	return payload, overrideBuilder, nil
+
+	// TODO: remove bundleCache code
+	//bundleCache.add(slot, res.BlobsBundle)


james-prysm · 2024-05-22T20:46:09Z

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

-	}
+	winningBid := primitives.ZeroWei
+	var bundle *enginev1.BlobsBundle
+	if slots.ToEpoch(sBlk.Block().Slot()) >= params.BeaconConfig().BellatrixForkEpoch {


dumb question maybe, do we have to check in this way to account for forks? instead of checking the block/state version?

As to why I did it this way, I think I was copying existing code in setExecutionData (which this method calls). I don't mind changing it though.

james-prysm · 2024-05-22T20:54:53Z

beacon-chain/builder/testing/mock.go

 		if err != nil {
 			return nil, nil, errors.Wrap(err, "could not wrap capella payload")
 		}
 		return w, nil, s.ErrSubmitBlindedBlock
 	case version.Deneb:
-		w, err := blocks.WrappedExecutionPayloadDeneb(s.PayloadDeneb, big.NewInt(0))
+		w, err := blocks.WrappedExecutionPayloadDeneb(s.PayloadDeneb)


should we add electra stuff in this PR? ( I saw some electra stuff but also a lot of other places not accounted for)

I'd rather not, I'm only touching electra stuff that needs to be changed in reaction to removing bids from the payload interface.

james-prysm · 2024-05-22T21:03:59Z

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

 		}

+		// Compare payload values between local and builder. Default to the local value if it is higher.
+		localValueGwei := primitives.WeiToGwei(local.Bid)
+		builderValueGwei := primitives.WeiToGwei(bid.WeiValue())


should we add a function on bid to return GweiValue instead? not sure which is more readable

I only added the WeiValue method because it encapsulates the tricky reversing logic for accessing a value that is directly stored by the type. IMO the gwei conversion is pretty trivial and doesn't warrant another method.

Also thinking about modifying the Value method to just be the WeiValue method since I don't think we want to expose the little endian version, but I'll need to double check usage.

james-prysm · 2024-05-22T21:05:23Z

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

 	}
-	return setExecution(blk, execution, false, kzgCommitments)
+	// TODO: plumb blobs bundle through here.


is this still a TODO?

nope, will delete

james-prysm · 2024-05-22T21:08:35Z

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

 }

 // warnIfFeeRecipientDiffers logs a warning if the fee recipient in the included payload does not
 // match the requested one.
-func warnIfFeeRecipientDiffers(payload interfaces.ExecutionData, feeRecipient primitives.ExecutionAddress) {
+func warnIfFeeRecipientDiffers(payload, val []byte) {


these parameters feel harder to read, is there a better name for this? ( not a big deal , just a comment)

I thought this was more clear since both parameters are fee recipients and now have the same type (previously one was called feeRecipient, which is also in the fuction name, and yeah they're both fee recipients). The function name tells you that you are comparing fee recipients, I figured it would be clear that one is the fee recipient from the payload, and the other is the one from the validator registration. Maybe the val name is too ambiguous and something like this would be more clear?
func warnIfFeeRecipientDiffers(payload, registration []byte) {

func warnIfFeeRecipientDiffers(payload, registration []byte) {

yeah i think that may be better 👍

Even more simple: func warnIfFeeRecipientDiffers(want, got []byte) {

kasey requested review from prestonvanloon, terencechain, rkapka and james-prysm May 22, 2024 03:02

kasey requested a review from a team as a code owner May 22, 2024 03:02

kasey force-pushed the unwrap-payload-bid branch from 3c78b20 to e8b14ca Compare May 22, 2024 03:02

terencechain reviewed May 22, 2024

View reviewed changes

kasey force-pushed the unwrap-payload-bid branch from e8b14ca to e358490 Compare May 22, 2024 03:09

kasey commented May 22, 2024

View reviewed changes

terencechain previously approved these changes May 22, 2024

View reviewed changes

kasey dismissed terencechain’s stale review via bc22b4a May 22, 2024 20:22

kasey added 14 commits May 22, 2024 15:23

engine GetPayload returns entire response

5b0c293

deprecate PayloadValueTo(Gwei|Wei)

f9c7ae9

return entire bid from builder getter

3d5b1a0

get bid value from api bid (not ExecutionData)

6d47b8f

plumb bid and bundle through BuildBlockParallel

62f5acc

rm ValueInGwei

19ec4f2

removing wei/gwei fields from the payload wrappers

0fadc5f

commentary around the little-endian situation

ae7e532

finish the job in BuildBlockParallel

e4ecf63

light self-review cleanup

e2b4539

fix spectest mock

7ebf48e

restore engine timeout

76e3d1b

lint fixes

5cf8763

de-duplicate imports

e3134dd

kasey force-pushed the unwrap-payload-bid branch from bc22b4a to e3134dd Compare May 22, 2024 20:23

remove errant comment

17b05fe

james-prysm reviewed May 22, 2024

View reviewed changes

James feedback

79ec647

terencechain approved these changes May 22, 2024

View reviewed changes

james-prysm approved these changes May 22, 2024

View reviewed changes

kasey enabled auto-merge May 22, 2024 21:58

kasey added this pull request to the merge queue May 22, 2024

Merged via the queue into develop with commit 2265af5 May 22, 2024
17 checks passed

kasey deleted the unwrap-payload-bid branch May 22, 2024 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unwrap payload bid #14035

Unwrap payload bid #14035

kasey commented May 22, 2024

terencechain May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

james-prysm May 22, 2024

kasey May 22, 2024

james-prysm May 22, 2024

kasey May 22, 2024

james-prysm May 22, 2024

kasey May 22, 2024

kasey May 22, 2024

james-prysm May 22, 2024

kasey May 22, 2024

james-prysm May 22, 2024

kasey May 22, 2024 •

edited

Loading

james-prysm May 22, 2024 •

edited

Loading

kasey May 22, 2024

Unwrap payload bid #14035

Unwrap payload bid #14035

Conversation

kasey commented May 22, 2024

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

kasey May 22, 2024 • edited Loading

Choose a reason for hiding this comment

james-prysm May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasey May 22, 2024 •

edited

Loading

james-prysm May 22, 2024 •

edited

Loading