Be even stricter in processing unrequested blocks

Github-Pull: #6224
Rebased-From: bfc30b3 6b1066f 04b5d23 59b49cd

Author: sdaftuar

qa/rpc-tests/p2p-acceptblock.py

@@ -40,6 +40,11 @@
    it's missing an intermediate block.
    Node1 should reorg to this longer chain.
 
+4b.Send 288 more blocks on the longer chain.
+   Node0 should process all but the last block (too far ahead in height).
+   Send all headers to Node1, and then send the last block in that chain.
+   Node1 should accept the block because it's coming from a whitelisted peer.
+
 5. Send a duplicate of the block in #3 to Node0.
    Node0 should not process the block because it is unrequested, and stay on
    the shorter chain.
@@ -59,6 +64,8 @@ def __init__(self):
         NodeConnCB.__init__(self)
         self.create_callback_map()
         self.connection = None
+        self.ping_counter = 1
+        self.last_pong = msg_pong()
 
     def add_connection(self, conn):
         self.connection = conn
@@ -82,6 +89,24 @@ def wait_for_verack(self):
     def send_message(self, message):
         self.connection.send_message(message)
 
+    def on_pong(self, conn, message):
+        self.last_pong = message
+
+    # Sync up with the node after delivery of a block
+    def sync_with_ping(self, timeout=30):
+        self.connection.send_message(msg_ping(nonce=self.ping_counter))
+        received_pong = False
+        sleep_time = 0.05
+        while not received_pong and timeout > 0:
+            time.sleep(sleep_time)
+            timeout -= sleep_time
+            with mininode_lock:
+                if self.last_pong.nonce == self.ping_counter:
+                    received_pong = True
+        self.ping_counter += 1
+        return received_pong
+
+
 class AcceptBlockTest(BitcoinTestFramework):
     def add_options(self, parser):
         parser.add_option("--testbinary", dest="testbinary",
@@ -126,13 +151,15 @@ def run_test(self):
         # 2. Send one block that builds on each tip.
         # This should be accepted.
         blocks_h2 = []  # the height 2 blocks on each node's chain
+        block_time = time.time() + 1
         for i in xrange(2):
-            blocks_h2.append(create_block(tips[i], create_coinbase(), time.time()+1))
+            blocks_h2.append(create_block(tips[i], create_coinbase(), block_time))
             blocks_h2[i].solve()
+            block_time += 1
         test_node.send_message(msg_block(blocks_h2[0]))
         white_node.send_message(msg_block(blocks_h2[1]))
 
-        time.sleep(1)
+        [ x.sync_with_ping() for x in [test_node, white_node] ]
         assert_equal(self.nodes[0].getblockcount(), 2)
         assert_equal(self.nodes[1].getblockcount(), 2)
         print "First height 2 block accepted by both nodes"
@@ -145,7 +172,7 @@ def run_test(self):
         test_node.send_message(msg_block(blocks_h2f[0]))
         white_node.send_message(msg_block(blocks_h2f[1]))
 
-        time.sleep(1)  # Give time to process the block
+        [ x.sync_with_ping() for x in [test_node, white_node] ]
         for x in self.nodes[0].getchaintips():
             if x['hash'] == blocks_h2f[0].hash:
                 assert_equal(x['status'], "headers-only")
@@ -164,7 +191,7 @@ def run_test(self):
         test_node.send_message(msg_block(blocks_h3[0]))
         white_node.send_message(msg_block(blocks_h3[1]))
 
-        time.sleep(1)
+        [ x.sync_with_ping() for x in [test_node, white_node] ]
         # Since the earlier block was not processed by node0, the new block
         # can't be fully validated.
         for x in self.nodes[0].getchaintips():
@@ -182,6 +209,45 @@ def run_test(self):
         assert_equal(self.nodes[1].getblockcount(), 3)
         print "Successfully reorged to length 3 chain from whitelisted peer"
 
+        # 4b. Now mine 288 more blocks and deliver; all should be processed but
+        # the last (height-too-high) on node0.  Node1 should process the tip if
+        # we give it the headers chain leading to the tip.
+        tips = blocks_h3
+        headers_message = msg_headers()
+        all_blocks = []   # node0's blocks
+        for j in xrange(2):
+            for i in xrange(288):
+                next_block = create_block(tips[j].sha256, create_coinbase(), tips[j].nTime+1)
+                next_block.solve()
+                if j==0:
+                    test_node.send_message(msg_block(next_block))
+                    all_blocks.append(next_block)
+                else:
+                    headers_message.headers.append(CBlockHeader(next_block))
+                tips[j] = next_block
+
+        time.sleep(2)
+        for x in all_blocks:
+            try:
+                self.nodes[0].getblock(x.hash)
+                if x == all_blocks[287]:
+                    raise AssertionError("Unrequested block too far-ahead should have been ignored")
+            except:
+                if x == all_blocks[287]:
+                    print "Unrequested block too far-ahead not processed"
+                else:
+                    raise AssertionError("Unrequested block with more work should have been accepted")
+
+        headers_message.headers.pop() # Ensure the last block is unrequested
+        white_node.send_message(headers_message) # Send headers leading to tip
+        white_node.send_message(msg_block(tips[1]))  # Now deliver the tip
+        try:
+            white_node.sync_with_ping()
+            self.nodes[1].getblock(tips[1].hash)
+            print "Unrequested block far ahead of tip accepted from whitelisted peer"
+        except:
+            raise AssertionError("Unrequested block from whitelisted peer not accepted")
+
         # 5. Test handling of unrequested block on the node that didn't process
         # Should still not be processed (even though it has a child that has more
         # work).
@@ -192,7 +258,7 @@ def run_test(self):
         # the node processes it and incorrectly advances the tip).
         # But this would be caught later on, when we verify that an inv triggers
         # a getdata request for this block.
-        time.sleep(1)
+        test_node.sync_with_ping()
         assert_equal(self.nodes[0].getblockcount(), 2)
         print "Unrequested block that would complete more-work chain was ignored"
 
@@ -204,21 +270,20 @@ def run_test(self):
             test_node.last_getdata = None
             test_node.send_message(msg_inv([CInv(2, blocks_h3[0].sha256)]))
 
-        time.sleep(1)
+        test_node.sync_with_ping()
         with mininode_lock:
             getdata = test_node.last_getdata
 
-        # Check that the getdata is for the right block
-        assert_equal(len(getdata.inv), 1)
+        # Check that the getdata includes the right block
         assert_equal(getdata.inv[0].hash, blocks_h2f[0].sha256)
         print "Inv at tip triggered getdata for unprocessed block"
 
         # 7. Send the missing block for the third time (now it is requested)
         test_node.send_message(msg_block(blocks_h2f[0]))
 
-        time.sleep(1)
-        assert_equal(self.nodes[0].getblockcount(), 3)
-        print "Successfully reorged to length 3 chain from non-whitelisted peer"
+        test_node.sync_with_ping()
+        assert_equal(self.nodes[0].getblockcount(), 290)
+        print "Successfully reorged to longer chain from non-whitelisted peer"
 
         [ c.disconnect_node() for c in connections ]
 

src/main.cpp

@@ -2847,16 +2847,23 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
 
     // Try to process all requested blocks that we don't have, but only
     // process an unrequested block if it's new and has enough work to
-    // advance our tip.
+    // advance our tip, and isn't too many blocks ahead.
     bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;
     bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true);
+    // Blocks that are too out-of-order needlessly limit the effectiveness of
+    // pruning, because pruning will not delete block files that contain any
+    // blocks which are too close in height to the tip.  Apply this test
+    // regardless of whether pruning is enabled; it should generally be safe to
+    // not process unrequested blocks.
+    bool fTooFarAhead = (pindex->nHeight > int(chainActive.Height() + MIN_BLOCKS_TO_KEEP));
 
     // TODO: deal better with return value and error conditions for duplicate
     // and unrequested blocks.
     if (fAlreadyHave) return true;
     if (!fRequested) {  // If we didn't ask for it:
         if (pindex->nTx != 0) return true;  // This is a previously-processed block that was pruned
         if (!fHasMoreWork) return true;     // Don't process less-work chains
+        if (fTooFarAhead) return true;      // Block height is too high
     }
 
     if ((!CheckBlock(block, state)) || !ContextualCheckBlock(block, state, pindex->pprev)) {
@@ -4478,8 +4485,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
         pfrom->AddInventoryKnown(inv);
 
         CValidationState state;
-        // Process all blocks from whitelisted peers, even if not requested.
-        ProcessNewBlock(state, pfrom, &block, pfrom->fWhitelisted, NULL);
+        // Process all blocks from whitelisted peers, even if not requested,
+        // unless we're still syncing with the network.
+        // Such an unrequested block may still be processed, subject to the
+        // conditions in AcceptBlock().
+        bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload();
+        ProcessNewBlock(state, pfrom, &block, forceProcessing, NULL);
         int nDoS;
         if (state.IsInvalid(nDoS)) {
             pfrom->PushMessage("reject", strCommand, state.GetRejectCode(),