From 26cc87c0de830155b2d5fd95ea7ad93249b16fc7 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 30 Jun 2023 11:19:50 +0800 Subject: [PATCH] cmd, eth/catalyst, miner: terminate block building by ResolveFull --- cmd/geth/config.go | 4 ++-- eth/catalyst/api.go | 4 ++-- eth/catalyst/simulated_beacon.go | 39 +++++++++++++------------------- miner/payload_building.go | 10 ++++++++ 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 529de735447ab..8573120742117 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -192,7 +192,8 @@ func makeFullNode(ctx *cli.Context) (*node.Node, ethapi.Backend) { utils.RegisterFullSyncTester(stack, eth, ctx.Path(utils.SyncTargetFlag.Name)) } - // Start the dev mode if requested + // Start the dev mode if requested, or launch the engine API for + // interacting with external consensus client. if ctx.IsSet(utils.DeveloperFlag.Name) { simBeacon, err := catalyst.NewSimulatedBeacon(ctx.Uint64(utils.DeveloperPeriodFlag.Name), eth) if err != nil { @@ -206,7 +207,6 @@ func makeFullNode(ctx *cli.Context) (*node.Node, ethapi.Backend) { utils.Fatalf("failed to register catalyst service: %v", err) } } - return stack, backend } diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index aa23417eb8246..e4b5533b753f8 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -414,13 +414,13 @@ func (api *ConsensusAPI) getPayload(payloadID engine.PayloadID) (*engine.Executi return data, nil } -// GetFullPayload returns a cached payload by it. The difference is that this +// getFullPayload returns a cached payload by it. The difference is that this // function always expects a non-empty payload, but can also return empty one // if no transaction is executable. // // Note, this function is not a part of standard engine API, meant to be used // by consensus client mock in dev mode. -func (api *ConsensusAPI) GetFullPayload(payloadID engine.PayloadID) (*engine.ExecutionPayloadEnvelope, error) { +func (api *ConsensusAPI) getFullPayload(payloadID engine.PayloadID) (*engine.ExecutionPayloadEnvelope, error) { log.Trace("Engine API request received", "method", "GetFullPayload", "id", payloadID) data := api.localBlocks.get(payloadID, true) if data == nil { diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index 6edf5c91594ca..f0956d262c03b 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -32,12 +32,13 @@ import ( "github.com/ethereum/go-ethereum/rpc" ) -// withdrawalQueue implements a FIFO queue which holds withdrawals that are pending inclusion +// withdrawalQueue implements a FIFO queue which holds withdrawals that are +// pending inclusion. type withdrawalQueue struct { pending chan *types.Withdrawal } -// add queues a withdrawal for future inclusion +// add queues a withdrawal for future inclusion. func (w *withdrawalQueue) add(withdrawal *types.Withdrawal) error { select { case w.pending <- withdrawal: @@ -48,7 +49,7 @@ func (w *withdrawalQueue) add(withdrawal *types.Withdrawal) error { return nil } -// gatherPending returns a number of queued withdrawals up to a maximum count +// gatherPending returns a number of queued withdrawals up to a maximum count. func (w *withdrawalQueue) gatherPending(maxCount int) []*types.Withdrawal { withdrawals := []*types.Withdrawal{} for { @@ -75,7 +76,6 @@ type SimulatedBeacon struct { engineAPI *ConsensusAPI curForkchoiceState engine.ForkchoiceStateV1 - buildWaitTime time.Duration lastBlockTime uint64 } @@ -84,16 +84,16 @@ func NewSimulatedBeacon(period uint64, eth *eth.Ethereum) (*SimulatedBeacon, err if !chainConfig.IsDevMode { return nil, errors.New("incompatible pre-existing chain configuration") } - header := eth.BlockChain().CurrentHeader() + block := eth.BlockChain().CurrentBlock() current := engine.ForkchoiceStateV1{ - HeadBlockHash: header.Hash(), - SafeBlockHash: header.Hash(), - FinalizedBlockHash: header.Hash(), + HeadBlockHash: block.Hash(), + SafeBlockHash: block.Hash(), + FinalizedBlockHash: block.Hash(), } engineAPI := NewConsensusAPI(eth) // if genesis block, send forkchoiceUpdated to trigger transition to PoS - if header.Number.Sign() == 0 { + if block.Number.Sign() == 0 { if _, err := engineAPI.ForkchoiceUpdatedV2(current, nil); err != nil { return nil, err } @@ -102,9 +102,8 @@ func NewSimulatedBeacon(period uint64, eth *eth.Ethereum) (*SimulatedBeacon, err eth: eth, period: period, shutdownCh: make(chan struct{}), - buildWaitTime: time.Millisecond * 100, engineAPI: engineAPI, - lastBlockTime: header.Time, + lastBlockTime: block.Time, curForkchoiceState: current, withdrawals: withdrawalQueue{make(chan *types.Withdrawal, 20)}, }, nil @@ -116,7 +115,7 @@ func (c *SimulatedBeacon) setFeeRecipient(feeRecipient common.Address) { c.feeRecipientLock.Unlock() } -// Start invokes the SimulatedBeacon life-cycle function in a goroutine +// Start invokes the SimulatedBeacon life-cycle function in a goroutine. func (c *SimulatedBeacon) Start() error { if c.period == 0 { go c.loopOnDemand() @@ -126,13 +125,14 @@ func (c *SimulatedBeacon) Start() error { return nil } -// Stop halts the SimulatedBeacon service +// Stop halts the SimulatedBeacon service. func (c *SimulatedBeacon) Stop() error { close(c.shutdownCh) return nil } -// sealBlock initiates payload building for a new block and creates a new block with the completed payload +// sealBlock initiates payload building for a new block and creates a new block +// with the completed payload. func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal) error { tstamp := uint64(time.Now().Unix()) if tstamp <= c.lastBlockTime { @@ -151,17 +151,10 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal) error { return fmt.Errorf("error calling forkchoice update: %v", err) } - envelope, err := c.engineAPI.GetFullPayload(*fcResponse.PayloadID) + envelope, err := c.engineAPI.getFullPayload(*fcResponse.PayloadID) if err != nil { return fmt.Errorf("error retrieving payload: %v", err) } - - // TODO: this is a bit of a hack. need to modify payload builder to stop payload w/ GetFullPayload - _, _ = c.engineAPI.GetPayloadV1(*fcResponse.PayloadID) - if err != nil { - return fmt.Errorf("error retrieving payload: %v", err) - } - payload := envelope.ExecutionPayload // mark the payload as canon @@ -211,7 +204,7 @@ func (c *SimulatedBeacon) loopOnDemand() { // loopOnDemand runs the block production loop for non-zero period configuration func (c *SimulatedBeacon) loop() { - timer := time.NewTimer(time.Duration(0)) + timer := time.NewTimer(0) for { select { case <-c.shutdownCh: diff --git a/miner/payload_building.go b/miner/payload_building.go index 3c2488e1840af..f48be8dfbbb1a 100644 --- a/miner/payload_building.go +++ b/miner/payload_building.go @@ -135,6 +135,7 @@ func (payload *Payload) ResolveEmpty() *engine.ExecutionPayloadEnvelope { } // ResolveFull is basically identical to Resolve, but it expects full block only. +// Don't call Resolve until ResolveFull returns, otherwise it might block forever. func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { payload.lock.Lock() defer payload.lock.Unlock() @@ -145,8 +146,17 @@ func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { return nil default: } + // Wait the full payload construction. Note it might block + // forever if Resolve is called in the meantime which + // terminates the background construction process. payload.cond.Wait() } + // Terminate the background payload construction + select { + case <-payload.stop: + default: + close(payload.stop) + } return engine.BlockToExecutableData(payload.full, payload.fullFees) }