Merge #8872: Remove block-request logic from INV message processing

037159c Remove block-request logic from INV message processing (Matt Corallo)
3451203 [qa] Respond to getheaders and do not assume a getdata on inv (Matt Corallo)
d768f15 [qa] Make comptool push blocks instead of relying on inv-fetch (mrbandrews)

qa/rpc-tests/sendheaders.py

@@ -364,14 +364,13 @@ class SendHeadersTest(BitcoinTestFramework):
                 if j == 0:
                     # Announce via inv
                     test_node.send_block_inv(tip)
-                    test_node.wait_for_getdata([tip], timeout=5)
+                    test_node.wait_for_getheaders(timeout=5)
+                    # Should have received a getheaders now
+                    test_node.send_header_for_blocks(blocks)
                     # Test that duplicate inv's won't result in duplicate
                     # getdata requests, or duplicate headers announcements
-                    inv_node.send_block_inv(tip)
+                    [ inv_node.send_block_inv(x.sha256) for x in blocks ]
-                    # Should have received a getheaders as well!
+                    test_node.wait_for_getdata([x.sha256 for x in blocks], timeout=5)
-                    test_node.send_header_for_blocks(blocks)
-                    test_node.wait_for_getdata([x.sha256 for x in blocks[0:-1]], timeout=5)
-                    [ inv_node.send_block_inv(x.sha256) for x in blocks[0:-1] ]
                     inv_node.sync_with_ping()
                 else:
                     # Announce via headers

qa/rpc-tests/test_framework/comptool.py

@@ -111,6 +111,11 @@ class TestNode(NodeConnCB):
         m.locator = self.block_store.get_locator(self.bestblockhash)
         self.conn.send_message(m)
 
+    def send_header(self, header):
+        m = msg_headers()
+        m.headers.append(header)
+        self.conn.send_message(m)
+
     # This assumes BIP31
     def send_ping(self, nonce):
         self.pingMap[nonce] = True
@@ -345,8 +350,16 @@ class TestManager(object):
                     # Either send inv's to each node and sync, or add
                     # to invqueue for later inv'ing.
                     if (test_instance.sync_every_block):
-                        [ c.cb.send_inv(block) for c in self.connections ]
+                        # if we expect success, send inv and sync every block
-                        self.sync_blocks(block.sha256, 1)
+                        # if we expect failure, just push the block and see what happens.
+                        if outcome == True:
+                            [ c.cb.send_inv(block) for c in self.connections ]
+                            self.sync_blocks(block.sha256, 1)
+                        else:
+                            [ c.send_message(msg_block(block)) for c in self.connections ]
+                            [ c.cb.send_ping(self.ping_counter) for c in self.connections ]
+                            self.wait_for_pings(self.ping_counter)
+                            self.ping_counter += 1
                         if (not self.check_results(tip, outcome)):
                             raise AssertionError("Test failed at test %d" % test_number)
                     else:
@@ -354,6 +367,8 @@ class TestManager(object):
                 elif isinstance(b_or_t, CBlockHeader):
                     block_header = b_or_t
                     self.block_store.add_header(block_header)
+                    [ c.cb.send_header(block_header) for c in self.connections ]
+
                 else:  # Tx test runner
                     assert(isinstance(b_or_t, CTransaction))
                     tx = b_or_t

src/net_processing.cpp

@@ -1430,23 +1430,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
                         LogPrint("net", "delaying getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id);
                         pfrom->PushBlockHashFromINV(inv.hash);
                     } else {
-                        // First request the headers preceding the announced block. In the normal fully-synced
+                        // We used to request the full block here, but since headers-announcements are now the
-                        // case where a new block is announced that succeeds the current tip (no reorganization),
+                        // primary method of announcement on the network, and since, in the case that a node
-                        // there are no such headers.
+                        // fell back to inv we probably have a reorg which we should get the headers for first,
-                        // Secondly, and only when we are close to being synced, we request the announced block directly,
+                        // we now only provide a getheaders response here. When we receive the headers, we will
-                        // to avoid an extra round-trip. Note that we must *first* ask for the headers, so by the
+                        // then ask for the blocks we need.
-                        // time the block arrives, the header chain leading up to it is already validated. Not
-                        // doing this will result in the received block being rejected as an orphan in case it is
-                        // not a direct successor.
                         connman.PushMessage(pfrom, NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), inv.hash);
-                        CNodeState *nodestate = State(pfrom->GetId());
-                        if (CanDirectFetch(chainparams.GetConsensus()) &&
-                            nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
-                            vToFetch.push_back(inv);
-                            // Mark block as in flight already, even though the actual "getdata" message only goes out
-                            // later (within the same cs_main lock, though).
-                            MarkBlockAsInFlight(pfrom->GetId(), inv.hash, chainparams.GetConsensus());
-                        }
                         LogPrint("net", "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id);
                     }
                 }