Remove the ClientAutoIPv6ORPort option

changes/ticket32905

@@ -0,0 +1,6 @@
+  o Removed features:
+    - Remove the ClientAutoIPv6ORPort option. This option attempted
+      to randomly choose between IPv4 and IPv6 for client connections,
+      and isn't a true implementation of Happy Eyeballs. Often, this
+      option failed on IPv4-only or IPv6-only connections. Closes
+      ticket 32905. Patch by Neel Chauhan.

doc/tor.1.txt

@@ -1018,13 +1018,6 @@ The following options are useful only for clients (that is, if
     via the UI to mobile users for use where bandwidth may be expensive.
     (Default: 0)
 
-[[ClientAutoIPv6ORPort]] **ClientAutoIPv6ORPort** **0**|**1**::
-    If this option is set to 1, Tor clients randomly prefer a node's IPv4 or
-    IPv6 ORPort. The random preference is set every time a node is loaded
-    from a new consensus or bridge config. When this option is set to 1,
-    **ClientPreferIPv6ORPort** is ignored. (Default: 0) (DEPRECATED: This
-    option is unreliable if a connection isn't reliably dual-stack.)
-
 [[ClientBootstrapConsensusAuthorityDownloadInitialDelay]] **ClientBootstrapConsensusAuthorityDownloadInitialDelay** __N__::
     Initial delay in seconds for when clients should download consensuses from authorities
     if they are bootstrapping (that is, they don't have a usable, reasonably

src/app/config/config.c

@@ -366,7 +366,7 @@ static const config_var_t option_vars_[] = {
 #endif /* defined(HAVE_MODULE_RELAY) || defined(TOR_UNIT_TESTS) */
   V(ClientPreferIPv6ORPort,      AUTOBOOL, "auto"),
   V(ClientPreferIPv6DirPort,     AUTOBOOL, "auto"),
-  V(ClientAutoIPv6ORPort,        BOOL,     "0"),
+  OBSOLETE("ClientAutoIPv6ORPort"),
   V(ClientRejectInternalAddresses, BOOL,   "1"),
   V(ClientTransportPlugin,       LINELIST, NULL),
   V(ClientUseIPv6,               BOOL,     "0"),

src/app/config/or_options_st.h

@@ -662,9 +662,6 @@ struct or_options_t {
    * accessing this value directly.  */
   int ClientPreferIPv6DirPort;
 
-  /** If true, prefer an IPv4 or IPv6 OR port at random. */
-  int ClientAutoIPv6ORPort;
-
   /** The length of time that we think a consensus should be fresh. */
   int V3AuthVotingInterval;
   /** The length of time we think it will take to distribute votes. */

src/core/or/policies.c

@@ -463,8 +463,7 @@ fascist_firewall_use_ipv6(const or_options_t *options)
    * ClientPreferIPv6DirPort is deprecated, but check it anyway. */
   return (options->ClientUseIPv6 == 1 || options->ClientUseIPv4 == 0 ||
           options->ClientPreferIPv6ORPort == 1 ||
-          options->ClientPreferIPv6DirPort == 1 || options->UseBridges == 1 ||
+          options->ClientPreferIPv6DirPort == 1 || options->UseBridges == 1);
-          options->ClientAutoIPv6ORPort == 1);
 }
 
 /** Do we prefer to connect to IPv6, ignoring ClientPreferIPv6ORPort and
@@ -491,15 +490,6 @@ fascist_firewall_prefer_ipv6_impl(const or_options_t *options)
   return -1;
 }
 
-/* Choose whether we prefer IPv4 or IPv6 by randomly choosing an address
- * family. Return 0 for IPv4, and 1 for IPv6. */
-MOCK_IMPL(int,
-fascist_firewall_rand_prefer_ipv6_addr, (void))
-{
-  /* TODO: Check for failures, and infer our preference based on this. */
-  return crypto_rand_int(2);
-}
-
 /** Do we prefer to connect to IPv6 ORPorts?
  * Use node_ipv6_or_preferred() whenever possible: it supports bridge client
  * per-node IPv6 preferences.
@@ -514,10 +504,7 @@ fascist_firewall_prefer_ipv6_orport(const or_options_t *options)
   }
 
   /* We can use both IPv4 and IPv6 - which do we prefer? */
-  if (options->ClientAutoIPv6ORPort == 1) {
+  if (options->ClientPreferIPv6ORPort == 1) {
-    /* If ClientAutoIPv6ORPort is 1, we prefer IPv4 or IPv6 at random. */
-    return fascist_firewall_rand_prefer_ipv6_addr();
-  } else if (options->ClientPreferIPv6ORPort == 1) {
     return 1;
   }
 

src/core/or/policies.h

@@ -70,7 +70,6 @@ typedef struct short_policy_t {
 int firewall_is_fascist_or(void);
 int firewall_is_fascist_dir(void);
 int fascist_firewall_use_ipv6(const or_options_t *options);
-MOCK_DECL(int, fascist_firewall_rand_prefer_ipv6_addr, (void));
 int fascist_firewall_prefer_ipv6_orport(const or_options_t *options);
 int fascist_firewall_prefer_ipv6_dirport(const or_options_t *options);
 

src/feature/client/bridges.c

@@ -844,8 +844,7 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node)
       }
     }
 
-    if (options->ClientPreferIPv6ORPort == -1 ||
+    if (options->ClientPreferIPv6ORPort == -1) {
-        options->ClientAutoIPv6ORPort == 0) {
       /* Mark which address to use based on which bridge_t we got. */
       node->ipv6_preferred = (tor_addr_family(&bridge->addr) == AF_INET6 &&
                               !tor_addr_is_null(&node->ri->ipv6_addr));

src/test/conf_examples/large_1/expected

@@ -15,7 +15,6 @@ CellStatistics 1
 CircuitBuildTimeout 200
 CircuitsAvailableTimeout 10
 CircuitStreamTimeout 20
-ClientAutoIPv6ORPort 1
 ClientOnly 1
 ClientPreferIPv6DirPort 1
 ClientPreferIPv6ORPort 1

src/test/conf_examples/large_1/expected_no_dirauth

@@ -15,7 +15,6 @@ CellStatistics 1
 CircuitBuildTimeout 200
 CircuitsAvailableTimeout 10
 CircuitStreamTimeout 20
-ClientAutoIPv6ORPort 1
 ClientOnly 1
 ClientPreferIPv6DirPort 1
 ClientPreferIPv6ORPort 1

src/test/conf_examples/large_1/torrc

@@ -16,7 +16,6 @@ CircuitBuildTimeout 200
 CircuitPadding 1
 CircuitsAvailableTimeout 10
 CircuitStreamTimeout 20
-ClientAutoIPv6ORPort 1
 ClientOnly 1
 ClientPreferIPv6DirPort 1
 ClientPreferIPv6ORPort 1

src/test/test_policy.c

@@ -2124,20 +2124,6 @@ test_policies_fascist_firewall_allows_address(void *arg)
     teardown_capture_of_logs(); \
   STMT_END
 
-/** Mock the preferred address function to return zero (prefer IPv4). */
-static int
-mock_fascist_firewall_rand_prefer_ipv6_addr_use_ipv4(void)
-{
-  return 0;
-}
-
-/** Mock the preferred address function to return one (prefer IPv6). */
-static int
-mock_fascist_firewall_rand_prefer_ipv6_addr_use_ipv6(void)
-{
-  return 1;
-}
-
 /** Run unit tests for fascist_firewall_choose_address */
 static void
 test_policies_fascist_firewall_choose_address(void *arg)
@@ -2536,42 +2522,6 @@ test_policies_fascist_firewall_choose_address(void *arg)
   CHECK_CHOSEN_ADDR_RN(fake_rs, fake_node, FIREWALL_DIR_CONNECTION, 1, 1,
                        ipv4_dir_ap);
 
-  /* Test ClientAutoIPv6ORPort and pretend we prefer IPv4. */
-  memset(&mock_options, 0, sizeof(or_options_t));
-  mock_options.ClientAutoIPv6ORPort = 1;
-  mock_options.ClientUseIPv4 = 1;
-  mock_options.ClientUseIPv6 = 1;
-  MOCK(fascist_firewall_rand_prefer_ipv6_addr,
-       mock_fascist_firewall_rand_prefer_ipv6_addr_use_ipv4);
-  /* Simulate the initialisation of fake_node.ipv6_preferred */
-  fake_node.ipv6_preferred = fascist_firewall_prefer_ipv6_orport(
-                                                                &mock_options);
-
-  CHECK_CHOSEN_ADDR_RN(fake_rs, fake_node, FIREWALL_OR_CONNECTION, 0, 1,
-                       ipv4_or_ap);
-  CHECK_CHOSEN_ADDR_RN(fake_rs, fake_node, FIREWALL_OR_CONNECTION, 1, 1,
-                       ipv4_or_ap);
-
-  UNMOCK(fascist_firewall_rand_prefer_ipv6_addr);
-
-  /* Test ClientAutoIPv6ORPort and pretend we prefer IPv6. */
-  memset(&mock_options, 0, sizeof(or_options_t));
-  mock_options.ClientAutoIPv6ORPort = 1;
-  mock_options.ClientUseIPv4 = 1;
-  mock_options.ClientUseIPv6 = 1;
-  MOCK(fascist_firewall_rand_prefer_ipv6_addr,
-       mock_fascist_firewall_rand_prefer_ipv6_addr_use_ipv6);
-  /* Simulate the initialisation of fake_node.ipv6_preferred */
-  fake_node.ipv6_preferred = fascist_firewall_prefer_ipv6_orport(
-                                                                &mock_options);
-
-  CHECK_CHOSEN_ADDR_RN(fake_rs, fake_node, FIREWALL_OR_CONNECTION, 0, 1,
-                       ipv6_or_ap);
-  CHECK_CHOSEN_ADDR_RN(fake_rs, fake_node, FIREWALL_OR_CONNECTION, 1, 1,
-                       ipv6_or_ap);
-
-  UNMOCK(fascist_firewall_rand_prefer_ipv6_addr);
-
   /* Test firewall_choose_address_ls(). To do this, we make a fake link
    * specifier. */
   smartlist_t *lspecs = smartlist_new(),