From 6543a278113f1f8035bec9b1dd1410a45ede9d21 Mon Sep 17 00:00:00 2001 From: lystopad Date: Mon, 17 Aug 2026 09:01:11 +0000 Subject: [PATCH] cl/beacon: resolve payload withdrawals in one place (#23280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split out of #23105, which grew too large to review in one piece. `produceBeaconBody` chose between three withdrawal sources inline — a Gloas head whose payload was revealed, a Gloas head whose payload was not, and everything before Gloas — each with its own hand-written conversion loop, and then assembled the payload attributes around whichever it picked. The choice is now a method that names what it selects between, and the attributes come from a single version-aware constructor. What is left inline is the genuinely fork-specific part: the two fields only Gloas sends. The three conversion loops go through the shared converter from hand-written when reviewing that PR. The refactor itself is behaviour-neutral, but the constructor also fixes two cases where the old inline construction built a request the chosen wire version cannot express: - **the parent beacon block root is omitted below Deneb** — Capella and Bellatrix dispatch to `forkchoiceUpdatedV2`/`V1`, and `validatePayloadAttributesPreFCU` rejects a non-nil parent root there with `InvalidPayloadAttributesErr`; - **withdrawals are omitted below Capella** — Bellatrix dispatches to `forkchoiceUpdatedV1`, which has no withdrawals field, and the expectation is a non-nil empty slice, so one was being sent. Both only bite on the engine transport, and both made the request fail rather than be ignored. The routing is otherwise unchanged, including the details that are easy to lose in a refactor: - a FULL Gloas head reads from the state copy with the parent payload applied, not from the head state; - an EMPTY Gloas head reads the expectation the state already cached, and does not compute a fresh one; - before Gloas the expectation is computed from the head state; - the resulting slice keeps its nil-ness in every case. `TestExpectedWithdrawalsReadsTheRightSourcePerFork` gives the two Gloas states different withdrawal outcomes, so it fails if the source selection is removed rather than passing either way. `TestPayloadAttributesOmitFieldsTheChosenVersionCannotCarry` pins the two gates across every fork. Part of a series splitting #23105. (cherry picked from commit af897d9919f9abb212cb97f948731082d0749a8a) --- cl/beacon/handler/block_production.go | 144 +++++++++++---------- cl/beacon/handler/block_production_test.go | 97 ++++++++++++++ 2 files changed, 174 insertions(+), 67 deletions(-) diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index e33f2017658..f459b145a9f 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -222,6 +222,68 @@ func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconCha } } +// 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) } @@ -967,74 +1029,22 @@ func (a *ApiHandler) produceBeaconBody( }() retryTime := 10 * time.Millisecond feeRecipient, _ := a.validatorParams.GetFeeRecipient(proposerIndex) - var withdrawals []*types.Withdrawal - if 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, - }) - } - } else if 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, - }) - } - } - } else { - // 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, diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 4af9fa45a24..f871f3e0022 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/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) + }) + } +}