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)
bitcoin · Nov 21, 2016 · 0c577f2638b7f701e4b4b96c4c8df39dc097cf2f · 0c577f2
2 parents 2108911 + 037159c
commit 0c577f2638b7f701e4b4b96c4c8df39dc097cf2f
Unified Split

Showing with 46 additions and 31 deletions.

+7 −0 qa/rpc-tests/p2p-compactblocks.py

+12 −2 qa/rpc-tests/p2p-segwit.py

+5 −6 qa/rpc-tests/sendheaders.py

+17 −2 qa/rpc-tests/test_framework/comptool.py

+5 −21 src/main.cpp
diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py
@@ -27,6 +27,7 @@ def __init__(self):
        self.last_cmpctblock = None
        self.block_announced = False
        self.last_getdata = None
+  
+        self.last_getheaders = None
        self.last_getblocktxn = None
        self.last_block = None
        self.last_blocktxn = None
@@ -64,6 +65,9 @@ def on_inv(self, conn, message):
    def on_getdata(self, conn, message):
        self.last_getdata = message
+  
+    def on_getheaders(self, conn, message):
+  
+        self.last_getheaders = message
+  
+
    def on_getblocktxn(self, conn, message):
        self.last_getblocktxn = message
@@ -396,6 +400,9 @@ def test_compactblock_requests(self, node, test_node, version, segwit):
            if announce == "inv":
                test_node.send_message(msg_inv([CInv(2, block.sha256)]))
+  
+                success = wait_until(lambda: test_node.last_getheaders is not None, timeout=30)
+  
+                assert(success)
+  
+                test_node.send_header_for_blocks([block])
            else:
                test_node.send_header_for_blocks([block])
            success = wait_until(lambda: test_node.last_getdata is not None, timeout=30)

diff --git a/qa/rpc-tests/p2p-segwit.py b/qa/rpc-tests/p2p-segwit.py
@@ -64,6 +64,9 @@ def on_getdata(self, conn, message):
            self.getdataset.add(inv.hash)
        self.last_getdata = message
+  
+    def on_getheaders(self, conn, message):
+  
+        self.last_getheaders = message
+  
+
    def on_pong(self, conn, message):
        self.last_pong = message
@@ -97,6 +100,10 @@ def wait_for_getdata(self, timeout=60):
        test_function = lambda: self.last_getdata != None
        self.sync(test_function, timeout)
+  
+    def wait_for_getheaders(self, timeout=60):
+  
+        test_function = lambda: self.last_getheaders != None
+  
+        self.sync(test_function, timeout)
+  
+
    def wait_for_inv(self, expected_inv, timeout=60):
        test_function = lambda: self.last_inv != expected_inv
        self.sync(test_function, timeout)
@@ -111,12 +118,15 @@ def announce_tx_and_wait_for_getdata(self, tx, timeout=60):
    def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
        with mininode_lock:
            self.last_getdata = None
+  
+            self.last_getheaders = None
+  
+        msg = msg_headers()
+  
+        msg.headers = [ CBlockHeader(block) ]
        if use_header:
-  
-            msg = msg_headers()
-  
-            msg.headers = [ CBlockHeader(block) ]
            self.send_message(msg)
        else:
            self.send_message(msg_inv(inv=[CInv(2, block.sha256)]))
+  
+            self.wait_for_getheaders()
+  
+            self.send_message(msg)
        self.wait_for_getdata()
        return

diff --git a/qa/rpc-tests/sendheaders.py b/qa/rpc-tests/sendheaders.py
@@ -348,14 +348,13 @@ def run_test(self):
                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)
-  
-                    # Should have received a getheaders as well!
-  
-                    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.send_block_inv(x.sha256) for x in blocks ]
+  
+                    test_node.wait_for_getdata([x.sha256 for x in blocks], timeout=5)
                    inv_node.sync_with_ping()
                else:
                    # Announce via headers

diff --git a/qa/rpc-tests/test_framework/comptool.py b/qa/rpc-tests/test_framework/comptool.py
@@ -111,6 +111,11 @@ def send_getheaders(self):
        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,15 +350,25 @@ def run(self):
                    # 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 ]
-  
-                        self.sync_blocks(block.sha256, 1)
+  
+                        # if we expect success, send inv and sync every block
+  
+                        # 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:
                        invqueue.append(CInv(2, block.sha256))
                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

diff --git a/src/main.cpp b/src/main.cpp
@@ -5370,28 +5370,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
            if (inv.type == MSG_BLOCK) {
                UpdateBlockAvailability(pfrom->GetId(), inv.hash);
                if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) {
-  
-                    // First request the headers preceding the announced block. In the normal fully-synced
-  
-                    // case where a new block is announced that succeeds the current tip (no reorganization),
-  
-                    // there are no such headers.
-  
-                    // Secondly, and only when we are close to being synced, we request the announced block directly,
-  
-                    // to avoid an extra round-trip. Note that we must *first* ask for the headers, so by the
-  
-                    // 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.
+  
+                    // We used to request the full block here, but since headers-announcements are now the
+  
+                    // primary method of announcement on the network, and since, in the case that a node
+  
+                    // fell back to inv we probably have a reorg which we should get the headers for first,
+  
+                    // we now only provide a getheaders response here. When we receive the headers, we will
+  
+                    // then ask for the blocks we need.
                    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 &&
-  
-                        (!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
-  
-                        inv.type |= nFetchFlags;
-  
-                        if (nodestate->fSupportsDesiredCmpctVersion)
-  
-                            vToFetch.push_back(CInv(MSG_CMPCT_BLOCK, inv.hash));
-  
-                        else
-  
-                            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);
                }
            }