[ethereum/go-ethereum] miner: streaming uncle blocks (#17320)

karalabe requested changes on this pull request.

Seems generally ok, I only have minor questions / nitpicks.

> @@ -676,31 +683,42 @@ func (w *worker) commitNewWork() {
txs := types.NewTransactionsByPriceAndNonce(w.current.signer, pending)
env.commitTransactions(w.mux, txs, w.chain, w.coinbase)

— // Create the full block to seal with the consensus engine
— fullState = env.state.Copy()
— if fullBlock, err = w.engine.Finalize(w.chain, header, fullState, env.txs, uncles, env.receipts); err != nil {
— log.Error(«Failed to finalize block for sealing», «err», err)
— return
— }
+ w.finalize(uncles, w.fullTaskInterval, true)
+}
+
+// finalize runs any post-transaction state modifications, assembles the final block
+// and commits new work if consensus engine is running.
+func (w *worker) finalize(uncles []*types.Header, interval func(), update bool) error {

Perhaps `commit` would better reflect the purpose of this? The name `finalize` somehow seems to mean to only «finish» some state modification, but nothing more. If we’re actually submitting it for mining, I think that should be more explicit from the method name.

> @@ -676,31 +683,42 @@ func (w *worker) commitNewWork() {
txs := types.NewTransactionsByPriceAndNonce(w.current.signer, pending)
env.commitTransactions(w.mux, txs, w.chain, w.coinbase)

— // Create the full block to seal with the consensus engine
— fullState = env.state.Copy()
— if fullBlock, err = w.engine.Finalize(w.chain, header, fullState, env.txs, uncles, env.receipts); err != nil {
— log.Error(«Failed to finalize block for sealing», «err», err)
— return
— }
+ w.finalize(uncles, w.fullTaskInterval, true)
+}
+
+// finalize runs any post-transaction state modifications, assembles the final block
+// and commits new work if consensus engine is running.
+func (w *worker) finalize(uncles []*types.Header, interval func(), update bool) error {
+ current := w.current

What’s the purpose of this variable closure?

Also minor nitpick, it’s usually cleaner code style if you leave an empty line before a comment like the one below (`// Deep copy […]`). It’s hard on the eyes when code and comments mix tightly. If the previous line is a `}` only by itself, it’s fine to not leave an extra empty line because you already have a visual separation (i.e. almost empty line).

> — cpy := make([]*types.Receipt, len(env.receipts))
— for i, l := range env.receipts {
— cpy[i] = new(types.Receipt)
— *cpy[i] = *l
+ receipts := make([]*types.Receipt, len(current.receipts))
+ for i, l := range current.receipts {
+ receipts[i] = new(types.Receipt)
+ *receipts[i] = *l
+ }
+
+ var (
+ block *types.Block
+ err error
+ )
+ state := current.state.Copy()
+ if block, err = w.engine.Finalize(w.chain, current.header, state, current.txs, uncles, current.receipts); err != nil {

I don’t think there’s value here to pre-declare `block` and `err`. You can just do

«`go
block, err := w.engine.Finalize(w.chain, current.header, state, current.txs, uncles, current.receipts)
if err != nil {
return err
}
«`

> @@ -213,8 +213,9 @@ type worker struct {
running int32 // The indicator whether the consensus engine is running or not.

// Test hooks
— newTaskHook func(*task) // Method to call upon receiving a new sealing task
— fullTaskInterval func() // Method to call before pushing the full sealing task
+ newTaskHook func(*task) // Method to call upon receiving a new sealing task
+ skipSealHook func(*task) bool // Method to decide whether skipping the sealing.
+ fullTaskInterval func() // Method to call before pushing the full sealing task

This seems an odd name. Is `fullTaskInterval` only a test hook? If yes, please call it `fullTaskHook` to make it clear that it’s a mock method, not a generally used one.

> select {
— case w.taskCh <- &task{receipts: cpy, state: fullState, block: fullBlock, createdAt: time.Now()}: - w.unconfirmed.Shift(fullBlock.NumberU64() - 1) - log.Info("Commit new full mining work", "number", fullBlock.Number(), "txs", env.tcount, "uncles", len(uncles), "elapsed", common.PrettyDuration(time.Since(tstart))) + case w.taskCh <- &task{receipts: receipts, state: state, block: block, createdAt: time.Now()}: + w.unconfirmed.Shift(block.NumberU64() - 1) + log.Info("Commit new mining work", "number", block.Number(), "txs", current.tcount, "uncles", len(uncles)) The previous log code had the elapsed time printed too. For empty and uncle blocks that's irrelevant, true, but for a full block, it might be important to highlight the time it took to run the transactions to see any potential issues. Perhaps we should somehow retain this functionality?

Добавить комментарий