From d8dba8d7f9f7e52ad1f1827ac7eb473e75b97771 Mon Sep 17 00:00:00 2001 From: Sam Bukowski Date: Mon, 17 Jul 2023 11:53:05 -0600 Subject: [PATCH] removed sleep before getPayload by removing goroutine in buildPayload --- eth/catalyst/api.go | 2 ++ grpc/execution/server.go | 11 +++------ miner/payload_building.go | 51 ++++++++++++++++++--------------------- 3 files changed, 29 insertions(+), 35 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 7049a45ff2..e5b04df3d1 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -169,6 +169,7 @@ func NewConsensusAPI(eth *eth.Ethereum) *ConsensusAPI { // If there are payloadAttributes: we try to assemble a block with the payloadAttributes // and return its payloadID. func (api *ConsensusAPI) ForkchoiceUpdatedV1(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { + log.Info("ForkchoiceUpdatedV1 called") if payloadAttributes != nil { if payloadAttributes.Withdrawals != nil { return engine.STATUS_INVALID, engine.InvalidParams.With(fmt.Errorf("withdrawals not supported in V1")) @@ -399,6 +400,7 @@ func (api *ConsensusAPI) ExchangeTransitionConfigurationV1(config engine.Transit // GetPayloadV1 returns a cached payload by id. func (api *ConsensusAPI) GetPayloadV1(payloadID engine.PayloadID) (*engine.ExecutableData, error) { + log.Info("GetPayloadV1 called") data, err := api.getPayload(payloadID) if err != nil { return nil, err diff --git a/grpc/execution/server.go b/grpc/execution/server.go index 848be6006c..564503d073 100644 --- a/grpc/execution/server.go +++ b/grpc/execution/server.go @@ -44,7 +44,7 @@ func NewExecutionServiceServer(eth *eth.Ethereum) *ExecutionServiceServer { } func (s *ExecutionServiceServer) DoBlock(ctx context.Context, req *executionv1.DoBlockRequest) (*executionv1.DoBlockResponse, error) { - log.Info("DoBlock called request [sam version]", "request", req) + log.Info("DoBlock called request", "request", req) prevHeadHash := common.BytesToHash(req.PrevBlockHash) // The Engine API has been modified to use transactions from this mempool and abide by it's ordering. @@ -61,17 +61,14 @@ func (s *ExecutionServiceServer) DoBlock(ctx context.Context, req *executionv1.D Random: common.Hash{}, SuggestedFeeRecipient: common.Address{}, } - // NOTE: ForkchoiceUpdatedV1 calls forkchoiceUpdated. forkchoiceUpdated calls api.forkchoiceLock.Lock() - // what is this doing that requires us to wait? + fcStartResp, err := s.consensus.ForkchoiceUpdatedV1(*startForkChoice, payloadAttributes) if err != nil { return nil, err } - // super janky but this is what the payload builder requires :/ (miner.worker.buildPayload()) - // we should probably just execute + store the block directly instead of using the engine api. - // time.Sleep(time.Second) - payloadResp, err := s.consensus.GetPayloadV1(*fcStartResp.PayloadID) // this looks like it is totally fine + // TODO: we should probably just execute + store the block directly instead of using the engine api. + payloadResp, err := s.consensus.GetPayloadV1(*fcStartResp.PayloadID) if err != nil { log.Error("failed to call GetPayloadV1", "err", err) return nil, err diff --git a/miner/payload_building.go b/miner/payload_building.go index f84d908e86..ff124074e5 100644 --- a/miner/payload_building.go +++ b/miner/payload_building.go @@ -163,36 +163,31 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { // Construct a payload object for return. payload := newPayload(empty, args.Id()) - // Spin up a routine for updating the payload in background. This strategy - // can maximum the revenue for including transactions with highest fee. - go func() { - // Setup the timer for re-building the payload. The initial clock is kept - // for triggering process immediately. - timer := time.NewTimer(0) - defer timer.Stop() + // Setup the timer for re-building the payload. The initial clock is kept + // for triggering process immediately. + timer := time.NewTimer(0) + defer timer.Stop() - // Setup the timer for terminating the process if SECONDS_PER_SLOT (12s in - // the Mainnet configuration) have passed since the point in time identified - // by the timestamp parameter. - endTimer := time.NewTimer(time.Second * 12) + // Setup the timer for terminating the process if payload building exceeds 5 seconds. + // This should be much longer than normal payload building should take. + endTimer := time.NewTimer(time.Second * 5) - for { - select { - case <-timer.C: - start := time.Now() - block, fees, err := w.getSealingBlock(args.Parent, args.Timestamp, args.FeeRecipient, args.Random, args.Withdrawals, false) - if err == nil { - payload.update(block, fees, time.Since(start)) - } - timer.Reset(w.recommit) - case <-payload.stop: - log.Info("Stopping work on payload", "id", payload.id, "reason", "delivery") - return - case <-endTimer.C: - log.Info("Stopping work on payload", "id", payload.id, "reason", "timeout") - return + // TODO: figure out if timeout case can be removed + for { + select { + case <-timer.C: + start := time.Now() + block, fees, err := w.getSealingBlock(args.Parent, args.Timestamp, args.FeeRecipient, args.Random, args.Withdrawals, false) + if err == nil { + payload.update(block, fees, time.Since(start)) } + timer.Reset(w.recommit) + case <-payload.stop: + log.Info("Stopping work on payload", "id", payload.id, "reason", "delivery") + return payload, nil + case <-endTimer.C: + log.Info("Stopping work on payload", "id", payload.id, "reason", "timeout") + return payload, nil } - }() - return payload, nil + } }