cmd/devp2p: fix flakey tests in CI (#22757)

This PR fixes a couple of issues in the eth test suite that caused flakiness when run in the CI.

cmd/devp2p/internal/ethtest/eth66_suite.go

@@ -217,17 +217,7 @@ func (s *Suite) TestLargeAnnounce_66(t *utesting.T) {
 		sendConn.Close()
 	}
 	// Test the last block as a valid block
-	sendConn, receiveConn := s.setupConnection66(t), s.setupConnection66(t)
+	s.sendNextBlock66(t)
-	defer sendConn.Close()
-	defer receiveConn.Close()
-
-	s.testAnnounce66(t, sendConn, receiveConn, blocks[3])
-	// update test suite chain
-	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
-	// wait for client to update its chain
-	if err := receiveConn.waitForBlock66(s.fullChain.blocks[nextBlock]); err != nil {
-		t.Fatal(err)
-	}
 }
 
 func (s *Suite) TestOldAnnounce_66(t *utesting.T) {

cmd/devp2p/internal/ethtest/eth66_suiteHelpers.go

@@ -229,8 +229,11 @@ func (s *Suite) waitAnnounce66(t *utesting.T, conn *Conn, blockAnnouncement *New
 func (c *Conn) waitForBlock66(block *types.Block) error {
 	defer c.SetReadDeadline(time.Time{})
 
-	timeout := time.Now().Add(20 * time.Second)
+	c.SetReadDeadline(time.Now().Add(20 * time.Second))
-	c.SetReadDeadline(timeout)
+	// note: if the node has not yet imported the block, it will respond
+	// to the GetBlockHeaders request with an empty BlockHeaders response,
+	// so the GetBlockHeaders request must be sent again until the BlockHeaders
+	// response contains the desired header.
 	for {
 		req := eth.GetBlockHeadersPacket66{
 			RequestId: 54,
@@ -253,8 +256,10 @@ func (c *Conn) waitForBlock66(block *types.Block) error {
 			if reqID != req.RequestId {
 				return fmt.Errorf("request ID mismatch: wanted %d, got %d", req.RequestId, reqID)
 			}
-			if len(msg) > 0 {
+			for _, header := range msg {
-				return nil
+				if header.Number.Uint64() == block.NumberU64() {
+					return nil
+				}
 			}
 			time.Sleep(100 * time.Millisecond)
 		case *NewPooledTransactionHashes:
@@ -319,10 +324,10 @@ func (s *Suite) sendNextBlock66(t *utesting.T) {
 	}
 	// send announcement and wait for node to request the header
 	s.testAnnounce66(t, sendConn, receiveConn, blockAnnouncement)
-	// update test suite chain
-	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
 	// wait for client to update its chain
-	if err := receiveConn.waitForBlock66(s.chain.Head()); err != nil {
+	if err := receiveConn.waitForBlock66(s.fullChain.blocks[nextBlock]); err != nil {
 		t.Fatal(err)
 	}
+	// update test suite chain
+	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
 }

cmd/devp2p/internal/ethtest/suite.go

@@ -260,22 +260,28 @@ func (s *Suite) TestGetBlockBodies(t *utesting.T) {
 // TestBroadcast tests whether a block announcement is correctly
 // propagated to the given node's peer(s).
 func (s *Suite) TestBroadcast(t *utesting.T) {
+	s.sendNextBlock(t)
+}
+
+func (s *Suite) sendNextBlock(t *utesting.T) {
 	sendConn, receiveConn := s.setupConnection(t), s.setupConnection(t)
 	defer sendConn.Close()
 	defer receiveConn.Close()
 
+	// create new block announcement
 	nextBlock := len(s.chain.blocks)
 	blockAnnouncement := &NewBlock{
 		Block: s.fullChain.blocks[nextBlock],
 		TD:    s.fullChain.TD(nextBlock + 1),
 	}
+	// send announcement and wait for node to request the header
 	s.testAnnounce(t, sendConn, receiveConn, blockAnnouncement)
-	// update test suite chain
-	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
 	// wait for client to update its chain
-	if err := receiveConn.waitForBlock(s.chain.Head()); err != nil {
+	if err := receiveConn.waitForBlock(s.fullChain.blocks[nextBlock]); err != nil {
 		t.Fatal(err)
 	}
+	// update test suite chain
+	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
 }
 
 // TestMaliciousHandshake tries to send malicious data during the handshake.
@@ -394,18 +400,7 @@ func (s *Suite) TestLargeAnnounce(t *utesting.T) {
 		sendConn.Close()
 	}
 	// Test the last block as a valid block
-	sendConn := s.setupConnection(t)
+	s.sendNextBlock(t)
-	receiveConn := s.setupConnection(t)
-	defer sendConn.Close()
-	defer receiveConn.Close()
-
-	s.testAnnounce(t, sendConn, receiveConn, blocks[3])
-	// update test suite chain
-	s.chain.blocks = append(s.chain.blocks, s.fullChain.blocks[nextBlock])
-	// wait for client to update its chain
-	if err := receiveConn.waitForBlock(s.fullChain.blocks[nextBlock]); err != nil {
-		t.Fatal(err)
-	}
 }
 
 func (s *Suite) TestOldAnnounce(t *utesting.T) {

cmd/devp2p/internal/ethtest/types.go

@@ -334,8 +334,11 @@ loop:
 func (c *Conn) waitForBlock(block *types.Block) error {
 	defer c.SetReadDeadline(time.Time{})
 
-	timeout := time.Now().Add(20 * time.Second)
+	c.SetReadDeadline(time.Now().Add(20 * time.Second))
-	c.SetReadDeadline(timeout)
+	// note: if the node has not yet imported the block, it will respond
+	// to the GetBlockHeaders request with an empty BlockHeaders response,
+	// so the GetBlockHeaders request must be sent again until the BlockHeaders
+	// response contains the desired header.
 	for {
 		req := &GetBlockHeaders{Origin: eth.HashOrNumber{Hash: block.Hash()}, Amount: 1}
 		if err := c.Write(req); err != nil {
@@ -343,8 +346,10 @@ func (c *Conn) waitForBlock(block *types.Block) error {
 		}
 		switch msg := c.Read().(type) {
 		case *BlockHeaders:
-			if len(*msg) > 0 {
+			for _, header := range *msg {
-				return nil
+				if header.Number.Uint64() == block.NumberU64() {
+					return nil
+				}
 			}
 			time.Sleep(100 * time.Millisecond)
 		default: