Merge bitcoin/bitcoin#29213: doc, test: test and explain service flag handling

74ebd4d doc, test: Test and explain service flag handling (Martin Zumsande)

Pull request description:

  Service flags received from the peer-to-peer network are handled differently, depending on how we receive them.
  If received directly from an outbound peer the flags belong to, they replace existing flags.
  If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.

  Document that and add test coverage for it.

ACKs for top commit:
  achow101:
    ACK 74ebd4d
  furszy:
    ACK 74ebd4d
  brunoerg:
    utACK 74ebd4d

Tree-SHA512: 604adc3304b8e3cb1a10dfd017025c10b029bebd3ef533f96bcb5856fee5d4396a9aed4949908b8e7ef267ad21320d1814dd80f88426330c5c9c2c529c497591

Author: achow101

src/addrman.h

@@ -111,13 +111,16 @@ class AddrMan
 
     /**
      * 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);
 
     /**

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

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)