From f751c6ed47db49e0032dfcd0f4c06ce72f3616bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Tue, 28 Aug 2018 16:48:20 +0300 Subject: [PATCH] miner: track uncles more aggressively --- miner/worker.go | 32 ++++++++--------- miner/worker_test.go | 82 ++++++++++++++++++++++++++------------------ 2 files changed, 63 insertions(+), 51 deletions(-) diff --git a/miner/worker.go b/miner/worker.go index ca68da6e9..99f1f8dcf 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -18,7 +18,7 @@ package miner import ( "bytes" - "fmt" + "errors" "math/big" "sync" "sync/atomic" @@ -615,13 +615,16 @@ func (w *worker) makeCurrent(parent *types.Block, header *types.Header) error { func (w *worker) commitUncle(env *environment, uncle *types.Header) error { hash := uncle.Hash() if env.uncles.Contains(hash) { - return fmt.Errorf("uncle not unique") + return errors.New("uncle not unique") + } + if env.header.ParentHash == uncle.ParentHash { + return errors.New("uncle is sibling") } if !env.ancestors.Contains(uncle.ParentHash) { - return fmt.Errorf("uncle's parent unknown (%x)", uncle.ParentHash[0:4]) + return errors.New("uncle's parent unknown") } if env.family.Contains(hash) { - return fmt.Errorf("uncle already in family (%x)", hash) + return errors.New("uncle already included") } env.uncles.Add(uncle.Hash()) return nil @@ -847,29 +850,24 @@ func (w *worker) commitNewWork(interrupt *int32, noempty bool) { if w.config.DAOForkSupport && w.config.DAOForkBlock != nil && w.config.DAOForkBlock.Cmp(header.Number) == 0 { misc.ApplyDAOHardFork(env.state) } - - // compute uncles for the new block. - var ( - uncles []*types.Header - badUncles []common.Hash - ) + // Accumulate the uncles for the current block + for hash, uncle := range w.possibleUncles { + if uncle.NumberU64()+staleThreshold <= header.Number.Uint64() { + delete(w.possibleUncles, hash) + } + } + uncles := make([]*types.Header, 0, 2) for hash, uncle := range w.possibleUncles { if len(uncles) == 2 { break } if err := w.commitUncle(env, uncle.Header()); err != nil { - log.Trace("Bad uncle found and will be removed", "hash", hash) - log.Trace(fmt.Sprint(uncle)) - - badUncles = append(badUncles, hash) + log.Trace("Possible uncle rejected", "hash", hash, "reason", err) } else { log.Debug("Committing new uncle to block", "hash", hash) uncles = append(uncles, uncle.Header()) } } - for _, hash := range badUncles { - delete(w.possibleUncles, hash) - } if !noempty { // Create an empty block based on temporary copied state for sealing in advance without waiting block diff --git a/miner/worker_test.go b/miner/worker_test.go index 16708c18c..40a57d137 100644 --- a/miner/worker_test.go +++ b/miner/worker_test.go @@ -45,8 +45,8 @@ var ( testBankAddress = crypto.PubkeyToAddress(testBankKey.PublicKey) testBankFunds = big.NewInt(1000000000000000000) - acc1Key, _ = crypto.GenerateKey() - acc1Addr = crypto.PubkeyToAddress(acc1Key.PublicKey) + testUserKey, _ = crypto.GenerateKey() + testUserAddress = crypto.PubkeyToAddress(testUserKey.PublicKey) // Test transactions pendingTxs []*types.Transaction @@ -62,9 +62,9 @@ func init() { Period: 10, Epoch: 30000, } - tx1, _ := types.SignTx(types.NewTransaction(0, acc1Addr, big.NewInt(1000), params.TxGas, nil, nil), types.HomesteadSigner{}, testBankKey) + tx1, _ := types.SignTx(types.NewTransaction(0, testUserAddress, big.NewInt(1000), params.TxGas, nil, nil), types.HomesteadSigner{}, testBankKey) pendingTxs = append(pendingTxs, tx1) - tx2, _ := types.SignTx(types.NewTransaction(1, acc1Addr, big.NewInt(1000), params.TxGas, nil, nil), types.HomesteadSigner{}, testBankKey) + tx2, _ := types.SignTx(types.NewTransaction(1, testUserAddress, big.NewInt(1000), params.TxGas, nil, nil), types.HomesteadSigner{}, testBankKey) newTxs = append(newTxs, tx2) } @@ -77,7 +77,7 @@ type testWorkerBackend struct { uncleBlock *types.Block } -func newTestWorkerBackend(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) *testWorkerBackend { +func newTestWorkerBackend(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine, n int) *testWorkerBackend { var ( db = ethdb.NewMemDatabase() gspec = core.Genesis{ @@ -92,14 +92,28 @@ func newTestWorkerBackend(t *testing.T, chainConfig *params.ChainConfig, engine copy(gspec.ExtraData[32:], testBankAddress[:]) case *ethash.Ethash: default: - t.Fatal("unexpect consensus engine type") + t.Fatalf("unexpected consensus engine type: %T", engine) } genesis := gspec.MustCommit(db) chain, _ := core.NewBlockChain(db, nil, gspec.Config, engine, vm.Config{}) txpool := core.NewTxPool(testTxPoolConfig, chainConfig, chain) - blocks, _ := core.GenerateChain(chainConfig, genesis, engine, db, 1, func(i int, gen *core.BlockGen) { - gen.SetCoinbase(acc1Addr) + + // Generate a small n-block chain and an uncle block for it + if n > 0 { + blocks, _ := core.GenerateChain(chainConfig, genesis, engine, db, n, func(i int, gen *core.BlockGen) { + gen.SetCoinbase(testBankAddress) + }) + if _, err := chain.InsertChain(blocks); err != nil { + t.Fatalf("failed to insert origin chain: %v", err) + } + } + parent := genesis + if n > 0 { + parent = chain.GetBlockByHash(chain.CurrentBlock().ParentHash()) + } + blocks, _ := core.GenerateChain(chainConfig, parent, engine, db, 1, func(i int, gen *core.BlockGen) { + gen.SetCoinbase(testUserAddress) }) return &testWorkerBackend{ @@ -116,8 +130,8 @@ func (b *testWorkerBackend) PostChainEvents(events []interface{}) { b.chain.PostChainEvents(events, nil) } -func newTestWorker(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) (*worker, *testWorkerBackend) { - backend := newTestWorkerBackend(t, chainConfig, engine) +func newTestWorker(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine, blocks int) (*worker, *testWorkerBackend) { + backend := newTestWorkerBackend(t, chainConfig, engine, blocks) backend.txPool.AddLocals(pendingTxs) w := newWorker(chainConfig, engine, backend, new(event.TypeMux), time.Second) w.setEtherbase(testBankAddress) @@ -134,24 +148,24 @@ func TestPendingStateAndBlockClique(t *testing.T) { func testPendingStateAndBlock(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) { defer engine.Close() - w, b := newTestWorker(t, chainConfig, engine) + w, b := newTestWorker(t, chainConfig, engine, 0) defer w.close() // Ensure snapshot has been updated. time.Sleep(100 * time.Millisecond) block, state := w.pending() if block.NumberU64() != 1 { - t.Errorf("block number mismatch, has %d, want %d", block.NumberU64(), 1) + t.Errorf("block number mismatch: have %d, want %d", block.NumberU64(), 1) } - if balance := state.GetBalance(acc1Addr); balance.Cmp(big.NewInt(1000)) != 0 { - t.Errorf("account balance mismatch, has %d, want %d", balance, 1000) + if balance := state.GetBalance(testUserAddress); balance.Cmp(big.NewInt(1000)) != 0 { + t.Errorf("account balance mismatch: have %d, want %d", balance, 1000) } b.txPool.AddLocals(newTxs) // Ensure the new tx events has been processed time.Sleep(100 * time.Millisecond) block, state = w.pending() - if balance := state.GetBalance(acc1Addr); balance.Cmp(big.NewInt(2000)) != 0 { - t.Errorf("account balance mismatch, has %d, want %d", balance, 2000) + if balance := state.GetBalance(testUserAddress); balance.Cmp(big.NewInt(2000)) != 0 { + t.Errorf("account balance mismatch: have %d, want %d", balance, 2000) } } @@ -165,7 +179,7 @@ func TestEmptyWorkClique(t *testing.T) { func testEmptyWork(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) { defer engine.Close() - w, _ := newTestWorker(t, chainConfig, engine) + w, _ := newTestWorker(t, chainConfig, engine, 0) defer w.close() var ( @@ -179,10 +193,10 @@ func testEmptyWork(t *testing.T, chainConfig *params.ChainConfig, engine consens receiptLen, balance = 1, big.NewInt(1000) } if len(task.receipts) != receiptLen { - t.Errorf("receipt number mismatch has %d, want %d", len(task.receipts), receiptLen) + t.Errorf("receipt number mismatch: have %d, want %d", len(task.receipts), receiptLen) } - if task.state.GetBalance(acc1Addr).Cmp(balance) != 0 { - t.Errorf("account balance mismatch has %d, want %d", task.state.GetBalance(acc1Addr), balance) + if task.state.GetBalance(testUserAddress).Cmp(balance) != 0 { + t.Errorf("account balance mismatch: have %d, want %d", task.state.GetBalance(testUserAddress), balance) } } @@ -219,19 +233,19 @@ func TestStreamUncleBlock(t *testing.T) { ethash := ethash.NewFaker() defer ethash.Close() - w, b := newTestWorker(t, ethashChainConfig, ethash) + w, b := newTestWorker(t, ethashChainConfig, ethash, 1) defer w.close() var taskCh = make(chan struct{}) taskIndex := 0 w.newTaskHook = func(task *task) { - if task.block.NumberU64() == 1 { + if task.block.NumberU64() == 2 { if taskIndex == 2 { - has := task.block.Header().UncleHash + have := task.block.Header().UncleHash want := types.CalcUncleHash([]*types.Header{b.uncleBlock.Header()}) - if has != want { - t.Errorf("uncle hash mismatch, has %s, want %s", has.Hex(), want.Hex()) + if have != want { + t.Errorf("uncle hash mismatch: have %s, want %s", have.Hex(), want.Hex()) } } taskCh <- struct{}{} @@ -248,12 +262,12 @@ func TestStreamUncleBlock(t *testing.T) { // Ensure worker has finished initialization for { b := w.pendingBlock() - if b != nil && b.NumberU64() == 1 { + if b != nil && b.NumberU64() == 2 { break } } - w.start() + // Ignore the first two works for i := 0; i < 2; i += 1 { select { @@ -282,7 +296,7 @@ func TestRegenerateMiningBlockClique(t *testing.T) { func testRegenerateMiningBlock(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) { defer engine.Close() - w, b := newTestWorker(t, chainConfig, engine) + w, b := newTestWorker(t, chainConfig, engine, 0) defer w.close() var taskCh = make(chan struct{}) @@ -293,10 +307,10 @@ func testRegenerateMiningBlock(t *testing.T, chainConfig *params.ChainConfig, en if taskIndex == 2 { receiptLen, balance := 2, big.NewInt(2000) if len(task.receipts) != receiptLen { - t.Errorf("receipt number mismatch has %d, want %d", len(task.receipts), receiptLen) + t.Errorf("receipt number mismatch: have %d, want %d", len(task.receipts), receiptLen) } - if task.state.GetBalance(acc1Addr).Cmp(balance) != 0 { - t.Errorf("account balance mismatch has %d, want %d", task.state.GetBalance(acc1Addr), balance) + if task.state.GetBalance(testUserAddress).Cmp(balance) != 0 { + t.Errorf("account balance mismatch: have %d, want %d", task.state.GetBalance(testUserAddress), balance) } } taskCh <- struct{}{} @@ -347,7 +361,7 @@ func TestAdjustIntervalClique(t *testing.T) { func testAdjustInterval(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine) { defer engine.Close() - w, _ := newTestWorker(t, chainConfig, engine) + w, _ := newTestWorker(t, chainConfig, engine, 0) defer w.close() w.skipSealHook = func(task *task) bool { @@ -387,10 +401,10 @@ func testAdjustInterval(t *testing.T, chainConfig *params.ChainConfig, engine co // Check interval if minInterval != wantMinInterval { - t.Errorf("resubmit min interval mismatch want %s has %s", wantMinInterval, minInterval) + t.Errorf("resubmit min interval mismatch: have %v, want %v ", minInterval, wantMinInterval) } if recommitInterval != wantRecommitInterval { - t.Errorf("resubmit interval mismatch want %s has %s", wantRecommitInterval, recommitInterval) + t.Errorf("resubmit interval mismatch: have %v, want %v", recommitInterval, wantRecommitInterval) } result = append(result, float64(recommitInterval.Nanoseconds())) index += 1