From bfc30b34374d71928acee5ff41282f09cedfd5e4 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 2 Jun 2015 15:17:36 -0400 Subject: [PATCH 1/4] Ignore unrequested blocks too far ahead of tip --- qa/rpc-tests/p2p-acceptblock.py | 65 ++++++++++++++++++++++++++++----- src/main.cpp | 9 ++++- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/p2p-acceptblock.py b/qa/rpc-tests/p2p-acceptblock.py index fcdd1e1b99..6431f430aa 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. @@ -126,13 +131,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) + time.sleep(0.5) 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 +152,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 + time.sleep(0.5) # Give time to process the block for x in self.nodes[0].getchaintips(): if x['hash'] == blocks_h2f[0].hash: assert_equal(x['status'], "headers-only") @@ -164,7 +171,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) + time.sleep(0.5) # 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 +189,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: + time.sleep(0.5) + 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). @@ -204,21 +250,20 @@ class AcceptBlockTest(BitcoinTestFramework): test_node.last_getdata = None test_node.send_message(msg_inv([CInv(2, blocks_h3[0].sha256)])) - time.sleep(1) + time.sleep(0.5) 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" + time.sleep(2) + 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 760cda5dee..e0997515b7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2841,9 +2841,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 - chainActive.Height()) > MIN_BLOCKS_TO_KEEP; // TODO: deal better with return value and error conditions for duplicate // and unrequested blocks. @@ -2851,6 +2857,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)) { From 6b1066fab41523d25d75b45a10e4b0f61d7c61d0 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 2 Jun 2015 20:46:41 -0400 Subject: [PATCH 2/4] Ignore whitelisting during IBD for unrequested blocks. --- src/main.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index e0997515b7..87750f9ea3 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4478,8 +4478,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(), From 04b5d235f1560b8285591b963d3636f7ddf74563 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 15 Jun 2015 15:30:05 -0400 Subject: [PATCH 3/4] Replace sleep with syncing using pings --- qa/rpc-tests/p2p-acceptblock.py | 34 ++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/qa/rpc-tests/p2p-acceptblock.py b/qa/rpc-tests/p2p-acceptblock.py index 6431f430aa..83c03eeb78 100755 --- a/qa/rpc-tests/p2p-acceptblock.py +++ b/qa/rpc-tests/p2p-acceptblock.py @@ -64,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 @@ -87,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", @@ -139,7 +159,7 @@ class AcceptBlockTest(BitcoinTestFramework): test_node.send_message(msg_block(blocks_h2[0])) white_node.send_message(msg_block(blocks_h2[1])) - time.sleep(0.5) + [ 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" @@ -152,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(0.5) # 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") @@ -171,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(0.5) + [ 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(): @@ -222,7 +242,7 @@ class AcceptBlockTest(BitcoinTestFramework): white_node.send_message(headers_message) # Send headers leading to tip white_node.send_message(msg_block(tips[1])) # Now deliver the tip try: - time.sleep(0.5) + white_node.sync_with_ping() self.nodes[1].getblock(tips[1].hash) print "Unrequested block far ahead of tip accepted from whitelisted peer" except: @@ -238,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" @@ -250,7 +270,7 @@ class AcceptBlockTest(BitcoinTestFramework): test_node.last_getdata = None test_node.send_message(msg_inv([CInv(2, blocks_h3[0].sha256)])) - time.sleep(0.5) + test_node.sync_with_ping() with mininode_lock: getdata = test_node.last_getdata @@ -261,7 +281,7 @@ class AcceptBlockTest(BitcoinTestFramework): # 7. Send the missing block for the third time (now it is requested) test_node.send_message(msg_block(blocks_h2f[0])) - time.sleep(2) + test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 290) print "Successfully reorged to longer chain from non-whitelisted peer" From 59b49cd07482494d9a99068af502be9ceda41ed2 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 11 Jul 2015 06:52:35 -0400 Subject: [PATCH 4/4] Eliminate signed/unsigned comparison warning --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 87750f9ea3..875b7b7068 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2849,7 +2849,7 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex, // 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 - chainActive.Height()) > MIN_BLOCKS_TO_KEEP; + 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.