Merge pull request #6224

59b49cd Eliminate signed/unsigned comparison warning (Suhas Daftuar)
04b5d23 Replace sleep with syncing using pings (Suhas Daftuar)
6b1066f Ignore whitelisting during IBD for unrequested blocks. (Suhas Daftuar)
bfc30b3 Ignore unrequested blocks too far ahead of tip (Suhas Daftuar)

2 files changed, 90 insertions, 14 deletions

diff --git a/qa/rpc-tests/p2p-acceptblock.py b/qa/rpc-tests/p2p-acceptblock.py
index fcdd1e1b99..83c03eeb78 100755
--- a/qa/rpc-tests/p2p-acceptblock.py
+++ b/qa/rpc-tests/p2p-acceptblock.py
@@ -40,6 +40,11 @@ The test:
    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 @@ class TestNode(NodeConnCB):
         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 @@ class TestNode(NodeConnCB):
     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 @@ class AcceptBlockTest(BitcoinTestFramework):
         # 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 @@ class AcceptBlockTest(BitcoinTestFramework):
         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 @@ class AcceptBlockTest(BitcoinTestFramework):
         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 @@ class AcceptBlockTest(BitcoinTestFramework):
         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 @@ class AcceptBlockTest(BitcoinTestFramework):
         # 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 @@ class AcceptBlockTest(BitcoinTestFramework):
             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 ]
 
diff --git a/src/main.cpp b/src/main.cpp
index fefeabeb64..d470ba9003 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -2734,9 +2734,15 @@ 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.
@@ -2744,6 +2750,7 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
     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)) {
@@ -4368,8 +4375,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(),