Merge #20187: Addrman: test-before-evict bugfix and improvements for block-relay-only peers

16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751 Avoid test-before-evict evictions of current peers (Suhas Daftuar)
e8b215a086d91a8774210bb6ce8d1560aaaf0789 Refactor test for existing peer connection into own function (Suhas Daftuar)
4fe338ab3ed73b3ffb20eedf95500c56ec2920e1 Call CAddrMan::Good() on block-relay-only peer addresses (Suhas Daftuar)
daf55531260833d597ee599e2d289ea1be0b1d9c Avoid calling CAddrMan::Connected() on block-relay-only peer addresses (Suhas Daftuar)

Pull request description:

  This PR does two things:

  * Block-relay-only interaction with addrman.
    * Calling `CAddrMan::Connected()` on an address that was a block-relay-only peer causes the time we report in `addr` messages containing that peer to be updated; particularly now that we use anchor connections with a our block-relay-only peers, this risks leaking information about those peers.  So, stop this.
    * Avoiding calling `CAddrMan::Good()` on block-relay-only peer addresses causes the addrman logic around maintaining the new and tried table to be less good, and in particular makes it so that block-relay-only peer addresses are more likely to be evicted from the addrman (for no good reason I can think of).  So, mark those addresses as good when we connect.

  * Fix test-before-evict bug. There's a bug where if we get a collision in the tried table with an existing address that is one of our current peers, and the connection is long-lived enough, then `SelectTriedCollisions()` might return that existing peer address to us as a test-before-evict connection candidate. However, our logic for new outbound connections would later prevent us from actually making a connection; the result would be that when we get a collision with a long-lived current peer, that peer's address is likely to get evicted from the tried table.  Fix this by checking to see if a test-before-evict candidate is a peer we're currently connected to, and if so, mark it as `Good()`.

ACKs for top commit:
  sipa:
    utACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751
  amitiuttarwar:
    code review ACK 16d9bfc417
  mzumsande:
    Code-Review ACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751.
  jnewbery:
    utACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751
  ariard:
    Code Review ACK 16d9bfc.
  jonatack:
    Tested ACK 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751

Tree-SHA512: 188ccb814e436937cbb91d29d73c316ce83f4b9c22f1cda56747f0949a093e10161ae724e87e4a2d85ac40f85f5f6b4e87e97d350a1ac44f80c57783f4423324

1 files changed, 5 insertions, 5 deletions

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 6743dc0070..c399da900f 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -129,7 +129,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     SetMockTime(0);
 
     bool dummy;
-    peerLogic->FinalizeNode(dummyNode1.GetId(), dummy);
+    peerLogic->FinalizeNode(dummyNode1, dummy);
 }
 
 static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerManager &peerLogic, CConnmanTest* connman)
@@ -211,7 +211,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 
     bool dummy;
     for (const CNode *node : vNodes) {
-        peerLogic->FinalizeNode(node->GetId(), dummy);
+        peerLogic->FinalizeNode(*node, dummy);
     }
 
     connman->ClearNodes();
@@ -259,8 +259,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     BOOST_CHECK(banman->IsDiscouraged(addr2));  // to be discouraged now
 
     bool dummy;
-    peerLogic->FinalizeNode(dummyNode1.GetId(), dummy);
-    peerLogic->FinalizeNode(dummyNode2.GetId(), dummy);
+    peerLogic->FinalizeNode(dummyNode1, dummy);
+    peerLogic->FinalizeNode(dummyNode2, dummy);
 }
 
 BOOST_AUTO_TEST_CASE(DoS_bantime)
@@ -288,7 +288,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     BOOST_CHECK(banman->IsDiscouraged(addr));
 
     bool dummy;
-    peerLogic->FinalizeNode(dummyNode.GetId(), dummy);
+    peerLogic->FinalizeNode(dummyNode, dummy);
 }
 
 static CTransactionRef RandomOrphan()