doc, test: Test and explain service flag handling

Service flags are handled differently, depending on whether
validated (if received from the peer) or unvalidated (received
via gossip relay).

3 files changed, 34 insertions, 2 deletions

diff --git a/src/addrman.h b/src/addrman.h
index ef9c766eff..be2ee8c2cb 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -111,13 +111,16 @@ public:
 
     /**
      * Attempt to add one or more addresses to addrman's new table.
+     * If an address already exists in addrman, the existing entry may be updated
+     * (e.g. adding additional service flags). If the existing entry is in the new table,
+     * it may be added to more buckets, improving the probability of selection.
      *
      * @param[in] vAddr           Address records to attempt to add.
      * @param[in] source          The address of the node that sent us these addr records.
      * @param[in] time_penalty    A "time penalty" to apply to the address record's nTime. If a peer
      *                            sends us an address record with nTime=n, then we'll add it to our
      *                            addrman with nTime=(n - time_penalty).
-     * @return    true if at least one address is successfully added. */
+     * @return    true if at least one address is successfully added, or added to an additional bucket. Unaffected by updates. */
     bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty = 0s);
 
     /**
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index df54a62f28..ad1548dc76 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3376,6 +3376,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         vRecv >> CNetAddr::V1(addrMe);
         if (!pfrom.IsInboundConn())
         {
+            // Overwrites potentially existing services. In contrast to this,
+            // unvalidated services received via gossip relay in ADDR/ADDRV2
+            // messages are only ever added but cannot replace existing ones.
             m_addrman.SetServices(pfrom.addr, nServices);
         }
         if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices))
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 5bd4f271cd..9668a85484 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -1068,7 +1068,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
 
 BOOST_AUTO_TEST_CASE(addrman_update_address)
 {
-    // Tests updating nTime via Connected() and nServices via SetServices()
+    // Tests updating nTime via Connected() and nServices via SetServices() and Add()
     auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
     CNetAddr source{ResolveIP("252.2.2.2")};
     CAddress addr{CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE)};
@@ -1095,6 +1095,32 @@ BOOST_AUTO_TEST_CASE(addrman_update_address)
     BOOST_CHECK_EQUAL(vAddr2.size(), 1U);
     BOOST_CHECK(vAddr2.at(0).nTime >= start_time + 10000s);
     BOOST_CHECK_EQUAL(vAddr2.at(0).nServices, NODE_NETWORK_LIMITED);
+
+    // Updating an existing addr through Add() (used in gossip relay) can add additional services but can't remove existing ones.
+    CAddress addr_v2{CAddress(ResolveService("250.1.1.1", 8333), NODE_P2P_V2)};
+    addr_v2.nTime = start_time;
+    BOOST_CHECK(!addrman->Add({addr_v2}, source));
+    std::vector<CAddress> vAddr3{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};
+    BOOST_CHECK_EQUAL(vAddr3.size(), 1U);
+    BOOST_CHECK_EQUAL(vAddr3.at(0).nServices, NODE_P2P_V2 | NODE_NETWORK_LIMITED);
+
+    // SetServices() (used when we connected to them) overwrites existing service flags
+    addrman->SetServices(addr, NODE_NETWORK);
+    std::vector<CAddress> vAddr4{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};
+    BOOST_CHECK_EQUAL(vAddr4.size(), 1U);
+    BOOST_CHECK_EQUAL(vAddr4.at(0).nServices, NODE_NETWORK);
+
+    // Promoting to Tried does not affect the service flags
+    BOOST_CHECK(addrman->Good(addr)); // addr has NODE_NONE
+    std::vector<CAddress> vAddr5{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};
+    BOOST_CHECK_EQUAL(vAddr5.size(), 1U);
+    BOOST_CHECK_EQUAL(vAddr5.at(0).nServices, NODE_NETWORK);
+
+    // Adding service flags even works when the addr is in Tried
+    BOOST_CHECK(!addrman->Add({addr_v2}, source));
+    std::vector<CAddress> vAddr6{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};
+    BOOST_CHECK_EQUAL(vAddr6.size(), 1U);
+    BOOST_CHECK_EQUAL(vAddr6.at(0).nServices, NODE_NETWORK | NODE_P2P_V2);
 }
 
 BOOST_AUTO_TEST_CASE(addrman_size)