net_processing: make any misbehavior trigger immediate discouragement

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

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.

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]));
@@ -352,15 +352,14 @@ 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]));
     BOOST_CHECK(nodes[0]->fDisconnect);
     // [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());
     BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     // Expect both [0] and [1] to be discouraged/disconnected now.
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
@@ -383,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]));
@@ -425,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));
 

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()
 

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()

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):

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)
 
 

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,42 +541,24 @@ 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):
-            # Send a header that doesn't connect, check that we get a getheaders.
+        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
+            # 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()
 
-        # 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([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,

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()