Merge bitcoin/bitcoin#34538: net: advertise -externalip addresses

dab7f2c984 test: cover -externalip/onlynet interaction in functional test (will)
657a5aa3f3 test: cover -externalip bypassing -onlynet (will)
8c87e32bd3 net: let -externalip bypass -onlynet (will)
f4af02e827 net: add an add_even_if_unreachable argument to AddLocal (will)

Pull request description:

  `-onlynet` is documented to restrict automatic outbound connections, but it also currently prevents `-externalip` addresses from being advertised when their network is not in the `-onlynet` set. This happens because `AddLocal()` rejects addresses outside `g_reachable_nets`, regardless of whether the address was explicitly configured by the user.

  Previous attempts to fix this (#24835 and #25690) removed the `g_reachable_nets` check from `AddLocal()`.

  This PR instead adds an explicit `add_even_if_unreachable` argument to `AddLocal()`. The argument defaults to `false`, and is set to `true` only when adding addresses from `-externalip`.

  As a result, explicitly configured `-externalip` addresses can still be advertised even when their network is excluded by `-onlynet`, while discovered, mapped, bound, Tor control, and I2P SAM addresses continue to use the existing reachable-network filter.

  This keeps the fix scoped to `-externalip` and addresses the concern raised in #25690:

  > I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network

  The branch adds unit coverage for `AddLocal()` and functional coverage in `p2p_addr_selfannouncement.py` for `-onlynet=ipv4 -externalip=<onion>`.

  Fixes: #25336
  Fixes: #25669

ACKs for top commit:
  achow101:
    ACK dab7f2c984
  mzumsande:
    re-ACK dab7f2c984
  w0xlt:
    ACK dab7f2c984

Tree-SHA512: a4ac9334b85da8b6902d3850e21d3a1c9d7dce70bcb79182448c8d5684e24462cd6e440385af7aa4420d9582e4dff9dc9e827ca7a6da0363fff2d3c531784d9b

src/init.cpp

@@ -1825,7 +1825,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     for (const std::string& strAddr : args.GetArgs("-externalip")) {
         const std::optional<CService> addrLocal{Lookup(strAddr, GetListenPort(), fNameLookup)};
         if (addrLocal.has_value() && addrLocal->IsValid())
-            AddLocal(addrLocal.value(), LOCAL_MANUAL);
+            AddLocal(addrLocal.value(), LOCAL_MANUAL, /*add_even_if_unreachable=*/true);
         else
             return InitError(ResolveErrMsg("externalip", strAddr));
     }

src/net.cpp

@@ -275,7 +275,7 @@ void ClearLocal()
 }
 
 // learn a new local address
-bool AddLocal(const CService& addr_, int nScore)
+bool AddLocal(const CService& addr_, int nScore, bool add_even_if_unreachable)
 {
     CService addr{MaybeFlipIPv6toCJDNS(addr_)};
 
@@ -285,7 +285,7 @@ bool AddLocal(const CService& addr_, int nScore)
     if (!fDiscover && nScore < LOCAL_MANUAL)
         return false;
 
-    if (!g_reachable_nets.Contains(addr))
+    if (!g_reachable_nets.Contains(addr) && !add_even_if_unreachable)
         return false;
 
     if (fLogIPs) {
@@ -305,9 +305,9 @@ bool AddLocal(const CService& addr_, int nScore)
     return true;
 }
 
-bool AddLocal(const CNetAddr &addr, int nScore)
+bool AddLocal(const CNetAddr& addr, int nScore, bool add_even_if_unreachable)
 {
-    return AddLocal(CService(addr, GetListenPort()), nScore);
+    return AddLocal(CService(addr, GetListenPort()), nScore, add_even_if_unreachable);
 }
 
 void RemoveLocal(const CService& addr)

src/net.h

@@ -164,8 +164,8 @@ enum
 std::optional<CService> GetLocalAddrForPeer(CNode& node);
 
 void ClearLocal();
-bool AddLocal(const CService& addr, int nScore = LOCAL_NONE);
-bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE);
+bool AddLocal(const CService& addr, int nScore = LOCAL_NONE, bool add_even_if_unreachable = false);
+bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE, bool add_even_if_unreachable = false);
 void RemoveLocal(const CService& addr);
 bool SeenLocal(const CService& addr);
 bool IsLocal(const CService& addr);

src/test/net_tests.cpp

@@ -1626,4 +1626,47 @@ BOOST_AUTO_TEST_CASE(private_broadcast_version_does_not_update_addrman_services)
     m_node.peerman->FinalizeNode(node);
 }
 
+BOOST_AUTO_TEST_CASE(addlocal_onlynet_externalip)
+{
+    // Test that `-externalip` addresses bypass `-onlynet`, but score alone does
+    // not.
+
+    CAddress addr_onion;
+    BOOST_REQUIRE(addr_onion.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));
+    BOOST_REQUIRE(addr_onion.IsValid());
+    BOOST_REQUIRE(addr_onion.IsTor());
+
+    const auto reachable_nets_at_start{g_reachable_nets.All()};
+    const bool discover_orig{fDiscover};
+
+    // Simulate using -onlynet=ipv4 -externalip=<onion>
+    g_reachable_nets.RemoveAll();
+    g_reachable_nets.Add(NET_IPV4);
+    fDiscover = false;
+
+    // Now AddLocal with a non-manual score should fail for an unreachable network.
+    BOOST_CHECK(!AddLocal(addr_onion, LOCAL_BIND));
+    BOOST_CHECK(!IsLocal(addr_onion));
+
+    BOOST_CHECK(!AddLocal(addr_onion, LOCAL_MANUAL));
+    BOOST_CHECK(!IsLocal(addr_onion));
+
+    // Whereas AddLocal for -externalip should succeed.
+    BOOST_CHECK(AddLocal(addr_onion, LOCAL_MANUAL, /*add_even_if_unreachable=*/true));
+    BOOST_CHECK(IsLocal(addr_onion));
+
+    // Normal AddLocal on a reachable network still works.
+    const CNetAddr addr_ipv4{LookupHost("1.2.3.4", false).value()};
+    BOOST_CHECK(AddLocal(addr_ipv4, LOCAL_MANUAL));
+    BOOST_CHECK(IsLocal(CService{addr_ipv4, GetListenPort()}));
+
+    RemoveLocal(CService{addr_ipv4, GetListenPort()});
+    RemoveLocal(addr_onion);
+    g_reachable_nets.RemoveAll();
+    for (const auto& net : reachable_nets_at_start) {
+        g_reachable_nets.Add(net);
+    }
+    fDiscover = discover_orig;
+}
+
 BOOST_AUTO_TEST_SUITE_END()

test/functional/p2p_addr_selfannouncement.py

@@ -23,6 +23,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_greater_than
 
 IP_TO_ANNOUNCE = "42.42.42.42"
+ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
 ONE_DAY = 60 * 60 * 24
 
 
@@ -77,6 +78,7 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
         self.self_announcement_test(outbound=False, addrv2=True)
         self.self_announcement_test(outbound=True, addrv2=False)
         self.self_announcement_test(outbound=True, addrv2=True)
+        self.test_externalip_bypasses_onlynet()
 
     @staticmethod
     def inbound_connection_open_assertions(addr_receiver):
@@ -150,5 +152,17 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
 
         self.nodes[0].disconnect_p2ps()
 
+    def test_externalip_bypasses_onlynet(self):
+        self.log.info("Test that -externalip onion is advertised despite -onlynet=ipv4")
+        self.restart_node(0, extra_args=["-onlynet=ipv4", f"-externalip={ONION_ADDR}"])
+
+        netinfo = self.nodes[0].getnetworkinfo()
+
+        onion_net = next(n for n in netinfo["networks"] if n["name"] == "onion")
+        assert not onion_net["reachable"], "onion should not be reachable under -onlynet=ipv4"
+
+        addrs = [a["address"] for a in netinfo["localaddresses"]]
+        assert ONION_ADDR in addrs, f"onion address missing from localaddresses: {addrs}"
+
 if __name__ == '__main__':
     AddrSelfAnnouncementTest(__file__).main()