From d6e2d4b93dce82406a4581cf2c54376a8b3cb933 Mon Sep 17 00:00:00 2001 From: Damilola Edwards Date: Mon, 17 Aug 2026 15:27:46 +0100 Subject: [PATCH] Clear the candidate build marker on a failed attempt executeCandidateBuild marked a (slot, parent-tuple) candidate as started before attempting the build, but never cleared that marker when the build failed - either the engine call itself or the payload transform step. A single transient error permanently blocked any retry of that candidate for the rest of the slot, including a legitimate CL-client attributes redelivery for the exact same parent, which was silently dropped instead of retried. Both failure paths now clear the marker before returning, so a later trigger for the same tuple gets a real attempt instead of being dropped. --- pkg/payload_builder/candidate_retry_test.go | 94 +++++++++++++++++++++ pkg/payload_builder/service.go | 22 +++++ 2 files changed, 116 insertions(+) create mode 100644 pkg/payload_builder/candidate_retry_test.go diff --git a/pkg/payload_builder/candidate_retry_test.go b/pkg/payload_builder/candidate_retry_test.go new file mode 100644 index 0000000..1c3d192 --- /dev/null +++ b/pkg/payload_builder/candidate_retry_test.go @@ -0,0 +1,94 @@ +package payload_builder + +// Test for NM-13: slotBuildState.started (the per-candidate build dedup map) +// used to be set before a build attempt and never cleared on failure, so a +// single transient engine error permanently blocked any retry of that +// (slot, parent-tuple) candidate for the rest of the slot -- including a +// legitimate CL-client attributes redelivery for the exact same parent. + +import ( + "context" + "testing" + "time" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethpandaops/go-eth2-client/spec/phase0" + "github.com/ethpandaops/go-eth2-client/spec/version" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + + "github.com/ethpandaops/buildoor/pkg/action_plan" + "github.com/ethpandaops/buildoor/pkg/chain" + "github.com/ethpandaops/buildoor/pkg/config" + "github.com/ethpandaops/buildoor/pkg/rpc/beacon" +) + +// unknownForkChainService forces every build attempt to fail deterministically +// and immediately: chain.EngineVersion(DataVersionUnknown) errors before any +// engine or beacon-API call is made, so engineClient/clClient can stay nil, +// matching the existing newSkipTestService pattern in this package. +type unknownForkChainService struct { + stubChainService +} + +func (s *unknownForkChainService) ActiveForkAtEpoch(phase0.Epoch) version.DataVersion { + return version.DataVersionUnknown +} + +func TestExecuteCandidateBuild_RetriesAfterAFailedAttempt(t *testing.T) { + chainSvc := &unknownForkChainService{stubChainService{spec: &chain.ChainSpec{ + SecondsPerSlot: 12 * time.Second, + SlotsPerEpoch: 32, + }}} + + log := logrus.New() + log.SetLevel(logrus.PanicLevel) + + cfg := config.DefaultConfig() + planSvc := action_plan.NewPlanService(cfg, chainSvc, log) + + svc, err := NewService(cfg, nil, chainSvc, planSvc, nil, common.Address{}, log) + require.NoError(t, err) + + // Set up the state Start() would normally set up, without calling Start() + // itself (which needs a live beacon client for its event stream). + svc.ctx = context.Background() + svc.payloadBuilder = NewPayloadBuilder(nil, nil, chainSvc, common.Address{}, cfg, log, nil) + + sub := svc.SubscribePayloadBuildFailed(4, false) + defer sub.Unsubscribe() + + slot := phase0.Slot(500) + attrs := &beacon.PayloadAttributesEvent{ + ProposalSlot: slot, + ParentBlockRoot: phase0.Root{0x11}, + ParentBlockHash: phase0.Hash32{0x22}, + } + target := &buildTarget{candidate: chain.CandidateParentFull, attrs: attrs} + + // First attempt fails (the unknown-fork stand-in for a transient engine + // error) and marks the tuple started. + svc.executeCandidateBuild(slot, target) + + select { + case event := <-sub.Channel(): + require.Equal(t, slot, event.Slot) + case <-time.After(time.Second): + t.Fatal("expected the first build attempt to fail and fire buildFailedDispatcher") + } + + // A fresh payload_attributes redelivery for the EXACT SAME parent tuple + // arrives -- a real CL client behavior (reorgs, retries, some clients + // simply re-emit payload_attributes). NM-13 fixed: this must actually + // retry, not be silently dropped because the tuple is still marked + // "started" from the failed attempt. + svc.executeCandidateBuild(slot, target) + + select { + case event := <-sub.Channel(): + require.Equal(t, slot, event.Slot) + case <-time.After(time.Second): + t.Fatal("NM-13 regression: the second attempt for the same parent tuple never ran " + + "-- the started marker was not cleared after the first failure") + } +} diff --git a/pkg/payload_builder/service.go b/pkg/payload_builder/service.go index 51b8348..116911e 100644 --- a/pkg/payload_builder/service.go +++ b/pkg/payload_builder/service.go @@ -480,6 +480,20 @@ func newSlotBuildState() *slotBuildState { return &slotBuildState{started: make(map[beacon.AttrParentKey]bool, 4)} } +// clearBuildStarted un-marks a (slot, parent-tuple) candidate as started +// after a failed build attempt, so a later trigger for the exact same tuple +// (a fresh payload_attributes redelivery, a late-build check, ...) is free +// to retry it instead of being silently dropped by the started check in +// executeCandidateBuild for the rest of the slot. +func (s *Service) clearBuildStarted(slot phase0.Slot, tuple beacon.AttrParentKey) { + s.scheduledBuildMu.Lock() + defer s.scheduledBuildMu.Unlock() + + if state := s.slotBuilds[slot]; state != nil { + delete(state.started, tuple) + } +} + // maybeLateBuild activates a candidate build for an attributes variant that // arrived after the slot's build pass already ran: the chain moved (reorg, // payload-miss flip, late reveal) and the new parent still deserves a payload @@ -826,6 +840,12 @@ func (s *Service) executeCandidateBuild(slot phase0.Slot, target *buildTarget) { FailedAt: time.Now(), }) + // A transient engine error must not permanently forfeit this parent + // tuple for the rest of the slot: clear the started marker so a + // later trigger (a fresh payload_attributes for the same tuple, a + // late-build check, ...) can retry it. + s.clearBuildStarted(slot, tuple) + return } @@ -849,6 +869,8 @@ func (s *Service) executeCandidateBuild(slot phase0.Slot, target *buildTarget) { FailedAt: time.Now(), }) + s.clearBuildStarted(slot, tuple) + return }