From 047ceac142246b5d51056a51dbf4645b31802be4 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 13:59:37 -0700 Subject: [PATCH 1/4] [net processing] ignore tx GETDATA from blocks-only peers Co-Authored-By: John Newbery --- src/net_processing.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 26327ac6ebd..7561e269e37 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1595,15 +1595,13 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm std::vector 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() - 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() - 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); while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) { @@ -1613,8 +1611,12 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm 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; From e257cf71c851e25e1a533bf1d4296f6b55c81332 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 14:00:21 -0700 Subject: [PATCH 2/4] [net processing] ignore unknown INV types in GETDATA messages Co-Authored-By: John Newbery --- src/net_processing.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7561e269e37..533966a07c2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1645,18 +1645,14 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } // release cs_main 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()) { From 2f032556e08a04807c71eb02104ca9589eaadf1b Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 14:52:10 -0700 Subject: [PATCH 3/4] [test] test that an invalid GETDATA doesn't prevent processing of future messages Co-Authored-By: John Newbery --- test/functional/p2p_getdata.py | 51 ++++++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 52 insertions(+) create mode 100755 test/functional/p2p_getdata.py diff --git a/test/functional/p2p_getdata.py b/test/functional/p2p_getdata.py new file mode 100755 index 00000000000..fd94a09d802 --- /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 b8523e16b79..c60bc3761d2 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -151,6 +151,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', From 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 28 Apr 2020 20:28:51 -0400 Subject: [PATCH 4/4] [docs] Improve commenting in ProcessGetData() --- src/net_processing.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 533966a07c2..fbd2ccfd424 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1604,10 +1604,14 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm { 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; @@ -1644,6 +1648,8 @@ 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++; if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) {