From 3cbf1f3f9987f43be4bd30baa428b094faba4912 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 5 Mar 2024 08:28:14 -0500 Subject: [PATCH] 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()