test: add unit test for block-relay-only eviction #23614

nit: You may remove extra space.

done

Suggestion: How about renaming it to:

    constexpr int max_outbound_blocks_only = MAX_BLOCK_RELAY_ONLY_CONNECTIONS;

Since we are following the snake case naming system here, and blocks and only are two different words.

    constexpr int max_outbound_blockrelay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS};

Might be better to call this blockrelay to avoid confusion with -blocksonly.

changed to max_outbound_block_relay, as suggested below.

changed to max_outbound_block_relay

nit

    constexpr int64_t MINIMUM_CONNECT_TIME{30};

done

This has been used before too, but can we name it peerman instead, like connman? Sounds more clear.

Yes, will do if I push again, though this should be probably done everywhere in this test per scripted-diff.

If you have a chance to re-touch

        BOOST_CHECK(!vNodes[i]->fDisconnect);

(and similarly below for both true and false.) It's okay as-is, but it's more standard to not compare against (literal) true and false. You can write boolean_expression instead of boolean_expression == true and !boolean_expression instead of boolean_expression == false.

in new code you can just use nodes;

Alternatively, use range-based for loops throughout:

index ef18ed1385..c74864bd7f 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -207,14 +207,17 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
     connman->Init(options);
     std::vector<CNode*> vNodes;
 
+    // reset global id counter
+    id = 0;
+
     // Add block-relay-only peers up to the limit
     for (int i = 0; i < max_outbound_block_relay; ++i) {
         AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY);
     }
     peerLogic->CheckForStaleTipAndEvictPeers();
 
-    for (int i = 0; i < max_outbound_block_relay; ++i) {
-        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    for (CNode* node : vNodes) {
+        BOOST_CHECK(node->fDisconnect == false);
     }
 
     // Add an extra block-relay-only peer breaking the limit (mocks logic in ThreadOpenConnections)
@@ -222,17 +225,19 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
     peerLogic->CheckForStaleTipAndEvictPeers();
 
     // The extra peer should only get marked for eviction after MINIMUM_CONNECT_TIME
-    for (int i = 0; i < max_outbound_block_relay; ++i) {
-        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    for (CNode* node : vNodes) {
+        BOOST_CHECK(node->fDisconnect == false);
     }
-    BOOST_CHECK(vNodes.back()->fDisconnect == false);
 
     SetMockTime(GetTime() + MINIMUM_CONNECT_TIME + 1);
     peerLogic->CheckForStaleTipAndEvictPeers();
-    for (int i = 0; i < max_outbound_block_relay; ++i) {
-        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    for (CNode* node : vNodes) {
+        if (node->GetId() == max_outbound_block_relay) {
+            BOOST_CHECK(node->fDisconnect == true);
+        } else {
+            BOOST_CHECK(node->fDisconnect == false);
+        }
     }
-    BOOST_CHECK(vNodes.back()->fDisconnect == true);
 
     // Update the last block time for the extra peer,
     // and check that the next youngest peer gets evicted.
@@ -240,11 +245,13 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
     vNodes.back()->nLastBlockTime = GetTime();
 
     peerLogic->CheckForStaleTipAndEvictPeers();
-    for (int i = 0; i < max_outbound_block_relay - 1; ++i) {
-        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    for (CNode* node : vNodes) {
+        if (node->GetId() == max_outbound_block_relay - 1) {
+            BOOST_CHECK(node->fDisconnect == true);
+        } else {
+            BOOST_CHECK(node->fDisconnect == false);
+        }
     }
-    BOOST_CHECK(vNodes[max_outbound_block_relay - 1]->fDisconnect == true);
-    BOOST_CHECK(vNodes.back()->fDisconnect == false);
 
     for (const CNode* node : vNodes) {
         peerLogic->FinalizeNode(*node);

New parameters/variables should use snake_case:

static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connection_type)