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)

Author: laanwj

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

@@ -2734,16 +2734,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)) {
@@ -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(),