From 07da7a9795f5f0c17d9f0a9692297651df6f6c95 Mon Sep 17 00:00:00 2001 From: Sam Bukowski Date: Fri, 14 Jul 2023 15:18:18 -0600 Subject: [PATCH 1/4] simple logging change --- eth/catalyst/api.go | 1 + grpc/execution/server.go | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 9077f20bff..7049a45ff2 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -205,6 +205,7 @@ func (api *ConsensusAPI) verifyPayloadAttributes(attr *engine.PayloadAttributes) return nil } +// TODO: figure out the timing here func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { api.forkchoiceLock.Lock() defer api.forkchoiceLock.Unlock() diff --git a/grpc/execution/server.go b/grpc/execution/server.go index 5a46ffd9e0..848be6006c 100644 --- a/grpc/execution/server.go +++ b/grpc/execution/server.go @@ -7,7 +7,6 @@ package execution import ( "context" "fmt" - "time" "github.com/ethereum/go-ethereum/beacon/engine" "github.com/ethereum/go-ethereum/common" @@ -45,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", "request", req) + log.Info("DoBlock called request [sam version]", "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. @@ -62,6 +61,8 @@ 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 @@ -69,8 +70,8 @@ func (s *ExecutionServiceServer) DoBlock(ctx context.Context, req *executionv1.D // 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) + // time.Sleep(time.Second) + payloadResp, err := s.consensus.GetPayloadV1(*fcStartResp.PayloadID) // this looks like it is totally fine if err != nil { log.Error("failed to call GetPayloadV1", "err", err) return nil, err From d8dba8d7f9f7e52ad1f1827ac7eb473e75b97771 Mon Sep 17 00:00:00 2001 From: Sam Bukowski Date: Mon, 17 Jul 2023 11:53:05 -0600 Subject: [PATCH 2/4] 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 + } } From 100f133d4961a9b4a056874bd4a20b78dd948830 Mon Sep 17 00:00:00 2001 From: Sam Bukowski Date: Mon, 17 Jul 2023 12:34:11 -0600 Subject: [PATCH 3/4] removed unneeded comment --- eth/catalyst/api.go | 1 - 1 file changed, 1 deletion(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index e5b04df3d1..c63305d32d 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -206,7 +206,6 @@ func (api *ConsensusAPI) verifyPayloadAttributes(attr *engine.PayloadAttributes) return nil } -// TODO: figure out the timing here func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { api.forkchoiceLock.Lock() defer api.forkchoiceLock.Unlock() From 23275e0934d6d96e252af5f54c97f54bedcd2c32 Mon Sep 17 00:00:00 2001 From: Sam Bukowski Date: Mon, 17 Jul 2023 15:46:22 -0600 Subject: [PATCH 4/4] removed for loop from buildPayload --- miner/payload_building.go | 42 +++++++++++---------------------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/miner/payload_building.go b/miner/payload_building.go index ff124074e5..08c772bacb 100644 --- a/miner/payload_building.go +++ b/miner/payload_building.go @@ -21,7 +21,6 @@ import ( "encoding/binary" "math/big" "sync" - "time" "github.com/ethereum/go-ethereum/beacon/engine" "github.com/ethereum/go-ethereum/common" @@ -84,7 +83,7 @@ func newPayload(empty *types.Block, id engine.PayloadID) *Payload { } // update updates the full-block with latest built version. -func (payload *Payload) update(block *types.Block, fees *big.Int, elapsed time.Duration) { +func (payload *Payload) update(block *types.Block, fees *big.Int) { payload.lock.Lock() defer payload.lock.Unlock() @@ -103,7 +102,7 @@ func (payload *Payload) update(block *types.Block, fees *big.Int, elapsed time.D feesInEther := new(big.Float).Quo(new(big.Float).SetInt(fees), big.NewFloat(params.Ether)) log.Info("Updated payload", "id", payload.id, "number", block.NumberU64(), "hash", block.Hash(), "txs", len(block.Transactions()), "gas", block.GasUsed(), "fees", feesInEther, - "root", block.Root(), "elapsed", common.PrettyDuration(elapsed)) + "root", block.Root()) } payload.cond.Broadcast() // fire signal for notifying full block } @@ -160,34 +159,17 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { if err != nil { return nil, err } - // Construct a payload object for return. + // Construct a payload object for updating the block. payload := newPayload(empty, args.Id()) - // 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 payload building exceeds 5 seconds. - // This should be much longer than normal payload building should take. - endTimer := time.NewTimer(time.Second * 5) - - // 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 - } + // Get the block to update with the payload + block, fees, err := w.getSealingBlock(args.Parent, args.Timestamp, args.FeeRecipient, args.Random, args.Withdrawals, false) + if err == nil { + payload.update(block, fees) + log.Info("Stopping work on payload", "id", payload.id, "reason", "delivery") + return payload, nil + } else { + log.Info("Stopping work on payload", "id", payload.id, "reason", "failed to retrieve payload") + return payload, nil } }