From e8461666ecd7f0f89fba92a377c0f8beffa233e9 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@chaincode.com>
Date: Wed, 26 Oct 2016 14:53:18 -0400
Subject: [PATCH] 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.

Github-Pull: #9058
Rebased-From: dac53b58b555183ccc0d5e64c428528267cd98b3
Github-Pull: #9160
Rebased-From: ec34648766c4052816e4072cc61ad429430bcfd9
---
 qa/rpc-tests/p2p-compactblocks.py | 12 +++++++++---
 src/main.cpp                      | 14 +++++++++++++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py
index 0c73032c9..e0b72e684 100755
--- a/qa/rpc-tests/p2p-compactblocks.py
+++ b/qa/rpc-tests/p2p-compactblocks.py
@@ -592,8 +592,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
@@ -626,11 +626,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 db020a449..191bcff4c 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -5402,7 +5402,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 inv;
+            inv.type = State(pfrom->GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK;
+            inv.hash = req.blockhash;
+            pfrom->vRecvGetData.push_back(inv);
+            ProcessGetData(pfrom, chainparams.GetConsensus());
             return true;
         }
 
@@ -5734,7 +5746,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
                 if (!fAlreadyInFlight && mapBlocksInFlight.size() == 1 && pindex->pprev->IsValid(BLOCK_VALID_CHAIN)) {
                     // We seem to be rather well-synced, so it appears pfrom was the first to provide us
                     // with this block! Let's get them to announce using compact blocks in the future.
-                    MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman);
+                    MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom);
                 }
 
                 BlockTransactionsRequest req;