Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core, ethclient/gethclient: improve flaky tests #25918

Merged
merged 11 commits into from
Oct 6, 2022
10 changes: 10 additions & 0 deletions consensus/clique/snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,17 @@ func TestClique(t *testing.T) {
failure: errRecentlySigned,
},
}
var toStop *core.BlockChain
defer func() {
if toStop != nil {
toStop.Stop()
}
}()
Copy link
Contributor Author

@holiman holiman Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugly. Use deferred stops instead, or put the test in a method so the defer happen sooner

// Run through the scenarios and test them
for i, tt := range tests {
if toStop != nil {
toStop.Stop()
}
// Create the account pool and generate the initial set of signers
accounts := newTesterAccountPool()

Expand Down Expand Up @@ -454,6 +463,7 @@ func TestClique(t *testing.T) {
t.Errorf("test %d: failed to create test chain: %v", i, err)
continue
}
toStop = chain
failed := false
for j := 0; j < len(batches)-1; j++ {
if k, err := chain.InsertChain(batches[j]); err != nil {
Expand Down
15 changes: 12 additions & 3 deletions core/blockchain.go
Original file line number Diff line number Diff line change
Expand Up @@ -856,9 +856,13 @@ func (bc *BlockChain) writeHeadBlock(block *types.Block) {
headBlockGauge.Update(int64(block.NumberU64()))
}

// Stop stops the blockchain service. If any imports are currently in progress
// it will abort them using the procInterrupt.
func (bc *BlockChain) Stop() {
// stop stops the blockchain service. If any imports are currently in progress
// it will abort them using the procInterrupt. This method stops all running
// goroutines, but does not do all the post-stop work of persisting data.
// OBS! It is generally recommended to use the Stop method!
// This method has been exposed to allow tests to stop the blockchain while simulating
// a crash.
func (bc *BlockChain) stop() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename it to stopWithoutPersistence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah maybe.. Another alternative - how about stopActivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or stopThreads ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe stopWithoutSaving

if !atomic.CompareAndSwapInt32(&bc.running, 0, 1) {
return
}
Expand All @@ -878,7 +882,12 @@ func (bc *BlockChain) Stop() {
// returned.
bc.chainmu.Close()
bc.wg.Wait()
}

// Stop stops the blockchain service. If any imports are currently in progress
// it will abort them using the procInterrupt.
func (bc *BlockChain) Stop() {
bc.stop()
// Ensure that the entirety of the state snapshot is journalled to disk.
var snapBase common.Hash
if bc.snaps != nil {
Expand Down
2 changes: 2 additions & 0 deletions core/blockchain_repair_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1826,6 +1826,7 @@ func testRepair(t *testing.T, tt *rewindTest, snapshots bool) {
}
// Pull the plug on the database, simulating a hard crash
db.Close()
chain.stop()

// Start a new blockchain back up and see where the repair leads us
db, err = rawdb.NewLevelDBDatabaseWithFreezer(datadir, 0, 0, datadir, "", false)
Expand Down Expand Up @@ -1940,6 +1941,7 @@ func TestIssue23496(t *testing.T) {

// Pull the plug on the database, simulating a hard crash
db.Close()
chain.stop()

// Start a new blockchain back up and see where the repair leads us
db, err = rawdb.NewLevelDBDatabaseWithFreezer(datadir, 0, 0, datadir, "", false)
Expand Down
1 change: 1 addition & 0 deletions core/blockchain_sethead_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,7 @@ func testSetHead(t *testing.T, tt *rewindTest, snapshots bool) {
if err != nil {
t.Fatalf("Failed to create chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
// If sidechain blocks are needed, make a light chain and import it
var sideblocks types.Blocks
if tt.sidechainBlocks > 0 {
Expand Down
5 changes: 4 additions & 1 deletion core/blockchain_snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ func (snaptest *crashSnapshotTest) test(t *testing.T) {
// Pull the plug on the database, simulating a hard crash
db := chain.db
db.Close()
chain.stop()

// Start a new blockchain back up and see where the repair leads us
newdb, err := rawdb.NewLevelDBDatabaseWithFreezer(snaptest.datadir, 0, 0, snaptest.datadir, "", false)
Expand Down Expand Up @@ -388,15 +389,17 @@ func (snaptest *wipeCrashSnapshotTest) test(t *testing.T) {
SnapshotLimit: 256,
SnapshotWait: false, // Don't wait rebuild
}
_, err = NewBlockChain(snaptest.db, config, snaptest.gspec, nil, snaptest.engine, vm.Config{}, nil, nil)
tmp, err := NewBlockChain(snaptest.db, config, snaptest.gspec, nil, snaptest.engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("Failed to recreate chain: %v", err)
}
tmp.Stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original intention here is to simulate an unclean shutdown without proper stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, then I should change it to stop (only kills the threads, doesn't clean up)

holiman marked this conversation as resolved.
Show resolved Hide resolved
// Simulate the blockchain crash.
newchain, err = NewBlockChain(snaptest.db, nil, snaptest.gspec, nil, snaptest.engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("Failed to recreate chain: %v", err)
}
defer newchain.Stop()
snaptest.verify(t, newchain, blocks)
}

Expand Down
31 changes: 27 additions & 4 deletions core/blockchain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ func TestHeadersInsertNonceError(t *testing.T) { testInsertNonceError(t, false)
func TestBlocksInsertNonceError(t *testing.T) { testInsertNonceError(t, true) }

func testInsertNonceError(t *testing.T, full bool) {
for i := 1; i < 25 && !t.Failed(); i++ {
doTest := func(i int) {
// Create a pristine chain and database
genDb, _, blockchain, err := newCanonical(ethash.NewFaker(), 0, full)
if err != nil {
Expand Down Expand Up @@ -730,6 +730,9 @@ func testInsertNonceError(t *testing.T, full bool) {
}
}
}
for i := 1; i < 25 && !t.Failed(); i++ {
doTest(i)
}
}

// Tests that fast importing a block chain produces the same chain data as the
Expand Down Expand Up @@ -1639,6 +1642,7 @@ func TestBlockchainHeaderchainReorgConsistency(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
for i := 0; i < len(blocks); i++ {
if _, err := chain.InsertChain(blocks[i : i+1]); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", i, err)
Expand Down Expand Up @@ -1681,6 +1685,7 @@ func TestTrieForkGC(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
for i := 0; i < len(blocks); i++ {
if _, err := chain.InsertChain(blocks[i : i+1]); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", i, err)
Expand Down Expand Up @@ -1717,6 +1722,7 @@ func TestLargeReorgTrieGC(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if _, err := chain.InsertChain(shared); err != nil {
t.Fatalf("failed to insert shared chain: %v", err)
}
Expand Down Expand Up @@ -1896,6 +1902,7 @@ func TestLowDiffLongChain(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -1955,6 +1962,7 @@ func testSideImport(t *testing.T, numCanonBlocksInSidechain, blocksBetweenCommon
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
// Activate the transition since genesis if required
if mergePoint == 0 {
merger.ReachTTD()
Expand Down Expand Up @@ -2092,7 +2100,7 @@ func testInsertKnownChainData(t *testing.T, typ string) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}

defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
var (
inserter func(blocks []*types.Block, receipts []types.Receipts) error
asserter func(t *testing.T, block *types.Block)
Expand Down Expand Up @@ -2242,6 +2250,7 @@ func testInsertKnownChainDataWithMerging(t *testing.T, typ string, mergeHeight i
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
var (
inserter func(blocks []*types.Block, receipts []types.Receipts) error
asserter func(t *testing.T, block *types.Block)
Expand Down Expand Up @@ -2394,6 +2403,7 @@ func TestReorgToShorterRemovesCanonMapping(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(canonblocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -2430,6 +2440,7 @@ func TestReorgToShorterRemovesCanonMappingHeaderChain(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
// Convert into headers
canonHeaders := make([]*types.Header, len(canonblocks))
for i, block := range canonblocks {
Expand Down Expand Up @@ -2629,6 +2640,7 @@ func TestSkipStaleTxIndicesInSnapSync(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
headers := make([]*types.Header, len(blocks))
for i, block := range blocks {
headers[i] = block.Header()
Expand Down Expand Up @@ -2769,6 +2781,7 @@ func TestSideImportPrunedBlocks(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -2857,6 +2870,7 @@ func TestDeleteCreateRevert(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -2966,6 +2980,7 @@ func TestDeleteRecreateSlots(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -3042,6 +3057,7 @@ func TestDeleteRecreateAccount(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -3163,7 +3179,7 @@ func TestDeleteRecreateSlotsAcrossManyBlocks(t *testing.T) {
e.exist = false
e.values = nil
}
t.Logf("block %d; adding destruct\n", e.blocknum)
//t.Logf("block %d; adding destruct\n", e.blocknum)
return tx
}
var newResurrect = func(e *expectation, b *BlockGen) *types.Transaction {
Expand All @@ -3174,7 +3190,7 @@ func TestDeleteRecreateSlotsAcrossManyBlocks(t *testing.T) {
e.exist = true
e.values = map[int]int{3: e.blocknum + 1, 4: 4}
}
t.Logf("block %d; adding resurrect\n", e.blocknum)
//t.Logf("block %d; adding resurrect\n", e.blocknum)
return tx
}

Expand Down Expand Up @@ -3211,6 +3227,7 @@ func TestDeleteRecreateSlotsAcrossManyBlocks(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
var asHash = func(num int) common.Hash {
return common.BytesToHash([]byte{byte(num)})
}
Expand Down Expand Up @@ -3340,6 +3357,7 @@ func TestInitThenFailCreateContract(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
statedb, _ := chain.State()
if got, exp := statedb.GetBalance(aa), big.NewInt(100000); got.Cmp(exp) != 0 {
t.Fatalf("Genesis err, got %v exp %v", got, exp)
Expand Down Expand Up @@ -3420,6 +3438,7 @@ func TestEIP2718Transition(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -3506,6 +3525,7 @@ func TestEIP1559Transition(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(blocks); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -3608,6 +3628,7 @@ func TestSetCanonical(t *testing.T) {
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
defer chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if n, err := chain.InsertChain(canon); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
Expand Down Expand Up @@ -3758,6 +3779,7 @@ func TestCanonicalHashMarker(t *testing.T) {
}
}
}
chain.Stop()
}
}

Expand Down Expand Up @@ -3961,6 +3983,7 @@ func TestTxIndexer(t *testing.T) {
chain.indexBlocks(rawdb.ReadTxIndexTail(db), 128, make(chan struct{}))
verify(db, 0)

chain.Stop()
db.Close()
os.RemoveAll(frdir)
}
Expand Down
4 changes: 2 additions & 2 deletions core/dao_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ func TestDAOForkRangeExtradata(t *testing.T) {
for i := int64(0); i < params.DAOForkExtraRange.Int64(); i++ {
// Create a pro-fork block, and try to feed into the no-fork chain
bc, _ := NewBlockChain(rawdb.NewMemoryDatabase(), nil, congspec, nil, ethash.NewFaker(), vm.Config{}, nil, nil)
defer bc.Stop()

blocks := conBc.GetBlocksFromHash(conBc.CurrentBlock().Hash(), int(conBc.CurrentBlock().NumberU64()))
for j := 0; j < len(blocks)/2; j++ {
Expand All @@ -87,6 +86,7 @@ func TestDAOForkRangeExtradata(t *testing.T) {
if err := bc.stateCache.TrieDB().Commit(bc.CurrentHeader().Root, true, nil); err != nil {
t.Fatalf("failed to commit contra-fork head for expansion: %v", err)
}
bc.Stop()
blocks, _ = GenerateChain(&proConf, conBc.CurrentBlock(), ethash.NewFaker(), genDb, 1, func(i int, gen *BlockGen) {})
if _, err := conBc.InsertChain(blocks); err == nil {
t.Fatalf("contra-fork chain accepted pro-fork block: %v", blocks[0])
Expand All @@ -98,7 +98,6 @@ func TestDAOForkRangeExtradata(t *testing.T) {
}
// Create a no-fork block, and try to feed into the pro-fork chain
bc, _ = NewBlockChain(rawdb.NewMemoryDatabase(), nil, progspec, nil, ethash.NewFaker(), vm.Config{}, nil, nil)
defer bc.Stop()

blocks = proBc.GetBlocksFromHash(proBc.CurrentBlock().Hash(), int(proBc.CurrentBlock().NumberU64()))
for j := 0; j < len(blocks)/2; j++ {
Expand All @@ -110,6 +109,7 @@ func TestDAOForkRangeExtradata(t *testing.T) {
if err := bc.stateCache.TrieDB().Commit(bc.CurrentHeader().Root, true, nil); err != nil {
t.Fatalf("failed to commit pro-fork head for expansion: %v", err)
}
bc.Stop()
blocks, _ = GenerateChain(&conConf, proBc.CurrentBlock(), ethash.NewFaker(), genDb, 1, func(i int, gen *BlockGen) {})
if _, err := proBc.InsertChain(blocks); err == nil {
t.Fatalf("pro-fork chain accepted contra-fork block: %v", blocks[0])
Expand Down
20 changes: 14 additions & 6 deletions eth/catalyst/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,20 @@ func TestEth2AssembleBlockWithAnotherBlocksTxs(t *testing.T) {
blockParams := beacon.PayloadAttributesV1{
Timestamp: blocks[8].Time() + 5,
}
execData, err := assembleBlock(api, blocks[8].Hash(), &blockParams)
if err != nil {
t.Fatalf("error producing block, err=%v", err)
}
if len(execData.Transactions) != blocks[9].Transactions().Len() {
t.Fatalf("invalid number of transactions %d != 1", len(execData.Transactions))
// This test is a bit time-sensitive, the miner needs to pick up on the
// txs in the pool. Therefore, we retry once if it fails on the first attempt.
var testErr error
for retries := 2; retries > 0; retries-- {
if execData, err := assembleBlock(api, blocks[8].Hash(), &blockParams); err != nil {
t.Fatalf("error producing block, err=%v", err)
} else if have, want := len(execData.Transactions), blocks[9].Transactions().Len(); have != want {
testErr = fmt.Errorf("invalid number of transactions, have %d want %d", have, want)
} else {
testErr = nil
holiman marked this conversation as resolved.
Show resolved Hide resolved
}
}
if testErr != nil {
t.Fatal(testErr)
}
}

Expand Down
2 changes: 1 addition & 1 deletion eth/gasprice/feehistory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestFeeHistory(t *testing.T) {
oracle := NewOracle(backend, config)

first, reward, baseFee, ratio, err := oracle.FeeHistory(context.Background(), c.count, c.last, c.percent)

backend.chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
expReward := c.expCount
if len(c.percent) == 0 {
expReward = 0
Expand Down
1 change: 1 addition & 0 deletions eth/gasprice/gasprice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ func TestSuggestTipCap(t *testing.T) {

// The gas price sampled is: 32G, 31G, 30G, 29G, 28G, 27G
got, err := oracle.SuggestTipCap(context.Background())
backend.chain.Stop()
holiman marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Fatalf("Failed to retrieve recommended gas price: %v", err)
}
Expand Down
Loading