Merge #18808: [net processing] Drop unknown types in getdata

9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e08a04807c71eb02104ca9589eaadf1b [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c851e25e1a533bf1d4296f6b55c81332 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142246b5d51056a51dbf4645b31802be4 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)

Pull request description:

  Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.

  Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.

  There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.

ACKs for top commit:
  sipa:
    utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
  brakmic:
    ACK https://github.com/bitcoin/bitcoin/commit/9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
  luke-jr:
    utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
  naumenkogs:
    utACK 9847e20
  ajtowns:
    utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7

Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c

3 files changed, 74 insertions, 18 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a4d4953281..7907ee70c7 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1612,26 +1612,32 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
     std::vector<CInv> vNotFound;
     const CNetMsgMaker msgMaker(pfrom->GetSendVersion());
 
-    // Note that if we receive a getdata for a MSG_TX or MSG_WITNESS_TX from a
-    // block-relay-only outbound peer, we will stop processing further getdata
-    // messages from this peer (likely resulting in our peer eventually
-    // disconnecting us).
-    if (pfrom->m_tx_relay != nullptr) {
-        // mempool entries added before this time have likely expired from mapRelay
-        const std::chrono::seconds longlived_mempool_time = GetTime<std::chrono::seconds>() - RELAY_TX_CACHE_TIME;
-        const std::chrono::seconds mempool_req = pfrom->m_tx_relay->m_last_mempool_req.load();
+    // mempool entries added before this time have likely expired from mapRelay
+    const std::chrono::seconds longlived_mempool_time = GetTime<std::chrono::seconds>() - RELAY_TX_CACHE_TIME;
+    // Get last mempool request time
+    const std::chrono::seconds mempool_req = pfrom->m_tx_relay != nullptr ? pfrom->m_tx_relay->m_last_mempool_req.load()
+                                                                          : std::chrono::seconds::min();
 
+    {
         LOCK(cs_main);
 
+        // Process as many TX items from the front of the getdata queue as
+        // possible, since they're common and it's efficient to batch process
+        // them.
         while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) {
             if (interruptMsgProc)
                 return;
-            // Don't bother if send buffer is too full to respond anyway
+            // The send buffer provides backpressure. If there's no space in
+            // the buffer, pause processing until the next call.
             if (pfrom->fPauseSend)
                 break;
 
-            const CInv &inv = *it;
-            it++;
+            const CInv &inv = *it++;
+
+            if (pfrom->m_tx_relay == nullptr) {
+                // Ignore GETDATA requests for transactions from blocks-only peers.
+                continue;
+            }
 
             // Send stream from relay memory
             bool push = false;
@@ -1665,19 +1671,17 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
         }
     } // release cs_main
 
+    // Only process one BLOCK item per call, since they're uncommon and can be
+    // expensive to process.
     if (it != pfrom->vRecvGetData.end() && !pfrom->fPauseSend) {
-        const CInv &inv = *it;
+        const CInv &inv = *it++;
         if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) {
-            it++;
             ProcessGetBlockData(pfrom, chainparams, inv, connman);
         }
+        // else: If the first item on the queue is an unknown type, we erase it
+        // and continue processing the queue on the next call.
     }
 
-    // Unknown types in the GetData stay in vRecvGetData and block any future
-    // message from this peer, see vRecvGetData check in ProcessMessages().
-    // Depending on future p2p changes, we might either drop unknown getdata on
-    // the floor or disconnect the peer.
-
     pfrom->vRecvGetData.erase(pfrom->vRecvGetData.begin(), it);
 
     if (!vNotFound.empty()) {
diff --git a/test/functional/p2p_getdata.py b/test/functional/p2p_getdata.py
new file mode 100755
index 0000000000..fd94a09d80
--- /dev/null
+++ b/test/functional/p2p_getdata.py
@@ -0,0 +1,51 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test GETDATA processing behavior"""
+from collections import defaultdict
+
+from test_framework.messages import (
+    CInv,
+    msg_getdata,
+)
+from test_framework.mininode import (
+    mininode_lock,
+    P2PInterface,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import wait_until
+
+class P2PStoreBlock(P2PInterface):
+
+    def __init__(self):
+        super().__init__()
+        self.blocks = defaultdict(int)
+
+    def on_block(self, message):
+        message.block.calc_sha256()
+        self.blocks[message.block.sha256] += 1
+
+class GetdataTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.nodes[0].add_p2p_connection(P2PStoreBlock())
+
+        self.log.info("test that an invalid GETDATA doesn't prevent processing of future messages")
+
+        # Send invalid message and verify that node responds to later ping
+        invalid_getdata = msg_getdata()
+        invalid_getdata.inv.append(CInv(t=0, h=0))  # INV type 0 is invalid.
+        self.nodes[0].p2ps[0].send_and_ping(invalid_getdata)
+
+        # Check getdata still works by fetching tip block
+        best_block = int(self.nodes[0].getbestblockhash(), 16)
+        good_getdata = msg_getdata()
+        good_getdata.inv.append(CInv(t=2, h=best_block))
+        self.nodes[0].p2ps[0].send_and_ping(good_getdata)
+        wait_until(lambda: self.nodes[0].p2ps[0].blocks[best_block] == 1, timeout=30, lock=mininode_lock)
+
+if __name__ == '__main__':
+    GetdataTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 45f591ba4a..55c8c1d033 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -157,6 +157,7 @@ BASE_SCRIPTS = [
     'rpc_deprecated.py',
     'wallet_disable.py',
     'p2p_addr_relay.py',
+    'p2p_getdata.py',
     'rpc_net.py',
     'wallet_keypool.py',
     'wallet_keypool.py --descriptors',