From 7db76ed83492bc42036e709628b3a526ccddb06a Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Sun, 11 Sep 2022 12:20:08 +0200 Subject: [PATCH 1/5] miner: periodically recommit block --- eth/catalyst/queue.go | 24 ++++++++++++++---------- miner/worker.go | 10 ++++++++-- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/eth/catalyst/queue.go b/eth/catalyst/queue.go index ff8edc1201c4..d555f4666ea5 100644 --- a/eth/catalyst/queue.go +++ b/eth/catalyst/queue.go @@ -55,18 +55,22 @@ func (req *payload) resolve() *beacon.ExecutableDataV1 { // Try to resolve the full block first if it's not obtained // yet. The returned block can be nil if the generation fails. + timeout := time.NewTimer(100 * time.Millisecond) + defer timeout.Stop() + + done := false if !req.done { - timeout := time.NewTimer(500 * time.Millisecond) - defer timeout.Stop() - - select { - case req.block = <-req.result: - req.done = true - case <-timeout.C: - // TODO(rjl49345642, Marius), should we keep this - // 100ms timeout allowance? Why not just use the - // default and then fallback to empty directly? + for !done { + select { + case req.block = <-req.result: + req.done = true + case <-timeout.C: + done = true + // TODO(rjl49345642, Marius), should we keep this + // 100ms timeout allowance? Why not just use the + // default and then fallback to empty directly? + } } } diff --git a/miner/worker.go b/miner/worker.go index 93fb6288bb45..4e3948dbe00c 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -542,6 +542,12 @@ func (w *worker) mainLoop() { req.err <- nil req.result <- block } + if time.Since(time.Unix(int64(req.params.timestamp), 0)) < maxRecommitInterval { + go func() { + time.Sleep(minRecommitInterval) + w.getWorkCh <- req + }() + } case ev := <-w.chainSideCh: // Short circuit for duplicate side blocks if _, exist := w.localUncles[ev.Block.Hash()]; exist { @@ -1172,8 +1178,8 @@ func (w *worker) commit(env *environment, interval func(), update bool, start ti // the generation itself succeeds or not. func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase common.Address, random common.Hash, noTxs bool) (chan *types.Block, chan error, error) { var ( - resCh = make(chan *types.Block, 1) - errCh = make(chan error, 1) + resCh = make(chan *types.Block, 20) + errCh = make(chan error, 20) ) req := &getWorkReq{ params: &generateParams{ From 932bad539e00d315d85d7422fb88d872fede0d39 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Sun, 11 Sep 2022 13:18:22 +0200 Subject: [PATCH 2/5] miner: periodically recommit block --- eth/catalyst/queue.go | 32 +++++++++++++------------------- miner/miner.go | 22 ++++++++++++++++++++-- miner/worker.go | 29 +++++++++++++++++------------ 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/eth/catalyst/queue.go b/eth/catalyst/queue.go index d555f4666ea5..dcdb77edbb75 100644 --- a/eth/catalyst/queue.go +++ b/eth/catalyst/queue.go @@ -23,6 +23,7 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/beacon" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/miner" ) // maxTrackedPayloads is the maximum number of prepared payloads the execution @@ -42,7 +43,7 @@ type payload struct { done bool empty *types.Block block *types.Block - result chan *types.Block + result chan *miner.Work } // resolve extracts the generated full block from the given channel if possible @@ -53,27 +54,20 @@ func (req *payload) resolve() *beacon.ExecutableDataV1 { req.lock.Lock() defer req.lock.Unlock() - // Try to resolve the full block first if it's not obtained - // yet. The returned block can be nil if the generation fails. - timeout := time.NewTimer(100 * time.Millisecond) - defer timeout.Stop() - - done := false - if !req.done { - for !done { - select { - case req.block = <-req.result: - req.done = true - case <-timeout.C: - done = true - // TODO(rjl49345642, Marius), should we keep this - // 100ms timeout allowance? Why not just use the - // default and then fallback to empty directly? - } + timeout := time.NewTimer(500 * time.Millisecond) + defer timeout.Stop() + + select { + case res := <-req.result: + req.block = res.Resolve() + req.done = true + case <-timeout.C: + // TODO(rjl49345642, Marius), should we keep this + // 100ms timeout allowance? Why not just use the + // default and then fallback to empty directly? } } - if req.block != nil { return beacon.BlockToExecutableData(req.block) } diff --git a/miner/miner.go b/miner/miner.go index 1e9607a76ad9..2d1f97f48823 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -245,7 +245,7 @@ func (miner *Miner) SubscribePendingLogs(ch chan<- []*types.Log) event.Subscript // there is always a result that will be returned through the result channel. // The difference is that if the execution fails, the returned result is nil // and the concrete error is dropped silently. -func (miner *Miner) GetSealingBlockAsync(parent common.Hash, timestamp uint64, coinbase common.Address, random common.Hash, noTxs bool) (chan *types.Block, error) { +func (miner *Miner) GetSealingBlockAsync(parent common.Hash, timestamp uint64, coinbase common.Address, random common.Hash, noTxs bool) (chan *Work, error) { resCh, _, err := miner.worker.getSealingBlock(parent, timestamp, coinbase, random, noTxs) if err != nil { return nil, err @@ -261,5 +261,23 @@ func (miner *Miner) GetSealingBlockSync(parent common.Hash, timestamp uint64, co if err != nil { return nil, err } - return <-resCh, <-errCh + res := <-resCh + return res.Resolve(), <-errCh +} + +type Work struct { + b *types.Block + mu *sync.RWMutex +} + +func (w *Work) Resolve() *types.Block { + w.mu.RLock() + defer w.mu.RUnlock() + return w.b +} + +func (w *Work) set(b *types.Block) { + w.mu.Lock() + defer w.mu.Unlock() + w.b = b } diff --git a/miner/worker.go b/miner/worker.go index 4e3948dbe00c..e5352b33070f 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -170,7 +170,8 @@ type newWorkReq struct { // getWorkReq represents a request for getting a new sealing work with provided parameters. type getWorkReq struct { params *generateParams - result chan *types.Block // non-blocking channel + result *Work + resCh chan *Work // non-blocking channel err chan error } @@ -535,13 +536,16 @@ func (w *worker) mainLoop() { case req := <-w.getWorkCh: block, err := w.generateWork(req.params) - if err != nil { + res := &Work{b: block, mu: &sync.RWMutex{}} + if req.result == nil { + req.result = res + req.resCh <- res req.err <- err - req.result <- nil - } else { - req.err <- nil - req.result <- block + } else if err != nil { + // Update the block if no error occured + req.result.set(block) } + if time.Since(time.Unix(int64(req.params.timestamp), 0)) < maxRecommitInterval { go func() { time.Sleep(minRecommitInterval) @@ -1176,10 +1180,10 @@ func (w *worker) commit(env *environment, interval func(), update bool, start ti // getSealingBlock generates the sealing block based on the given parameters. // The generation result will be passed back via the given channel no matter // the generation itself succeeds or not. -func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase common.Address, random common.Hash, noTxs bool) (chan *types.Block, chan error, error) { +func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase common.Address, random common.Hash, noTxs bool) (chan *Work, chan error, error) { var ( - resCh = make(chan *types.Block, 20) - errCh = make(chan error, 20) + result = make(chan *Work, 1) + errCh = make(chan error, 1) ) req := &getWorkReq{ params: &generateParams{ @@ -1192,12 +1196,13 @@ func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase noExtra: true, noTxs: noTxs, }, - result: resCh, - err: errCh, + resCh: result, + err: errCh, } + select { case w.getWorkCh <- req: - return resCh, errCh, nil + return result, errCh, nil case <-w.exitCh: return nil, nil, errors.New("miner closed") } From a84b70150906ba4ddc871708bab2f8e61119e3fc Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Sun, 11 Sep 2022 13:31:10 +0200 Subject: [PATCH 3/5] miner: periodically recommit block --- miner/worker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/worker.go b/miner/worker.go index e5352b33070f..14dfd4b1620e 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -536,7 +536,7 @@ func (w *worker) mainLoop() { case req := <-w.getWorkCh: block, err := w.generateWork(req.params) - res := &Work{b: block, mu: &sync.RWMutex{}} + res := &Work{b: block, mu: new(sync.RWMutex)} if req.result == nil { req.result = res req.resCh <- res From 2847f99531a1f0bbc57beed76179a9e2a5940eb7 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Fri, 16 Sep 2022 11:58:25 +0200 Subject: [PATCH 4/5] miner: fixed tests --- miner/worker_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/miner/worker_test.go b/miner/worker_test.go index 2f1939f75981..30d5829461ac 100644 --- a/miner/worker_test.go +++ b/miner/worker_test.go @@ -644,7 +644,7 @@ func testGetSealingWork(t *testing.T, chainConfig *params.ChainConfig, engine co if err != nil { t.Errorf("Unexpected error %v", err) } - assertBlock(block, c.expectNumber, c.coinbase, c.random) + assertBlock(block.Resolve(), c.expectNumber, c.coinbase, c.random) } } @@ -662,7 +662,7 @@ func testGetSealingWork(t *testing.T, chainConfig *params.ChainConfig, engine co if err != nil { t.Errorf("Unexpected error %v", err) } - assertBlock(block, c.expectNumber, c.coinbase, c.random) + assertBlock(block.Resolve(), c.expectNumber, c.coinbase, c.random) } } } From f02597688254320c8c2f02ba2b6b0b0fb5a0c402 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 19 Sep 2022 17:07:05 +0200 Subject: [PATCH 5/5] happy lint, happy life --- miner/worker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/worker.go b/miner/worker.go index 14dfd4b1620e..00ecd9613672 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -542,7 +542,7 @@ func (w *worker) mainLoop() { req.resCh <- res req.err <- err } else if err != nil { - // Update the block if no error occured + // Update the block if no error occurred req.result.set(block) }