From ca948b85790b25c710a0f7e93677a5d9ef059477 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 7 Nov 2022 15:30:54 +0100 Subject: [PATCH] eth/catalyst, miner: deduplicate work + show payload id (#26115) This PR now also includes a fix to the problem of mult-routines building blocks on the same input. This PR works as before with regards to stopping the work, but it just will not spin up a second routine if one is already building. So if the CL does N calls to FCU+buildblock, and N calls to GetPayload, only the first of each will do something, the other calls will be mostly no-ops. This PR also adds printout of the payload id into the logs. --- eth/catalyst/api.go | 22 ++++++---------------- eth/catalyst/api_test.go | 7 ++++++- eth/catalyst/queue.go | 16 ++++++++++++++++ miner/payload_building.go | 29 +++++++++++++++++++++++++---- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 030a39837472..8dd71f48a7f8 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -18,8 +18,6 @@ package catalyst import ( - "crypto/sha256" - "encoding/binary" "errors" "fmt" "math/big" @@ -288,12 +286,17 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa FeeRecipient: payloadAttributes.SuggestedFeeRecipient, Random: payloadAttributes.Random, } + id := args.Id() + // If we already are busy generating this work, then we do not need + // to start a second process. + if api.localBlocks.has(id) { + return valid(&id), nil + } payload, err := api.eth.Miner().BuildPayload(args) if err != nil { log.Error("Failed to build payload", "err", err) return valid(nil), beacon.InvalidPayloadAttributes.With(err) } - id := computePayloadId(update.HeadBlockHash, payloadAttributes) api.localBlocks.put(id, payload) return valid(&id), nil } @@ -443,19 +446,6 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa return beacon.PayloadStatusV1{Status: beacon.VALID, LatestValidHash: &hash}, nil } -// computePayloadId computes a pseudo-random payloadid, based on the parameters. -func computePayloadId(headBlockHash common.Hash, params *beacon.PayloadAttributesV1) beacon.PayloadID { - // Hash - hasher := sha256.New() - hasher.Write(headBlockHash[:]) - binary.Write(hasher, binary.BigEndian, params.Timestamp) - hasher.Write(params.Random[:]) - hasher.Write(params.SuggestedFeeRecipient[:]) - var out beacon.PayloadID - copy(out[:], hasher.Sum(nil)[:8]) - return out -} - // delayPayloadImport stashes the given block away for import at a later time, // either via a forkchoice update or a sync extension. This method is meant to // be called by the newpayload command when the block seems to be ok, but some diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 18750d6a0323..63f5d19cb9fc 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -184,7 +184,12 @@ func TestEth2PrepareAndGetPayload(t *testing.T) { } // give the payload some time to be built time.Sleep(100 * time.Millisecond) - payloadID := computePayloadId(fcState.HeadBlockHash, &blockParams) + payloadID := (&miner.BuildPayloadArgs{ + Parent: fcState.HeadBlockHash, + Timestamp: blockParams.Timestamp, + FeeRecipient: blockParams.SuggestedFeeRecipient, + Random: blockParams.Random, + }).Id() execData, err := api.GetPayloadV1(payloadID) if err != nil { t.Fatalf("error getting payload, err=%v", err) diff --git a/eth/catalyst/queue.go b/eth/catalyst/queue.go index 6863edfad1ae..c15799487f20 100644 --- a/eth/catalyst/queue.go +++ b/eth/catalyst/queue.go @@ -85,6 +85,22 @@ func (q *payloadQueue) get(id beacon.PayloadID) *beacon.ExecutableDataV1 { return nil } +// has checks if a particular payload is already tracked. +func (q *payloadQueue) has(id beacon.PayloadID) bool { + q.lock.RLock() + defer q.lock.RUnlock() + + for _, item := range q.payloads { + if item == nil { + return false + } + if item.id == id { + return true + } + } + return false +} + // headerQueueItem represents an hash->header tuple to store until it's retrieved // or evicted. type headerQueueItem struct { diff --git a/miner/payload_building.go b/miner/payload_building.go index cdea6a3cc368..2e3ebe356c59 100644 --- a/miner/payload_building.go +++ b/miner/payload_building.go @@ -17,6 +17,8 @@ package miner import ( + "crypto/sha256" + "encoding/binary" "math/big" "sync" "time" @@ -38,12 +40,26 @@ type BuildPayloadArgs struct { Random common.Hash // The provided randomness value } +// Id computes an 8-byte identifier by hashing the components of the payload arguments. +func (args *BuildPayloadArgs) Id() beacon.PayloadID { + // Hash + hasher := sha256.New() + hasher.Write(args.Parent[:]) + binary.Write(hasher, binary.BigEndian, args.Timestamp) + hasher.Write(args.Random[:]) + hasher.Write(args.FeeRecipient[:]) + var out beacon.PayloadID + copy(out[:], hasher.Sum(nil)[:8]) + return out +} + // Payload wraps the built payload(block waiting for sealing). According to the // engine-api specification, EL should build the initial version of the payload // which has an empty transaction set and then keep update it in order to maximize // the revenue. Therefore, the empty-block here is always available and full-block // will be set/updated afterwards. type Payload struct { + id beacon.PayloadID empty *types.Block full *types.Block fullFees *big.Int @@ -53,11 +69,13 @@ type Payload struct { } // newPayload initializes the payload object. -func newPayload(empty *types.Block) *Payload { +func newPayload(empty *types.Block, id beacon.PayloadID) *Payload { payload := &Payload{ + id: id, empty: empty, stop: make(chan struct{}), } + log.Info("Starting work on payload", "id", payload.id) payload.cond = sync.NewCond(&payload.lock) return payload } @@ -80,8 +98,9 @@ func (payload *Payload) update(block *types.Block, fees *big.Int, elapsed time.D payload.fullFees = fees feesInEther := new(big.Float).Quo(new(big.Float).SetInt(fees), big.NewFloat(params.Ether)) - log.Info("Updated payload", "number", block.NumberU64(), "hash", block.Hash(), - "txs", len(block.Transactions()), "gas", block.GasUsed(), "fees", feesInEther, "elapsed", common.PrettyDuration(elapsed)) + 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)) } payload.cond.Broadcast() // fire signal for notifying full block } @@ -139,7 +158,7 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { return nil, err } // Construct a payload object for return. - payload := newPayload(empty) + 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. @@ -164,8 +183,10 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { } 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 } }