Merge bitcoin/bitcoin#20018: p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified

2555a39 p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)

Pull request description:

  If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.

  Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.

  Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.

  If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.

ACKs for top commit:
  mzumsande:
    Code Review ACK 2555a39
  achow101:
    ACK 2555a39
  vasild:
    ACK 2555a39

Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3

Author: achow101

src/init.cpp

@@ -1825,6 +1825,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         if (connect.size() != 1 || connect[0] != "0") {
             connOptions.m_specified_outgoing = connect;
         }
+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
+            LogPrintf("-seednode is ignored when -connect is used\n");
+        }
+
+        if (args.IsArgSet("-dnsseed") && args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED) && args.IsArgSet("-proxy")) {
+            LogPrintf("-dnsseed is ignored when -connect is used and -proxy is specified\n");
+        }
     }
 
     const std::string& i2psam_arg = args.GetArg("-i2psam", "");

src/net.cpp

@@ -1471,6 +1471,8 @@ void CConnman::ThreadDNSAddressSeed()
         }
 
         LogPrintf("Loading addresses from DNS seed %s\n", seed);
+        // If -proxy is in use, we make an ADDR_FETCH connection to the DNS resolved peer address
+        // for the base dns seed domain in chainparams
         if (HaveNameProxy()) {
             AddAddrFetch(seed);
         } else {
@@ -1492,8 +1494,9 @@ void CConnman::ThreadDNSAddressSeed()
                 }
                 addrman.Add(vAdd, resolveSource);
             } else {
-                // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
-                // instead using them as a addrfetch to get nodes with our desired service bits.
+                // If the seed does not support a subdomain with our desired service bits,
+                // we make an ADDR_FETCH connection to the DNS resolved peer address for the
+                // base dns seed domain in chainparams
                 AddAddrFetch(seed);
             }
         }
@@ -1602,7 +1605,6 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
     {
         for (int64_t nLoop = 0;; nLoop++)
         {
-            ProcessAddrFetch();
             for (const std::string& strAddr : connect)
             {
                 CAddress addr(CService(), NODE_NONE);

test/functional/feature_config_args.py

@@ -251,11 +251,43 @@ def test_seed_peers(self):
         ]):
             self.nodes[0].setmocktime(start + 65)
 
+    def test_connect_with_seednode(self):
+        self.log.info('Test -connect with -seednode')
+        seednode_ignored = ['-seednode is ignored when -connect is used\n']
+        dnsseed_ignored = ['-dnsseed is ignored when -connect is used and -proxy is specified\n']
+        addcon_thread_started = ['addcon thread start\n']
+        self.stop_node(0)
+
+        # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode)
+        # nodes is irrelevant and -seednode is ignored.
+        with self.nodes[0].assert_debug_log(expected_msgs=seednode_ignored):
+            self.start_node(0, extra_args=['-connect=fakeaddress1', '-seednode=fakeaddress2'])
+
+        # With -proxy, an ADDR_FETCH connection is made to a peer that the dns seed resolves to.
+        # ADDR_FETCH connections are not used when -connect is used.
+        with self.nodes[0].assert_debug_log(expected_msgs=dnsseed_ignored):
+            self.restart_node(0, extra_args=['-connect=fakeaddress1', '-dnsseed=1', '-proxy=1.2.3.4'])
+
+        # If the user did not disable -dnsseed, but it was soft-disabled because they provided -connect,
+        # they shouldn't see a warning about -dnsseed being ignored.
+        with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started,
+                unexpected_msgs=dnsseed_ignored):
+            self.restart_node(0, extra_args=['-connect=fakeaddress1', '-proxy=1.2.3.4'])
+
+        # We have to supply expected_msgs as it's a required argument
+        # The expected_msg must be something we are confident will be logged after the unexpected_msg
+        # These cases test for -connect being supplied but only to disable it
+        for connect_arg in ['-connect=0', '-noconnect']:
+            with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started,
+                    unexpected_msgs=seednode_ignored):
+                self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])
+
     def run_test(self):
         self.test_log_buffer()
         self.test_args_log()
         self.test_seed_peers()
         self.test_networkactive()
+        self.test_connect_with_seednode()
 
         self.test_config_file_parser()
         self.test_invalid_command_line_options()