Merge b7ff6a611a into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

src/net_processing.cpp

@@ -1397,9 +1397,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
 
     // Bootstrap quickly by guessing a parent of our best tip is the forking point.
     // Guessing wrong in either direction is not a problem.
-    // Also reset pindexLastCommonBlock after a snapshot was loaded, so that blocks after the snapshot will be prioritised for download.
-    if (state->pindexLastCommonBlock == nullptr ||
-        (snap_base && state->pindexLastCommonBlock->nHeight < snap_base->nHeight)) {
+    if (state->pindexLastCommonBlock == nullptr) {
         state->pindexLastCommonBlock = m_chainman.ActiveChain()[std::min(state->pindexBestKnownBlock->nHeight, m_chainman.ActiveChain().Height())];
     }
 
@@ -1409,6 +1407,12 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
     if (state->pindexLastCommonBlock == state->pindexBestKnownBlock)
         return;
 
+    // If our tip has advanced beyond pindexLastCommonBlock, move it ahead to the tip. We don't need to download any blocks in between, and skipping ahead here
+    // allows us to determine nWindowEnd better.
+    if (m_chainman.ActiveHeight() > state->pindexLastCommonBlock->nHeight && state->pindexBestKnownBlock->GetAncestor(m_chainman.ActiveHeight()) == m_chainman.ActiveTip()) {
+        state->pindexLastCommonBlock = m_chainman.ActiveTip();
+    }
+
     const CBlockIndex *pindexWalk = state->pindexLastCommonBlock;
     // Never fetch further than the best block we know the peer has, or more than BLOCK_DOWNLOAD_WINDOW + 1 beyond the last
     // linked block we have in common with this peer. The +1 is so we can detect stalling, namely if we would be able to

test/functional/p2p_ibd_stalling.py

@@ -29,15 +29,15 @@ from test_framework.util import (
 
 
 class P2PStaller(P2PDataStore):
-    def __init__(self, stall_block):
-        self.stall_block = stall_block
+    def __init__(self, stall_blocks):
+        self.stall_blocks = stall_blocks
         super().__init__()
 
     def on_getdata(self, message):
         for inv in message.inv:
             self.getdata_requests.append(inv.hash)
             if (inv.type & MSG_TYPE_MASK) == MSG_BLOCK:
-                if (inv.hash != self.stall_block):
+                if (inv.hash not in self.stall_blocks):
                     self.send_without_ping(msg_block(self.block_store[inv.hash]))
 
     def on_getheaders(self, message):
@@ -51,7 +51,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
 
     def run_test(self):
         NUM_BLOCKS = 1025
-        NUM_PEERS = 4
+        NUM_PEERS = 5
         node = self.nodes[0]
         tip = int(node.getbestblockhash(), 16)
         blocks = []
@@ -66,8 +66,10 @@ class P2PIBDStallingTest(BitcoinTestFramework):
             block_time += 1
             height += 1
             block_dict[blocks[-1].sha256] = blocks[-1]
-        stall_block = blocks[0].sha256
 
+        stall_index = 0
+        second_stall_index = 500
+        stall_blocks = [blocks[stall_index].sha256, blocks[second_stall_index].sha256]
         headers_message = msg_headers()
         headers_message.headers = [CBlockHeader(b) for b in blocks[:NUM_BLOCKS-1]]
         peers = []
@@ -76,14 +78,12 @@ class P2PIBDStallingTest(BitcoinTestFramework):
         self.mocktime = int(time.time()) + 1
         node.setmocktime(self.mocktime)
         for id in range(NUM_PEERS):
-            peers.append(node.add_outbound_p2p_connection(P2PStaller(stall_block), p2p_idx=id, connection_type="outbound-full-relay"))
+            peers.append(node.add_outbound_p2p_connection(P2PStaller(stall_blocks), p2p_idx=id, connection_type="outbound-full-relay"))
             peers[-1].block_store = block_dict
             peers[-1].send_and_ping(headers_message)
 
-        # Need to wait until 1023 blocks are received - the magic total bytes number is a workaround in lack of an rpc
-        # returning the number of downloaded (but not connected) blocks.
-        bytes_recv = 172761 if not self.options.v2transport else 169692
-        self.wait_until(lambda: self.total_bytes_recv_for_blocks() == bytes_recv)
+        # Wait until all blocks are received (except for the stall blocks), so that no other blocks are in flight.
+        self.wait_until(lambda: sum(len(peer['inflight']) for peer in node.getpeerinfo()) == len(stall_blocks))
 
         self.all_sync_send_with_ping(peers)
         # If there was a peer marked for stalling, it would get disconnected
@@ -104,7 +104,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
         node.setmocktime(self.mocktime)
         peers[0].wait_for_disconnect()
         assert_equal(node.num_test_p2p_connections(), NUM_PEERS - 1)
-        self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+        self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
         # Make sure that SendMessages() is invoked, which assigns the missing block
         # to another peer and starts the stalling logic for them
         self.all_sync_send_with_ping(peers)
@@ -119,7 +119,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
         self.mocktime += 2
         node.setmocktime(self.mocktime)
         self.wait_until(lambda: sum(x.is_connected for x in node.p2ps) == NUM_PEERS - 2)
-        self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+        self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
         self.all_sync_send_with_ping(peers)
 
         self.log.info("Check that the stalling timeout gets doubled to 8 seconds for the next staller")
@@ -132,24 +132,19 @@ class P2PIBDStallingTest(BitcoinTestFramework):
         self.mocktime += 2
         node.setmocktime(self.mocktime)
         self.wait_until(lambda: sum(x.is_connected for x in node.p2ps) == NUM_PEERS - 3)
-        self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+        self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
         self.all_sync_send_with_ping(peers)
 
-        self.log.info("Provide the withheld block and check that stalling timeout gets reduced back to 2 seconds")
-        with node.assert_debug_log(expected_msgs=['Decreased stalling timeout to 2 seconds']):
+        self.log.info("Provide the first withheld block and check that stalling timeout gets reduced back to 2 seconds")
+        with node.assert_debug_log(expected_msgs=['Decreased stalling timeout to 2 seconds'], unexpected_msgs=['Stall started']):
             for p in peers:
-                if p.is_connected and (stall_block in p.getdata_requests):
-                    p.send_without_ping(msg_block(block_dict[stall_block]))
+                if p.is_connected and (stall_blocks[0] in p.getdata_requests):
+                    p.send_without_ping(msg_block(block_dict[stall_blocks[0]]))
+            self.all_sync_send_with_ping(peers)
 
-        self.log.info("Check that all outstanding blocks get connected")
-        self.wait_until(lambda: node.getblockcount() == NUM_BLOCKS)
+        self.log.info("Check that all outstanding blocks up to the second stall block get connected")
+        self.wait_until(lambda: node.getblockcount() == second_stall_index)
 
-    def total_bytes_recv_for_blocks(self):
-        total = 0
-        for info in self.nodes[0].getpeerinfo():
-            if ("block" in info["bytesrecv_per_msg"].keys()):
-                total += info["bytesrecv_per_msg"]["block"]
-        return total
 
     def all_sync_send_with_ping(self, peers):
         for p in peers:

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)