From 47e9659ecfbe07077a4564591095bd5510e0f917 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 24 Oct 2016 15:33:14 -0400 Subject: [PATCH 1/3] [qa] Fix bug in compactblocks v2 merge Bug caused the wait_for_block_announcement to be called on the wrong node, leading to nondeterminism and occasional test failures. Bug was introduced in merge commit: d075479 Merge #8882: [qa] Fix race conditions in p2p-compactblocks.py and sendheaders.py Underlying commits which conflicted were: 27acfc1 [qa] Update p2p-compactblocks.py for compactblocks v2 6976db2 [qa] Another attempt to fix race condition in p2p-compactblocks.py The first commit changed the test_compactblock_construction function signature and second commit added code which wasn't updated during the merge to use the new arguments. Suhas Daftuar noticed the bug and suggested the fix. --- qa/rpc-tests/p2p-compactblocks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 6b5d47713..8a5f589d0 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -300,8 +300,8 @@ class CompactBlocksTest(BitcoinTestFramework): assert(segwit_tx_generated) # check that our test is not broken # Wait until we've seen the block announcement for the resulting tip - tip = int(self.nodes[0].getbestblockhash(), 16) - assert(self.test_node.wait_for_block_announcement(tip)) + tip = int(node.getbestblockhash(), 16) + assert(test_node.wait_for_block_announcement(tip)) # Now mine a block, and look at the resulting compact block. test_node.clear_block_announcement() From 55bfddcabbf9e8a3743f77167ba4a43aaba9f948 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 26 Oct 2016 14:15:16 -0400 Subject: [PATCH 2/3] [qa] Fix stale data bug in test_compactblocks_not_at_tip Clear test_node.last_block before requesting blocks in the compactblocks_not_at_tip test so comparisons won't fail if a blocks were received before the test started. The bug doesn't currently cause any problems due to the order tests run, but this will change in the next commit. --- qa/rpc-tests/p2p-compactblocks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 8a5f589d0..4b49dad03 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -648,6 +648,8 @@ class CompactBlocksTest(BitcoinTestFramework): node.generate(1) wait_until(test_node.received_block_announcement, timeout=30) test_node.clear_block_announcement() + with mininode_lock: + test_node.last_block = None test_node.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))])) success = wait_until(lambda: test_node.last_block is not None, timeout=30) assert(success) From dac53b58b555183ccc0d5e64c428528267cd98b3 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 26 Oct 2016 14:53:18 -0400 Subject: [PATCH 3/3] Modify getblocktxn handler not to drop requests for old blocks The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in https://github.com/bitcoin/bitcoin/issues/8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at https://github.com/bitcoin/bips/pull/469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. --- qa/rpc-tests/p2p-compactblocks.py | 12 +++++++++--- src/main.cpp | 12 ++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 4b49dad03..ecd1e4216 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -589,8 +589,8 @@ class CompactBlocksTest(BitcoinTestFramework): assert_equal(int(node.getbestblockhash(), 16), block.sha256) def test_getblocktxn_handler(self, node, test_node, version): - # bitcoind won't respond for blocks whose height is more than 15 blocks - # deep. + # bitcoind will not send blocktxn responses for blocks whose height is + # more than 10 blocks deep. MAX_GETBLOCKTXN_DEPTH = 10 chain_height = node.getblockcount() current_height = chain_height @@ -623,11 +623,17 @@ class CompactBlocksTest(BitcoinTestFramework): test_node.last_blocktxn = None current_height -= 1 - # Next request should be ignored, as we're past the allowed depth. + # Next request should send a full block response, as we're past the + # allowed depth for a blocktxn response. block_hash = node.getblockhash(current_height) msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [0]) + with mininode_lock: + test_node.last_block = None + test_node.last_blocktxn = None test_node.send_and_ping(msg) with mininode_lock: + test_node.last_block.block.calc_sha256() + assert_equal(test_node.last_block.block.sha256, int(block_hash, 16)) assert_equal(test_node.last_blocktxn, None) def test_compactblocks_not_at_tip(self, node, test_node): diff --git a/src/main.cpp b/src/main.cpp index 5e17ec625..55e3d934e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5450,7 +5450,19 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } if (it->second->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) { + // If an older block is requested (should never happen in practice, + // but can happen in tests) send a block response instead of a + // blocktxn response. Sending a full block response instead of a + // small blocktxn response is preferable in the case where a peer + // might maliciously send lots of getblocktxn requests to trigger + // expensive disk reads, because it will require the peer to + // actually receive all the data read from disk over the network. LogPrint("net", "Peer %d sent us a getblocktxn for a block > %i deep", pfrom->id, MAX_BLOCKTXN_DEPTH); + CInv vInv; + vInv.type = State(pfrom->GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK; + vInv.hash = req.blockhash; + pfrom->vRecvGetData.push_back(vInv); + ProcessGetData(pfrom, chainparams.GetConsensus(), connman); return true; }