core: fix data races in unit tests (#609)

* core: fix data race in TestInsertChainWithSidecars We get this data race when running tests WARNING: DATA RACE Write at 0x00c0117bbf40 by goroutine 77211: github.com/ethereum/go-ethereum/core.TestInsertChainWithSidecars() /home/runner/work/ronin/ronin/core/blockchain_test.go:4215 +0x49fd testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 Previous write at 0x00c0117bbf40 by goroutine 82133: github.com/ethereum/go-ethereum/core.TestInsertChainWithSidecars.func6.1() /home/runner/work/ronin/ronin/core/blockchain_test.go:4179 +0x5a4 testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 The data race happens because 2 goroutines use the same variables. This commit makes them use their own variables. * core: fix data race in cacheConfig.TriesInMemory access We get this data race when running tests WARNING: DATA RACE Read at 0x000001faa090 by goroutine 23: github.com/ethereum/go-ethereum/core.NewBlockChain() /home/runner/work/ronin/ronin/core/blockchain.go:247 +0xc4 github.com/ethereum/go-ethereum/eth/protocols/eth.newTestBackendWithGenerator() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:74 +0x4c4 github.com/ethereum/go-ethereum/eth/protocols/eth.testGetBlockReceipts() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:569 +0x1f5 github.com/ethereum/go-ethereum/eth/protocols/eth.TestGetBlockReceipts66() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:528 +0x33 testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 Previous write at 0x000001faa090 by goroutine 19: github.com/ethereum/go-ethereum/core.NewBlockChain() /home/runner/work/ronin/ronin/core/blockchain.go:248 +0xe4 github.com/ethereum/go-ethereum/eth/protocols/eth.newTestBackendWithGenerator() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:74 +0x4c4 github.com/ethereum/go-ethereum/eth/protocols/eth.newTestBackend() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:61 +0x64 github.com/ethereum/go-ethereum/eth/protocols/eth.testGetBlockHeaders() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:126 +0x53 github.com/ethereum/go-ethereum/eth/protocols/eth.TestGetBlockHeaders66() /home/runner/work/ronin/ronin/eth/protocols/eth/handler_test.go:121 +0x33 testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 In the tests, the cacheConfig passed to NewBlockChain is nil so the defaultCacheConfig is used. However, the defaultCacheConfig.TriesInMemory is 0, so we fall into the path that set the cacheConfig.TriesInMemory to DefaultTriesInMemory which is 128. This set is made to the global defaultCacheConfig which causes data race with other accesses. This commit set the defaultCacheConfig.TriesInMemory to DefaultTriesInMemory. * core/vote: hold read lock while getting vote pool stats We get multiples data races while running vote pool tests WARNING: DATA RACE Read at 0x00c0002a3ce0 by goroutine 19: github.com/ethereum/go-ethereum/core/vote.(*VotePool).verifyStructureSizeOfVotePool() /home/runner/work/ronin/ronin/core/vote/vote_pool_test.go:77 +0xaa github.com/ethereum/go-ethereum/core/vote.testVotePool() /home/runner/work/ronin/ronin/core/vote/vote_pool_test.go:160 +0xe84 github.com/ethereum/go-ethereum/core/vote.TestValidVotePool() /home/runner/work/ronin/ronin/core/vote/vote_pool_test.go:85 +0x33 testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 Previous write at 0x00c0002a3ce0 by goroutine 23: runtime.mapassign() /opt/hostedtoolcache/go/1.20.10/x64/src/runtime/map.go:578 +0x0 github.com/ethereum/go-ethereum/core/vote.(*VotePool).putVote() /home/runner/work/ronin/ronin/core/vote/vote_pool.go:236 +0x346 github.com/ethereum/go-ethereum/core/vote.(*VotePool).putIntoVotePool() /home/runner/work/ronin/ronin/core/vote/vote_pool.go:212 +0x978 github.com/ethereum/go-ethereum/core/vote.(*VotePool).loop() /home/runner/work/ronin/ronin/core/vote/vote_pool.go:123 +0x504 github.com/ethereum/go-ethereum/core/vote.NewVotePool.func1() /home/runner/work/ronin/ronin/core/vote/vote_pool.go:98 +0x39 Currently, when getting vote pool's stats for checking in unit test we access these fields directly without holding lock. This commit creates some helper funtion to get these stats safely with read lock hold. * internal/cmdtest: increase the cli command test timeout The current timeout is 5s which is not enough when running tests with race detector. Increase that timeout to 15s. * p2p/discover: use synchronous version of addSeenNode We get this data race when running TestTable_BucketIPLimit Write at 0x00c00029c000 by goroutine 64: github.com/ethereum/go-ethereum/p2p/discover.(*Table).addSeenNodeSync() /home/runner/work/ronin/ronin/p2p/discover/table.go:570 +0x70a github.com/ethereum/go-ethereum/p2p/discover.(*Table).addSeenNode.func1() /home/runner/work/ronin/ronin/p2p/discover/table.go:527 +0x47 Previous read at 0x00c00029c000 by goroutine 57: github.com/ethereum/go-ethereum/p2p/discover.checkIPLimitInvariant() /home/runner/work/ronin/ronin/p2p/discover/table_test.go:187 +0x105 github.com/ethereum/go-ethereum/p2p/discover.TestTable_BucketIPLimit() /home/runner/work/ronin/ronin/p2p/discover/table_test.go:177 +0x2e4 testing.tRunner() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.10/x64/src/testing/testing.go:1629 +0x47 This commit changes TestTable_BucketIPLimit to use synchronous version of addSeenNode to avoid the data race.
axieinfinity · Oct 22, 2024 · 782bf20 · 782bf20
1 parent 2d172f2
commit 782bf20
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 31 deletions.
diff --git a/core/blockchain.go b/core/blockchain.go
@@ -151,6 +151,7 @@ var defaultCacheConfig = &CacheConfig{
 	TrieTimeLimit:  5 * time.Minute,
 	SnapshotLimit:  256,
 	SnapshotWait:   true,
+	TriesInMemory:  DefaultTriesInMemory,
 }
 
 // BlockChain represents the canonical chain given a database with a genesis

diff --git a/core/blockchain_test.go b/core/blockchain_test.go
@@ -4176,11 +4176,11 @@ func TestInsertChainWithSidecars(t *testing.T) {
 
 			// Wait for future block to be inserted
 			time.Sleep(15 * time.Second)
-			block = chain.CurrentBlock()
+			block := chain.CurrentBlock()
 			if block.Hash() != futureBlocks[0].Hash() {
 				t.Fatalf("Failed to insert future block, current: %d expected: %d", block.NumberU64(), futureBlocks[0].NumberU64())
 			}
-			savedSidecars = chain.GetBlobSidecarsByHash(block.Hash())
+			savedSidecars := chain.GetBlobSidecarsByHash(block.Hash())
 			if len(savedSidecars) != len(sidecars) {
 				t.Fatalf("Expect length of sidecar to be %d, got %d", len(sidecars), len(savedSidecars))
 			}

diff --git a/core/vote/vote_pool.go b/core/vote/vote_pool.go
@@ -432,6 +432,25 @@ func (pool *VotePool) basicVerify(vote *types.VoteEnvelope, headNumber uint64, m
 	return true
 }
 
+// stats returns the vote pool's
+// - number of current votes
+// - length of current vote queue
+// - number of future votes
+// - length of future vote queue
+func (pool *VotePool) stats() (int, int, int, int) {
+	pool.mu.RLock()
+	defer pool.mu.RUnlock()
+
+	return len(pool.curVotes), pool.curVotesPq.Len(), len(pool.futureVotes), pool.futureVotesPq.Len()
+}
+
+func (pool *VotePool) getNumberOfFutureVoteByPeer(peer string) uint64 {
+	pool.mu.RLock()
+	defer pool.mu.RUnlock()
+
+	return pool.numFutureVotePerPeer[peer]
+}
+
 func (pq votesPriorityQueue) Less(i, j int) bool {
 	return pq[i].TargetNumber < pq[j].TargetNumber
 }

diff --git a/core/vote/vote_pool_test.go b/core/vote/vote_pool_test.go
@@ -74,7 +74,8 @@ func (m *mockPOSA) IsFinalityVoterAt(chain consensus.ChainHeaderReader, header *
 func (pool *VotePool) verifyStructureSizeOfVotePool(curVotes, futureVotes, curVotesPq, futureVotesPq int) bool {
 	for i := 0; i < timeThreshold; i++ {
 		time.Sleep(1 * time.Second)
-		if len(pool.curVotes) == curVotes && len(pool.futureVotes) == futureVotes && pool.curVotesPq.Len() == curVotesPq && pool.futureVotesPq.Len() == futureVotesPq {
+		poolCurVotes, poolCurVotesPq, poolFutureVotes, poolFutureVotesPq := pool.stats()
+		if poolCurVotes == curVotes && poolFutureVotes == futureVotes && poolCurVotesPq == curVotesPq && poolFutureVotesPq == futureVotesPq {
 			return true
 		}
 	}
@@ -286,14 +287,15 @@ func testVotePool(t *testing.T, isValidRules bool) {
 		}
 	}
 
+	done := false
 	for i := 0; i < timeThreshold; i++ {
 		time.Sleep(1 * time.Second)
-		_, ok := votePool.curVotes[futureBlockHash]
-		if ok && len(votePool.curVotes[futureBlockHash].voteMessages) == 2 {
+		if len(votePool.FetchVoteByBlockHash(futureBlockHash)) == 2 {
+			done = true
 			break
 		}
 	}
-	if votePool.curVotes[futureBlockHash] == nil || len(votePool.curVotes[futureBlockHash].voteMessages) != 2 {
+	if !done {
 		t.Fatalf("transfer vote failed")
 	}
 
@@ -410,46 +412,55 @@ func TestVotePoolDosProtection(t *testing.T) {
 		time.Sleep(100 * time.Millisecond)
 	}
 
-	if len(*votePool.futureVotesPq) != maxFutureVotePerPeer {
-		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, len(*votePool.futureVotesPq))
+	_, _, _, futureVoteQueueLength := votePool.stats()
+	if futureVoteQueueLength != maxFutureVotePerPeer {
+		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, futureVoteQueueLength)
 	}
-	if votePool.numFutureVotePerPeer["AAAA"] != maxFutureVotePerPeer {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, votePool.numFutureVotePerPeer["AAAA"])
+	numFutureVotePerPeer := votePool.getNumberOfFutureVoteByPeer("AAAA")
+	if numFutureVotePerPeer != maxFutureVotePerPeer {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, numFutureVotePerPeer)
 	}
 
 	// This vote is dropped due to DOS protection
 	vote := generateVote(1, common.BigToHash(big.NewInt(int64(maxFutureVoteAmountPerBlock+1))), secretKey)
 	votePool.PutVote("AAAA", vote)
 	time.Sleep(100 * time.Millisecond)
-	if len(*votePool.futureVotesPq) != maxFutureVotePerPeer {
-		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, len(*votePool.futureVotesPq))
+	_, _, _, futureVoteQueueLength = votePool.stats()
+	if futureVoteQueueLength != maxFutureVotePerPeer {
+		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, futureVoteQueueLength)
 	}
-	if votePool.numFutureVotePerPeer["AAAA"] != maxFutureVotePerPeer {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, votePool.numFutureVotePerPeer["AAAA"])
+	numFutureVotePerPeer = votePool.getNumberOfFutureVoteByPeer("AAAA")
+	if numFutureVotePerPeer != maxFutureVotePerPeer {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, numFutureVotePerPeer)
 	}
 
 	// Vote from different peer must be accepted
 	vote = generateVote(1, common.BigToHash(big.NewInt(int64(maxFutureVoteAmountPerBlock+2))), secretKey)
 	votePool.PutVote("BBBB", vote)
 	time.Sleep(100 * time.Millisecond)
-	if len(*votePool.futureVotesPq) != maxFutureVotePerPeer+1 {
-		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, len(*votePool.futureVotesPq))
+	_, _, _, futureVoteQueueLength = votePool.stats()
+	if futureVoteQueueLength != maxFutureVotePerPeer+1 {
+		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, futureVoteQueueLength)
 	}
-	if votePool.numFutureVotePerPeer["AAAA"] != maxFutureVotePerPeer {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, votePool.numFutureVotePerPeer["AAAA"])
+	numFutureVotePerPeer = votePool.getNumberOfFutureVoteByPeer("AAAA")
+	if numFutureVotePerPeer != maxFutureVotePerPeer {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", maxFutureVotePerPeer, numFutureVotePerPeer)
 	}
-	if votePool.numFutureVotePerPeer["BBBB"] != 1 {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", 1, votePool.numFutureVotePerPeer["BBBB"])
+	numFutureVotePerPeer = votePool.getNumberOfFutureVoteByPeer("BBBB")
+	if numFutureVotePerPeer != 1 {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", 1, numFutureVotePerPeer)
 	}
 
 	// One vote is not queued twice
 	votePool.PutVote("CCCC", vote)
 	time.Sleep(100 * time.Millisecond)
-	if len(*votePool.futureVotesPq) != maxFutureVotePerPeer+1 {
-		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, len(*votePool.futureVotesPq))
+	_, _, _, futureVoteQueueLength = votePool.stats()
+	if futureVoteQueueLength != maxFutureVotePerPeer+1 {
+		t.Fatalf("Future vote pool length, expect %d have %d", maxFutureVotePerPeer, futureVoteQueueLength)
 	}
-	if votePool.numFutureVotePerPeer["CCCC"] != 0 {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", 0, votePool.numFutureVotePerPeer["CCCC"])
+	numFutureVotePerPeer = votePool.getNumberOfFutureVoteByPeer("CCCC")
+	if numFutureVotePerPeer != 0 {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", 0, numFutureVotePerPeer)
 	}
 
 	if _, err := chain.InsertChain(bs[1:], nil); err != nil {
@@ -458,11 +469,13 @@ func TestVotePoolDosProtection(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 	// Future vote must be transferred to current and failed the verification,
 	// numFutureVotePerPeer decreases
-	if len(*votePool.futureVotesPq) != 0 {
-		t.Fatalf("Future vote pool length, expect %d have %d", 0, len(*votePool.futureVotesPq))
+	_, _, _, futureVoteQueueLength = votePool.stats()
+	if futureVoteQueueLength != 0 {
+		t.Fatalf("Future vote pool length, expect %d have %d", 0, futureVoteQueueLength)
 	}
-	if votePool.numFutureVotePerPeer["AAAA"] != 0 {
-		t.Fatalf("Number of future vote per peer, expect %d have %d", 0, votePool.numFutureVotePerPeer["AAAA"])
+	numFutureVotePerPeer = votePool.getNumberOfFutureVoteByPeer("AAAA")
+	if numFutureVotePerPeer != 0 {
+		t.Fatalf("Number of future vote per peer, expect %d have %d", 0, numFutureVotePerPeer)
 	}
 }
 

diff --git a/internal/cmdtest/test_cmd.go b/internal/cmdtest/test_cmd.go
@@ -84,7 +84,7 @@ func (tt *TestCmd) Run(name string, args ...string) {
 // InputLine writes the given text to the child's stdin.
 // This method can also be called from an expect template, e.g.:
 //
-//     geth.expect(`Passphrase: {{.InputLine "password"}}`)
+//	geth.expect(`Passphrase: {{.InputLine "password"}}`)
 func (tt *TestCmd) InputLine(s string) string {
 	io.WriteString(tt.stdin, s+"\n")
 	return ""
@@ -238,7 +238,7 @@ func (tt *TestCmd) Kill() {
 }
 
 func (tt *TestCmd) withKillTimeout(fn func()) {
-	timeout := time.AfterFunc(5*time.Second, func() {
+	timeout := time.AfterFunc(15*time.Second, func() {
 		tt.Log("killing the child process (timeout)")
 		tt.Kill()
 	})

diff --git a/p2p/discover/table_test.go b/p2p/discover/table_test.go
@@ -169,7 +169,7 @@ func TestTable_BucketIPLimit(t *testing.T) {
 	d := 3
 	for i := 0; i < bucketIPLimit+1; i++ {
 		n := nodeAtDistance(tab.self().ID(), d, net.IP{172, 0, 1, byte(i)})
-		tab.addSeenNode(n)
+		tab.addSeenNodeSync(n)
 	}
 	if tab.len() > bucketIPLimit {
 		t.Errorf("too many nodes in table")