cl/beacon: resolve payload withdrawals in one place

cl/beacon/handler/block_production.go

@@ -227,6 +227,68 @@
 	}
 }
 
+// payloadAttributes builds the attributes for a version of the forkchoice call, which is the one
+// place that decides which fields each version carries. A field the chosen version does not define
+// is left unpopulated rather than filled in and ignored: V1 has no withdrawals and V1 and V2 no
+// parent beacon block root, and an execution client rejects a request that supplies them.
+func payloadAttributes(
+	version clparams.StateVersion,
+	timestamp hexutil.Uint64,
+	prevRandao common.Hash,
+	feeRecipient common.Address,
+	withdrawals []*types.Withdrawal,
+	parentRoot *common.Hash,
+	slotNumber, targetGasLimit *hexutil.Uint64,
+) *engine_types.PayloadAttributes {
+	attrs := &engine_types.PayloadAttributes{
+		Timestamp:             timestamp,
+		PrevRandao:            prevRandao,
+		SuggestedFeeRecipient: feeRecipient,
+	}
+	if version.AfterOrEqual(clparams.CapellaVersion) {
+		attrs.Withdrawals = withdrawals
+	}
+	if version.AfterOrEqual(clparams.DenebVersion) {
+		attrs.ParentBeaconBlockRoot = parentRoot
+	}
+	if version.AfterOrEqual(clparams.GloasVersion) {
+		attrs.SlotNumber = slotNumber
+		attrs.TargetGasLimit = targetGasLimit
+	}
+	return attrs
+}
+
+// expectedWithdrawals resolves the withdrawals for the payload being built. Under Gloas the source
+// depends on whether the head's payload was revealed: a FULL head is read from the state copy with
+// that payload applied, an EMPTY one from the expectation the state already cached.
+func (a *ApiHandler) expectedWithdrawals(
+	baseState, withParentPayload *state.CachingBeaconState,
+	stateVersion clparams.StateVersion,
+	targetSlot uint64,
+) ([]*types.Withdrawal, error) {
+	epoch := targetSlot / a.beaconChainCfg.SlotsPerEpoch
+	if stateVersion.Before(clparams.GloasVersion) || withParentPayload != nil {
+		source := baseState
+		if withParentPayload != nil {
+			source = withParentPayload
+		}
+		clWithdrawals, err := state.GetExpectedWithdrawals(source, epoch)
+		if err != nil {
+			return nil, err
+		}
+		return cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(clWithdrawals.Withdrawals), nil
+	}
+	cached := baseState.GetPayloadExpectedWithdrawals()
+	if cached == nil {
+		return nil, nil
+	}
+	consensusWithdrawals := make([]*cltypes.Withdrawal, cached.Len())
+	for i := range consensusWithdrawals {
+		consensusWithdrawals[i] = cached.Get(i)
+	}
+	return cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals), nil
+}
+
 func shouldRetryGetPayload(now, deadline time.Time) bool {
 	return now.Before(deadline)
 }
@@ -976,75 +1038,22 @@
 		}()
 		retryTime := 10 * time.Millisecond
 		feeRecipient, _ := a.validatorParams.GetFeeRecipient(proposerIndex)
-		var withdrawals []*types.Withdrawal
-		switch {
-		case gloasWithdrawalsState != nil:
-			// GLOAS FULL: compute withdrawals from the state copy with parent payload applied
-			clWithdrawals, err := state.GetExpectedWithdrawals(
-				gloasWithdrawalsState,
-				targetSlot/a.beaconChainCfg.SlotsPerEpoch,
-			)
-			if err != nil {
-				log.Error("BlockProduction: GetExpectedWithdrawals (FULL) failed", "err", err)
-				return
-			}
-			withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals))
-			for _, w := range clWithdrawals.Withdrawals {
-				withdrawals = append(withdrawals, &types.Withdrawal{
-					Index:     w.Index,
-					Amount:    w.Amount,
-					Validator: w.Validator,
-					Address:   w.Address,
-				})
-			}
-		case stateVersion >= clparams.GloasVersion && gloasWithdrawalsState == nil:
-			// GLOAS EMPTY: use cached payload_expected_withdrawals from state
-			cachedWithdrawals := baseState.GetPayloadExpectedWithdrawals()
-			if cachedWithdrawals != nil {
-				withdrawals = make([]*types.Withdrawal, 0, cachedWithdrawals.Len())
-				for i := 0; i < cachedWithdrawals.Len(); i++ {
-					w := cachedWithdrawals.Get(i)
-					withdrawals = append(withdrawals, &types.Withdrawal{
-						Index:     w.Index,
-						Amount:    w.Amount,
-						Validator: w.Validator,
-						Address:   w.Address,
-					})
-				}
-			}
-		default:
-			// Pre-GLOAS: compute withdrawals normally
-			clWithdrawals, err := state.GetExpectedWithdrawals(
-				baseState,
-				targetSlot/a.beaconChainCfg.SlotsPerEpoch,
-			)
-			if err != nil {
-				log.Error("BlockProduction: GetExpectedWithdrawals failed", "err", err)
-				return
-			}
-			withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals))
-			for _, w := range clWithdrawals.Withdrawals {
-				withdrawals = append(withdrawals, &types.Withdrawal{
-					Index:     w.Index,
-					Amount:    w.Amount,
-					Validator: w.Validator,
-					Address:   w.Address,
-				})
-			}
-		}
-
-		attrs := &engine_types.PayloadAttributes{
-			Timestamp:             hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)),
-			PrevRandao:            random,
-			SuggestedFeeRecipient: feeRecipient,
-			Withdrawals:           withdrawals,
-			ParentBeaconBlockRoot: (*common.Hash)(&blockRoot),
-		}
-		if stateVersion.AfterOrEqual(clparams.GloasVersion) {
-			sn := hexutil.Uint64(targetSlot)
-			attrs.SlotNumber = &sn
-			attrs.TargetGasLimit = targetGasLimit
+		withdrawals, err := a.expectedWithdrawals(baseState, gloasWithdrawalsState, stateVersion, targetSlot)
+		if err != nil {
+			log.Error("BlockProduction: GetExpectedWithdrawals failed", "err", err)
+			return
 		}
+		slotNumber := hexutil.Uint64(targetSlot)
+		attrs := payloadAttributes(
+			stateVersion,
+			hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)),
+			random,
+			feeRecipient,
+			withdrawals,
+			(*common.Hash)(&blockRoot),
+			&slotNumber,
+			targetGasLimit,
+		)
 		builderStartedAt := time.Now()
 		idBytes, err := a.engine.ForkChoiceUpdate(
 			ctx,
@@ -1778,7 +1787,7 @@
 	return nil, errors.New("invalid content type")
 }
 
 func (a *ApiHandler) broadcastBlock(ctx context.Context, blk *cltypes.SignedBeaconBlock, signedEnvelope ...*cltypes.SignedExecutionPayloadEnvelope) error {
 	blkSSZ, err := blk.EncodeSSZ(nil)
 	if err != nil {
 		return err
@@ -2162,7 +2171,7 @@
 	reward      uint64
 }
 
 func (a *ApiHandler) electraMergedAttestationCandidates(s abstract.BeaconState) (map[common.Hash][]*solid.Attestation, error) {
 	pool := map[common.Hash]map[uint64][]*solid.Attestation{} // map root -> committee -> att candidates
 	// step 1: Group attestations by data root and committee index for merging
 	// so after this step, pool[dataRoot][committeeIndex] will contain all the attestation candidates for that data root and committee index

cl/beacon/handler/block_production_test.go

@@ -36,6 +36,7 @@ import (
 	"github.com/erigontech/erigon/cl/clparams"
 	"github.com/erigontech/erigon/cl/cltypes"
 	"github.com/erigontech/erigon/cl/cltypes/solid"
+	"github.com/erigontech/erigon/cl/phase1/core/state"
 	"github.com/erigontech/erigon/cl/phase1/execution_client"
 	"github.com/erigontech/erigon/common"
 	"github.com/erigontech/erigon/common/hexutil"
@@ -732,3 +733,99 @@ func TestCaplinBlockProductionGlamsterdamSlotNumber(t *testing.T) {
 	require.Equal(t, hexutil.Uint64(targetSlot), *spy.lastAttributes.SlotNumber,
 		"SlotNumber should equal the target slot")
 }
+
+func TestExpectedWithdrawalsReadsTheRightSourcePerFork(t *testing.T) {
+	cfg := clparams.MainnetBeaconConfig
+	cfg.AltairForkEpoch, cfg.BellatrixForkEpoch, cfg.CapellaForkEpoch = 0, 0, 0
+	a := &ApiHandler{beaconChainCfg: &cfg}
+
+	capellaState := state.New(&cfg)
+	capellaState.SetVersion(clparams.CapellaVersion)
+
+	// Before Gloas the expectation is computed from the head state itself, and the list is present
+	// even when empty: the execution layer rejects a nil one after Shanghai.
+	withdrawals, err := a.expectedWithdrawals(capellaState, nil, clparams.CapellaVersion, 0)
+	require.NoError(t, err)
+	require.NotNil(t, withdrawals)
+	require.Empty(t, withdrawals)
+
+	gloasState := state.New(&cfg)
+	gloasState.SetVersion(clparams.GloasVersion)
+
+	// A Gloas head whose payload was revealed is read from the state copy carrying that payload,
+	// not from the head state. Only that copy carries a pending builder withdrawal, so reading the
+	// wrong one comes back empty rather than merely equal.
+	withParentPayload := state.New(&cfg)
+	withParentPayload.SetVersion(clparams.GloasVersion)
+	pending := solid.NewDynamicListSSZ[*cltypes.BuilderPendingWithdrawal](int(cfg.MaxWithdrawalsPerPayload))
+	pending.Append(&cltypes.BuilderPendingWithdrawal{FeeRecipient: common.Address{0xbb}, Amount: 12, BuilderIndex: 3})
+	withParentPayload.SetBuilderPendingWithdrawals(pending)
+
+	withdrawals, err = a.expectedWithdrawals(gloasState, withParentPayload, clparams.GloasVersion, 0)
+	require.NoError(t, err)
+	require.Equal(t, []*types.Withdrawal{{
+		Index:     0,
+		Validator: state.ConvertBuilderIndexToValidatorIndex(3),
+		Address:   common.Address{0xbb},
+		Amount:    12,
+	}}, withdrawals)
+
+	// An EMPTY Gloas head uses the expectation the state already cached rather than computing a
+	// fresh one, so what it returns is whatever was cached.
+	withdrawals, err = a.expectedWithdrawals(gloasState, nil, clparams.GloasVersion, 0)
+	require.NoError(t, err)
+	require.Empty(t, withdrawals)
+
+	cached := solid.NewDynamicListSSZ[*cltypes.Withdrawal](int(cfg.MaxWithdrawalsPerPayload))
+	cached.Append(&cltypes.Withdrawal{Index: 7, Validator: 8, Address: common.Address{0xaa}, Amount: 9})
+	gloasState.SetPayloadExpectedWithdrawals(cached)
+	withdrawals, err = a.expectedWithdrawals(gloasState, nil, clparams.GloasVersion, 0)
+	require.NoError(t, err)
+	require.Equal(t, []*types.Withdrawal{
+		{Index: 7, Validator: 8, Address: common.Address{0xaa}, Amount: 9},
+	}, withdrawals)
+}
+
+func TestPayloadAttributesOmitFieldsTheChosenVersionCannotCarry(t *testing.T) {
+	root := common.Hash{0xaa}
+	withdrawals := []*types.Withdrawal{{Index: 1}}
+	slotNumber := hexutil.Uint64(64)
+	targetGasLimit := hexutil.Uint64(36_000_000)
+
+	for _, tc := range []struct {
+		version         clparams.StateVersion
+		wantWithdrawals bool
+		wantParentRoot  bool
+		wantGloasFields bool
+	}{
+		{clparams.BellatrixVersion, false, false, false},
+		{clparams.CapellaVersion, true, false, false},
+		{clparams.DenebVersion, true, true, false},
+		{clparams.FuluVersion, true, true, false},
+		{clparams.GloasVersion, true, true, true},
+	} {
+		t.Run(tc.version.String(), func(t *testing.T) {
+			attrs := payloadAttributes(tc.version, 1, common.Hash{0xbb}, common.Address{0xcc},
+				withdrawals, &root, &slotNumber, &targetGasLimit)
+
+			// A version that does not define a field must not have it populated: V1 carries no
+			// withdrawals, V1 and V2 no parent beacon block root, and supplying one is rejected
+			// rather than ignored.
+			require.Equal(t, tc.wantWithdrawals, attrs.Withdrawals != nil)
+			require.Equal(t, tc.wantParentRoot, attrs.ParentBeaconBlockRoot != nil)
+
+			// The values have to arrive, not merely be non-nil: dropping either Gloas field leaves
+			// every Gloas proposal rejected with -38003.
+			if tc.wantGloasFields {
+				require.Equal(t, &slotNumber, attrs.SlotNumber)
+				require.Equal(t, &targetGasLimit, attrs.TargetGasLimit)
+			} else {
+				require.Nil(t, attrs.SlotNumber)
+				require.Nil(t, attrs.TargetGasLimit)
+			}
+			require.Equal(t, hexutil.Uint64(1), attrs.Timestamp)
+			require.Equal(t, common.Hash{0xbb}, attrs.PrevRandao)
+			require.Equal(t, common.Address{0xcc}, attrs.SuggestedFeeRecipient)
+		})
+	}
+}