From ce238dd79bd3a1190a2229fac4502fa5742fc6e3 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 12 Mar 2024 13:18:15 -0400 Subject: [PATCH 1/5] net_processing: do not treat non-connecting headers as response Since https://github.com/bitcoin/bitcoin/pull/25454 we keep track of the last GETHEADERS request that was sent and wasn't responded to. So far, every incoming HEADERS message is treated as a response to whatever GETHEADERS was last sent, regardless of its contents. This commit makes this tracking more accurate, by only treating HEADERS messages which (1) are empty, (2) connect to our existing block header tree, or (3) are a continuation of a low-work headers sync as responses that clear the "outstanding GETHEADERS" state (m_last_getheaders_timestamp). That means that HEADERS messages which do not satisfy any of the above criteria will be ignored, not triggering a GETHEADERS, and potentially (for now, but see later commit) increase misbehavior score. --- src/net_processing.cpp | 14 ++++++++++---- test/functional/p2p_sendheaders.py | 7 ++++++- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 653daf8ff9712..a6a19752d9170 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2628,6 +2628,8 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro { if (peer.m_headers_sync) { auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == MAX_HEADERS_RESULTS); + // If it is a valid continuation, we should treat the existing getheaders request as responded to. + if (result.success) peer.m_last_getheaders_timestamp = {}; if (result.request_more) { auto locator = peer.m_headers_sync->NextHeadersRequestLocator(); // If we were instructed to ask for a locator, it should not be empty. @@ -2925,6 +2927,9 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, LOCK(m_headers_presync_mutex); m_headers_presync_stats.erase(pfrom.GetId()); } + // A headers message with no headers cannot be an announcement, so assume + // it is a response to our last getheaders request, if there is one. + peer.m_last_getheaders_timestamp = {}; return; } @@ -2989,6 +2994,11 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, return; } + // If headers connect, assume that this is in response to any outstanding getheaders + // request we may have sent, and clear out the time of our last request. Non-connecting + // headers cannot be a response to a getheaders request. + peer.m_last_getheaders_timestamp = {}; + // If the headers we received are already in memory and an ancestor of // m_best_header or our tip, skip anti-DoS checks. These headers will not // use any more memory (and we are not leaking information that could be @@ -4691,10 +4701,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } - // Assume that this is in response to any outstanding getheaders - // request we may have sent, and clear out the time of our last request - peer->m_last_getheaders_timestamp = {}; - std::vector headers; // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. diff --git a/test/functional/p2p_sendheaders.py b/test/functional/p2p_sendheaders.py index 508d6fe403170..1f27af977b5da 100755 --- a/test/functional/p2p_sendheaders.py +++ b/test/functional/p2p_sendheaders.py @@ -555,9 +555,13 @@ def test_nonnull_locators(self, test_node, inv_node): height += 1 for i in range(1, MAX_NUM_UNCONNECTING_HEADERS_MSGS): - # Send a header that doesn't connect, check that we get a getheaders. with p2p_lock: test_node.last_message.pop("getheaders", None) + # Send an empty header as failed response to the receive getheaders, to + # make marked responded to. New headers are not treated as announcements + # otherwise. + test_node.send_header_for_blocks([]) + # Send the actual unconnecting header, which should trigger a new getheaders. test_node.send_header_for_blocks([blocks[i]]) test_node.wait_for_getheaders() @@ -573,6 +577,7 @@ def test_nonnull_locators(self, test_node, inv_node): # Send a header that doesn't connect, check that we get a getheaders. with p2p_lock: test_node.last_message.pop("getheaders", None) + test_node.send_header_for_blocks([]) test_node.send_header_for_blocks([blocks[i % len(blocks)]]) test_node.wait_for_getheaders() From 78515e185755f5eba951917b8e2314ec69446d7d Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 7 Mar 2024 11:22:09 -0500 Subject: [PATCH 2/5] net_processing: drop Misbehavior for unconnecting headers This misbehavior was originally intended to prevent bandwidth wastage due to actually observed very broken (but likely non-malicious) nodes that respond to GETHEADERS with a response unrelated to the request, triggering a request cycle. This has however largely been addressed by the previous commit, which causes non-connecting HEADERS that are received while a GETHEADERS has not been responded to, to be ignored, as long as they do not time out (2 minutes). With that, the specific misbehavior is largely irrelevant (for inbound peers, it is now harmless; for outbound peers, the eviction logic will eventually kick them out if they're not keeping up with headers at all). --- src/net_processing.cpp | 29 ++-------------------- test/functional/p2p_sendheaders.py | 40 +++++------------------------- 2 files changed, 8 insertions(+), 61 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a6a19752d9170..6fc1b841dd257 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -129,8 +129,6 @@ static constexpr double BLOCK_DOWNLOAD_TIMEOUT_BASE = 1; static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.5; /** Maximum number of headers to announce when relaying blocks with headers message.*/ static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8; -/** Maximum number of unconnecting headers announcements before DoS score */ -static const int MAX_NUM_UNCONNECTING_HEADERS_MSGS = 10; /** Minimum blocks required to signal NODE_NETWORK_LIMITED */ static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; /** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */ @@ -381,9 +379,6 @@ struct Peer { /** Whether we've sent our peer a sendheaders message. **/ std::atomic m_sent_sendheaders{false}; - /** Length of current-streak of unconnecting headers announcements */ - int m_num_unconnecting_headers_msgs GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0}; - /** When to potentially disconnect peer for stalling headers download */ std::chrono::microseconds m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0us}; @@ -2579,37 +2574,24 @@ arith_uint256 PeerManagerImpl::GetAntiDoSWorkThreshold() * announcement. * * We'll send a getheaders message in response to try to connect the chain. - * - * The peer can send up to MAX_NUM_UNCONNECTING_HEADERS_MSGS in a row that - * don't connect before given DoS points. - * - * Once a headers message is received that is valid and does connect, - * m_num_unconnecting_headers_msgs gets reset back to 0. */ void PeerManagerImpl::HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector& headers) { - peer.m_num_unconnecting_headers_msgs++; // Try to fill in the missing headers. const CBlockIndex* best_header{WITH_LOCK(cs_main, return m_chainman.m_best_header)}; if (MaybeSendGetHeaders(pfrom, GetLocator(best_header), peer)) { - LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d, m_num_unconnecting_headers_msgs=%d)\n", + LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d)\n", headers[0].GetHash().ToString(), headers[0].hashPrevBlock.ToString(), best_header->nHeight, - pfrom.GetId(), peer.m_num_unconnecting_headers_msgs); + pfrom.GetId()); } // Set hashLastUnknownBlock for this peer, so that if we // eventually get the headers - even from a different peer - // we can use this peer to download. WITH_LOCK(cs_main, UpdateBlockAvailability(pfrom.GetId(), headers.back().GetHash())); - - // The peer may just be broken, so periodically assign DoS points if this - // condition persists. - if (peer.m_num_unconnecting_headers_msgs % MAX_NUM_UNCONNECTING_HEADERS_MSGS == 0) { - Misbehaving(peer, 20, strprintf("%d non-connecting headers", peer.m_num_unconnecting_headers_msgs)); - } } bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector& headers) const @@ -2857,11 +2839,6 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer, const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers) { - if (peer.m_num_unconnecting_headers_msgs > 0) { - LogPrint(BCLog::NET, "peer=%d: resetting m_num_unconnecting_headers_msgs (%d -> 0)\n", pfrom.GetId(), peer.m_num_unconnecting_headers_msgs); - } - peer.m_num_unconnecting_headers_msgs = 0; - LOCK(cs_main); CNodeState *nodestate = State(pfrom.GetId()); @@ -2988,8 +2965,6 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, // special logic for handling headers that don't connect, as this // could be benign. HandleFewUnconnectingHeaders(pfrom, peer, headers); - } else { - Misbehaving(peer, 10, "invalid header received"); } return; } diff --git a/test/functional/p2p_sendheaders.py b/test/functional/p2p_sendheaders.py index 1f27af977b5da..f0c773b8a9c19 100755 --- a/test/functional/p2p_sendheaders.py +++ b/test/functional/p2p_sendheaders.py @@ -71,19 +71,13 @@ Expect: no response. Part 5: Test handling of headers that don't connect. -a. Repeat 10 times: +a. Repeat 100 times: 1. Announce a header that doesn't connect. Expect: getheaders message 2. Send headers chain. Expect: getdata for the missing blocks, tip update. -b. Then send 9 more headers that don't connect. +b. Then send 100 more headers that don't connect. Expect: getheaders message each time. -c. Announce a header that does connect. - Expect: no response. -d. Announce 49 headers that don't connect. - Expect: getheaders message each time. -e. Announce one more that doesn't connect. - Expect: disconnect. """ from test_framework.blocktools import create_block, create_coinbase from test_framework.messages import CInv @@ -521,7 +515,8 @@ def test_nonnull_locators(self, test_node, inv_node): self.log.info("Part 5: Testing handling of unconnecting headers") # First we test that receipt of an unconnecting header doesn't prevent # chain sync. - for i in range(10): + NUM_HEADERS = 100 + for i in range(NUM_HEADERS): self.log.debug("Part 5.{}: starting...".format(i)) test_node.last_message.pop("getdata", None) blocks = [] @@ -546,15 +541,14 @@ def test_nonnull_locators(self, test_node, inv_node): blocks = [] # Now we test that if we repeatedly don't send connecting headers, we # don't go into an infinite loop trying to get them to connect. - MAX_NUM_UNCONNECTING_HEADERS_MSGS = 10 - for _ in range(MAX_NUM_UNCONNECTING_HEADERS_MSGS + 1): + for _ in range(NUM_HEADERS + 1): blocks.append(create_block(tip, create_coinbase(height), block_time)) blocks[-1].solve() tip = blocks[-1].sha256 block_time += 1 height += 1 - for i in range(1, MAX_NUM_UNCONNECTING_HEADERS_MSGS): + for i in range(1, NUM_HEADERS): with p2p_lock: test_node.last_message.pop("getheaders", None) # Send an empty header as failed response to the receive getheaders, to @@ -565,28 +559,6 @@ def test_nonnull_locators(self, test_node, inv_node): test_node.send_header_for_blocks([blocks[i]]) test_node.wait_for_getheaders() - # Next header will connect, should re-set our count: - test_node.send_header_for_blocks([blocks[0]]) - - # Remove the first two entries (blocks[1] would connect): - blocks = blocks[2:] - - # Now try to see how many unconnecting headers we can send - # before we get disconnected. Should be 5*MAX_NUM_UNCONNECTING_HEADERS_MSGS - for i in range(5 * MAX_NUM_UNCONNECTING_HEADERS_MSGS - 1): - # Send a header that doesn't connect, check that we get a getheaders. - with p2p_lock: - test_node.last_message.pop("getheaders", None) - test_node.send_header_for_blocks([]) - test_node.send_header_for_blocks([blocks[i % len(blocks)]]) - test_node.wait_for_getheaders() - - # Eventually this stops working. - test_node.send_header_for_blocks([blocks[-1]]) - - # Should get disconnected - test_node.wait_for_disconnect() - self.log.info("Part 5: success!") # Finally, check that the inv node never received a getdata request, From a1ac6b7f7ceebe5bab231d14d07c49da9315489a Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 7 Mar 2024 11:27:31 -0500 Subject: [PATCH 3/5] net_processing: drop 8 headers threshold for incoming BIP130 With the Misbehavior score gone for non-connecting headers (see previous commit), there is no need to only treat headers messages with up to 8 headers as potential BIP130 announcements. BIP130 does not specify such a limit; it was purely a heuristic. --- src/net_processing.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6fc1b841dd257..86a2569d295c2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -607,10 +607,10 @@ class PeerManagerImpl final : public PeerManager bool CheckHeadersPoW(const std::vector& headers, const Consensus::Params& consensusParams, Peer& peer); /** Calculate an anti-DoS work threshold for headers chains */ arith_uint256 GetAntiDoSWorkThreshold(); - /** Deal with state tracking and headers sync for peers that send the - * occasional non-connecting header (this can happen due to BIP 130 headers + /** Deal with state tracking and headers sync for peers that send + * non-connecting headers (this can happen due to BIP 130 headers * announcements for blocks interacting with the 2hr (MAX_FUTURE_BLOCK_TIME) rule). */ - void HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector& headers) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); + void HandleUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector& headers) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); /** Return true if the headers connect to each other, false otherwise */ bool CheckHeadersAreContinuous(const std::vector& headers) const; /** Try to continue a low-work headers sync that has already begun. @@ -2575,7 +2575,7 @@ arith_uint256 PeerManagerImpl::GetAntiDoSWorkThreshold() * * We'll send a getheaders message in response to try to connect the chain. */ -void PeerManagerImpl::HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer, +void PeerManagerImpl::HandleUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector& headers) { // Try to fill in the missing headers. @@ -2960,12 +2960,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, bool headers_connect_blockindex{chain_start_header != nullptr}; if (!headers_connect_blockindex) { - if (nCount <= MAX_BLOCKS_TO_ANNOUNCE) { - // If this looks like it could be a BIP 130 block announcement, use - // special logic for handling headers that don't connect, as this - // could be benign. - HandleFewUnconnectingHeaders(pfrom, peer, headers); - } + // This could be a BIP 130 block announcement, use + // special logic for handling headers that don't connect, as this + // could be benign. + HandleUnconnectingHeaders(pfrom, peer, headers); return; } From 3cbf1f3f9987f43be4bd30baa428b094faba4912 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 5 Mar 2024 08:28:14 -0500 Subject: [PATCH 4/5] net_processing: make all Misbehaving increments = 100 This removes the need to actually track misbehavior score (see further commit), because any Misbehaving node will immediately hit the discouragement threshold. --- src/net_processing.cpp | 13 ++++++------- src/test/denialofservice_tests.cpp | 3 +-- test/functional/p2p_addr_relay.py | 3 ++- test/functional/p2p_addrv2_relay.py | 12 +++++++----- test/functional/p2p_invalid_messages.py | 7 +++++-- test/functional/p2p_mutated_blocks.py | 9 ++++----- test/functional/p2p_unrequested_blocks.py | 4 +++- 7 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 86a2569d295c2..0614b45f293cc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1850,8 +1850,7 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati return true; // Conflicting (but not necessarily invalid) data or different policy: case BlockValidationResult::BLOCK_MISSING_PREV: - // TODO: Handle this much more gracefully (10 DoS points is super arbitrary) - if (peer) Misbehaving(*peer, 10, message); + if (peer) Misbehaving(*peer, 100, message); return true; case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: case BlockValidationResult::BLOCK_TIME_FUTURE: @@ -2550,7 +2549,7 @@ bool PeerManagerImpl::CheckHeadersPoW(const std::vector& headers, // Are these headers connected to each other? if (!CheckHeadersAreContinuous(headers)) { - Misbehaving(peer, 20, "non-continuous headers sequence"); + Misbehaving(peer, 100, "non-continuous headers sequence"); return false; } return true; @@ -3814,7 +3813,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (vAddr.size() > MAX_ADDR_TO_SEND) { - Misbehaving(*peer, 20, strprintf("%s message size = %u", msg_type, vAddr.size())); + Misbehaving(*peer, 100, strprintf("%s message size = %u", msg_type, vAddr.size())); return; } @@ -3896,7 +3895,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - Misbehaving(*peer, 20, strprintf("inv message size = %u", vInv.size())); + Misbehaving(*peer, 100, strprintf("inv message size = %u", vInv.size())); return; } @@ -3988,7 +3987,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - Misbehaving(*peer, 20, strprintf("getdata message size = %u", vInv.size())); + Misbehaving(*peer, 100, strprintf("getdata message size = %u", vInv.size())); return; } @@ -4679,7 +4678,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. unsigned int nCount = ReadCompactSize(vRecv); if (nCount > MAX_HEADERS_RESULTS) { - Misbehaving(*peer, 20, strprintf("headers message size = %u", nCount)); + Misbehaving(*peer, 100, strprintf("headers message size = %u", nCount)); return; } headers.resize(nCount); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 0fef8c59069e1..88efccc625683 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -352,7 +352,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[1], NODE_NETWORK); nodes[1]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[1]); - peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1); BOOST_CHECK(peerLogic->SendMessages(nodes[1])); // [0] is still discouraged/disconnected. BOOST_CHECK(banman->IsDiscouraged(addr[0])); @@ -360,7 +359,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) // [1] is not discouraged/disconnected yet. BOOST_CHECK(!banman->IsDiscouraged(addr[1])); BOOST_CHECK(!nodes[1]->fDisconnect); - peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), 1); // [1] reaches discouragement threshold + peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD); // [1] reaches discouragement threshold BOOST_CHECK(peerLogic->SendMessages(nodes[1])); // Expect both [0] and [1] to be discouraged/disconnected now. BOOST_CHECK(banman->IsDiscouraged(addr[0])); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index b23ec1028b615..d10e47e0367ac 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -142,7 +142,8 @@ def oversized_addr_test(self): msg = self.setup_addr_msg(1010) with self.nodes[0].assert_debug_log(['addr message size = 1010']): - addr_source.send_and_ping(msg) + addr_source.send_message(msg) + addr_source.wait_for_disconnect() self.nodes[0].disconnect_p2ps() diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index f9a8c44be285d..c67f93651718d 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -79,11 +79,6 @@ def run_test(self): addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) msg = msg_addrv2() - self.log.info('Send too-large addrv2 message') - msg.addrs = ADDRS * 101 - with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']): - addr_source.send_and_ping(msg) - self.log.info('Check that addrv2 message content is relayed and added to addrman') addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) msg.addrs = ADDRS @@ -99,6 +94,13 @@ def run_test(self): assert addr_receiver.addrv2_received_and_checked assert_equal(len(self.nodes[0].getnodeaddresses(count=0, network="i2p")), 0) + self.log.info('Send too-large addrv2 message') + msg.addrs = ADDRS * 101 + with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']): + addr_source.send_message(msg) + addr_source.wait_for_disconnect() + + if __name__ == '__main__': AddrTest().main() diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 40a69936bccca..cb78313786247 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -261,7 +261,9 @@ def test_oversized_msg(self, msg, size): msg_type = msg.msgtype.decode('ascii') self.log.info("Test {} message of size {} is logged as misbehaving".format(msg_type, size)) with self.nodes[0].assert_debug_log(['Misbehaving', '{} message size = {}'.format(msg_type, size)]): - self.nodes[0].add_p2p_connection(P2PInterface()).send_and_ping(msg) + conn = self.nodes[0].add_p2p_connection(P2PInterface()) + conn.send_message(msg) + conn.wait_for_disconnect() self.nodes[0].disconnect_p2ps() def test_oversized_inv_msg(self): @@ -322,7 +324,8 @@ def test_noncontinuous_headers_msg(self): # delete arbitrary block header somewhere in the middle to break link del block_headers[random.randrange(1, len(block_headers)-1)] with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): - peer.send_and_ping(msg_headers(block_headers)) + peer.send_message(msg_headers(block_headers)) + peer.wait_for_disconnect() self.nodes[0].disconnect_p2ps() def test_resource_exhaustion(self): diff --git a/test/functional/p2p_mutated_blocks.py b/test/functional/p2p_mutated_blocks.py index 737edaf5bf3ed..6a9bbc0a8ca50 100755 --- a/test/functional/p2p_mutated_blocks.py +++ b/test/functional/p2p_mutated_blocks.py @@ -104,11 +104,10 @@ def self_transfer_requested(): block_missing_prev.hashPrevBlock = 123 block_missing_prev.solve() - # Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100 - for _ in range(10): - assert_equal(len(self.nodes[0].getpeerinfo()), 2) - with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]): - attacker.send_message(msg_block(block_missing_prev)) + # Check that non-connecting block causes disconnect + assert_equal(len(self.nodes[0].getpeerinfo()), 2) + with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]): + attacker.send_message(msg_block(block_missing_prev)) attacker.wait_for_disconnect(timeout=5) diff --git a/test/functional/p2p_unrequested_blocks.py b/test/functional/p2p_unrequested_blocks.py index f368434895127..776eaf5255cfb 100755 --- a/test/functional/p2p_unrequested_blocks.py +++ b/test/functional/p2p_unrequested_blocks.py @@ -170,9 +170,11 @@ def run_test(self): tip = next_block # Now send the block at height 5 and check that it wasn't accepted (missing header) - test_node.send_and_ping(msg_block(all_blocks[1])) + test_node.send_message(msg_block(all_blocks[1])) + test_node.wait_for_disconnect() assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblock, all_blocks[1].hash) assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblockheader, all_blocks[1].hash) + test_node = self.nodes[0].add_p2p_connection(P2PInterface()) # The block at height 5 should be accepted if we provide the missing header, though headers_message = msg_headers() From e7ccaa01e4e0ef502d5727367baa54bcdcf1fb83 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 5 Mar 2024 08:46:03 -0500 Subject: [PATCH 5/5] net_processing: remove Misbehavior score and increments This is now all unused. --- src/banman.h | 2 +- src/net_processing.cpp | 68 +++++++++++------------------- src/net_processing.h | 4 +- src/test/denialofservice_tests.cpp | 8 ++-- 4 files changed, 31 insertions(+), 51 deletions(-) diff --git a/src/banman.h b/src/banman.h index c6df7ec3c3d6b..57ba2ac23ce58 100644 --- a/src/banman.h +++ b/src/banman.h @@ -34,7 +34,7 @@ class CSubNet; // disk on shutdown and reloaded on startup. Banning can be used to // prevent connections with spy nodes or other griefers. // -// 2. Discouragement. If a peer misbehaves enough (see Misbehaving() in +// 2. Discouragement. If a peer misbehaves (see Misbehaving() in // net_processing.cpp), we'll mark that address as discouraged. We still allow // incoming connections from them, but they're preferred for eviction when // we receive new incoming connections. We never make outgoing connections to diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0614b45f293cc..c905a55a730fc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -222,8 +222,6 @@ struct Peer { /** Protects misbehavior data members */ Mutex m_misbehavior_mutex; - /** Accumulated misbehavior score for this peer */ - int m_misbehavior_score GUARDED_BY(m_misbehavior_mutex){0}; /** Whether this peer should be disconnected and marked as discouraged (unless it has NetPermissionFlags::NoBan permission). */ bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false}; @@ -516,7 +514,7 @@ class PeerManagerImpl final : public PeerManager m_best_height = height; m_best_block_time = time; }; - void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); }; + void UnitTestMisbehaving(NodeId peer_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), ""); }; void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex); @@ -541,11 +539,9 @@ class PeerManagerImpl final : public PeerManager * May return an empty shared_ptr if the Peer object can't be found. */ PeerRef RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - /** - * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node - * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter. - */ - void Misbehaving(Peer& peer, int howmuch, const std::string& message); + /** Mark a peer as misbehaving, which will cause it to be disconnected and its + * address discouraged. */ + void Misbehaving(Peer& peer, const std::string& message); /** * Potentially mark a node discouraged based on the contents of a BlockValidationState object @@ -1621,7 +1617,6 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) void PeerManagerImpl::FinalizeNode(const CNode& node) { NodeId nodeid = node.GetId(); - int misbehavior{0}; { LOCK(cs_main); { @@ -1632,7 +1627,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) // destructed. PeerRef peer = RemovePeer(nodeid); assert(peer != nullptr); - misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score); m_wtxid_relay_peers -= peer->m_wtxid_relay; assert(m_wtxid_relay_peers >= 0); } @@ -1675,7 +1669,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_orphanage.Size() == 0); } } // cs_main - if (node.fSuccessfullyConnected && misbehavior == 0 && + if (node.fSuccessfullyConnected && !node.IsBlockOnlyConn() && !node.IsInboundConn()) { // Only change visible addrman state for full outbound peers. We don't // call Connected() for feeler connections since they don't have @@ -1787,25 +1781,13 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs; } -void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message) +void PeerManagerImpl::Misbehaving(Peer& peer, const std::string& message) { - assert(howmuch > 0); - LOCK(peer.m_misbehavior_mutex); - const int score_before{peer.m_misbehavior_score}; - peer.m_misbehavior_score += howmuch; - const int score_now{peer.m_misbehavior_score}; const std::string message_prefixed = message.empty() ? "" : (": " + message); - std::string warning; - - if (score_now >= DISCOURAGEMENT_THRESHOLD && score_before < DISCOURAGEMENT_THRESHOLD) { - warning = " DISCOURAGE THRESHOLD EXCEEDED"; - peer.m_should_discourage = true; - } - - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s%s\n", - peer.m_id, score_before, score_now, warning, message_prefixed); + peer.m_should_discourage = true; + LogPrint(BCLog::NET, "Misbehaving: peer=%d%s\n", peer.m_id, message_prefixed); } bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, @@ -1823,7 +1805,7 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati case BlockValidationResult::BLOCK_CONSENSUS: case BlockValidationResult::BLOCK_MUTATED: if (!via_compact_block) { - if (peer) Misbehaving(*peer, 100, message); + if (peer) Misbehaving(*peer, message); return true; } break; @@ -1838,7 +1820,7 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati // Discourage outbound (but not inbound) peers if on an invalid chain. // Exempt HB compact block peers. Manual connections are always protected from discouragement. if (!via_compact_block && !node_state->m_is_inbound) { - if (peer) Misbehaving(*peer, 100, message); + if (peer) Misbehaving(*peer, message); return true; } break; @@ -1846,11 +1828,11 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati case BlockValidationResult::BLOCK_INVALID_HEADER: case BlockValidationResult::BLOCK_CHECKPOINT: case BlockValidationResult::BLOCK_INVALID_PREV: - if (peer) Misbehaving(*peer, 100, message); + if (peer) Misbehaving(*peer, message); return true; // Conflicting (but not necessarily invalid) data or different policy: case BlockValidationResult::BLOCK_MISSING_PREV: - if (peer) Misbehaving(*peer, 100, message); + if (peer) Misbehaving(*peer, message); return true; case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: case BlockValidationResult::BLOCK_TIME_FUTURE: @@ -1870,7 +1852,7 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat break; // The node is providing invalid data: case TxValidationResult::TX_CONSENSUS: - if (peer) Misbehaving(*peer, 100, ""); + if (peer) Misbehaving(*peer, ""); return true; // Conflicting (but not necessarily invalid) data or different policy: case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE: @@ -2530,7 +2512,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo BlockTransactions resp(req); for (size_t i = 0; i < req.indexes.size(); i++) { if (req.indexes[i] >= block.vtx.size()) { - Misbehaving(peer, 100, "getblocktxn with out-of-bounds tx indices"); + Misbehaving(peer, "getblocktxn with out-of-bounds tx indices"); return; } resp.txn[i] = block.vtx[req.indexes[i]]; @@ -2543,13 +2525,13 @@ bool PeerManagerImpl::CheckHeadersPoW(const std::vector& headers, { // Do these headers have proof-of-work matching what's claimed? if (!HasValidProofOfWork(headers, consensusParams)) { - Misbehaving(peer, 100, "header with invalid proof of work"); + Misbehaving(peer, "header with invalid proof of work"); return false; } // Are these headers connected to each other? if (!CheckHeadersAreContinuous(headers)) { - Misbehaving(peer, 100, "non-continuous headers sequence"); + Misbehaving(peer, "non-continuous headers sequence"); return false; } return true; @@ -3329,7 +3311,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn); if (status == READ_STATUS_INVALID) { RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect - Misbehaving(peer, 100, "invalid compact block/non-matching block transactions"); + Misbehaving(peer, "invalid compact block/non-matching block transactions"); return; } else if (status == READ_STATUS_FAILED) { if (first_in_flight) { @@ -3813,7 +3795,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (vAddr.size() > MAX_ADDR_TO_SEND) { - Misbehaving(*peer, 100, strprintf("%s message size = %u", msg_type, vAddr.size())); + Misbehaving(*peer, strprintf("%s message size = %u", msg_type, vAddr.size())); return; } @@ -3895,7 +3877,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - Misbehaving(*peer, 100, strprintf("inv message size = %u", vInv.size())); + Misbehaving(*peer, strprintf("inv message size = %u", vInv.size())); return; } @@ -3987,7 +3969,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - Misbehaving(*peer, 100, strprintf("getdata message size = %u", vInv.size())); + Misbehaving(*peer, strprintf("getdata message size = %u", vInv.size())); return; } @@ -4533,7 +4515,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact); if (status == READ_STATUS_INVALID) { RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect - Misbehaving(*peer, 100, "invalid compact block"); + Misbehaving(*peer, "invalid compact block"); return; } else if (status == READ_STATUS_FAILED) { if (first_in_flight) { @@ -4678,7 +4660,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. unsigned int nCount = ReadCompactSize(vRecv); if (nCount > MAX_HEADERS_RESULTS) { - Misbehaving(*peer, 100, strprintf("headers message size = %u", nCount)); + Misbehaving(*peer, strprintf("headers message size = %u", nCount)); return; } headers.resize(nCount); @@ -4725,7 +4707,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (prev_block && IsBlockMutated(/*block=*/*pblock, /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) { LogDebug(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id); - Misbehaving(*peer, 100, "mutated block"); + Misbehaving(*peer, "mutated block"); WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), peer->m_id)); return; } @@ -4906,7 +4888,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (!filter.IsWithinSizeConstraints()) { // There is no excuse for sending a too-large filter - Misbehaving(*peer, 100, "too-large bloom filter"); + Misbehaving(*peer, "too-large bloom filter"); } else if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) { { LOCK(tx_relay->m_bloom_filter_mutex); @@ -4942,7 +4924,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } } if (bad) { - Misbehaving(*peer, 100, "bad filteradd message"); + Misbehaving(*peer, "bad filteradd message"); } return; } diff --git a/src/net_processing.h b/src/net_processing.h index f8d7a8f5115e1..fb1c3be70c598 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -23,8 +23,6 @@ static const uint32_t DEFAULT_MAX_ORPHAN_TRANSACTIONS{100}; static const uint32_t DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN{100}; static const bool DEFAULT_PEERBLOOMFILTERS = false; static const bool DEFAULT_PEERBLOCKFILTERS = false; -/** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */ -static const int DISCOURAGEMENT_THRESHOLD{100}; /** Maximum number of outstanding CMPCTBLOCK requests for the same block. */ static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3; @@ -96,7 +94,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface virtual void SetBestBlock(int height, std::chrono::seconds time) = 0; /* Public for unit testing. */ - virtual void UnitTestMisbehaving(NodeId peer_id, int howmuch) = 0; + virtual void UnitTestMisbehaving(NodeId peer_id) = 0; /** * Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound. diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 88efccc625683..cd112666c72f9 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -332,7 +332,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[0], NODE_NETWORK); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); - peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + peerLogic->UnitTestMisbehaving(nodes[0]->GetId()); // Should be discouraged BOOST_CHECK(peerLogic->SendMessages(nodes[0])); BOOST_CHECK(banman->IsDiscouraged(addr[0])); @@ -359,7 +359,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) // [1] is not discouraged/disconnected yet. BOOST_CHECK(!banman->IsDiscouraged(addr[1])); BOOST_CHECK(!nodes[1]->fDisconnect); - peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD); // [1] reaches discouragement threshold + peerLogic->UnitTestMisbehaving(nodes[1]->GetId()); BOOST_CHECK(peerLogic->SendMessages(nodes[1])); // Expect both [0] and [1] to be discouraged/disconnected now. BOOST_CHECK(banman->IsDiscouraged(addr[0])); @@ -382,7 +382,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[2], NODE_NETWORK); nodes[2]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[2]); - peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD); + peerLogic->UnitTestMisbehaving(nodes[2]->GetId()); BOOST_CHECK(peerLogic->SendMessages(nodes[2])); BOOST_CHECK(banman->IsDiscouraged(addr[0])); BOOST_CHECK(banman->IsDiscouraged(addr[1])); @@ -424,7 +424,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) peerLogic->InitializeNode(dummyNode, NODE_NETWORK); dummyNode.fSuccessfullyConnected = true; - peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); + peerLogic->UnitTestMisbehaving(dummyNode.GetId()); BOOST_CHECK(peerLogic->SendMessages(&dummyNode)); BOOST_CHECK(banman->IsDiscouraged(addr));