Comment the heck out of the stream/circuit attaching process.

src/or/addressmap.c

@@ -376,29 +376,38 @@ addressmap_rewrite(char *address, size_t maxlen,
   char *addr_orig = tor_strdup(address);
   char *log_addr_orig = NULL;
 
+  /* We use a loop here to limit the total number of rewrites we do,
+   * so that we can't hit an infinite loop. */
   for (rewrites = 0; rewrites < 16; rewrites++) {
     int exact_match = 0;
     log_addr_orig = tor_strdup(escaped_safe_str_client(address));
 
+    /* First check to see if there's an exact match for this address */
     ent = strmap_get(addressmap, address);
 
     if (!ent || !ent->new_address) {
+      /* And if we don't have an exact match, try to check whether
+       * we have a pattern-based match.
+       */
       ent = addressmap_match_superdomains(address);
     } else {
       if (ent->src_wildcard && !ent->dst_wildcard &&
           !strcasecmp(address, ent->new_address)) {
-        /* This is a rule like *.example.com example.com, and we just got
+        /* This is a rule like "rewrite *.example.com to example.com", and we
-         * "example.com" */
+         * just got "example.com". Instead of calling it an infinite loop,
+         * call it complete. */
         goto done;
       }
-
       exact_match = 1;
     }
 
     if (!ent || !ent->new_address) {
+      /* We still have no match at all.  We're done! */
       goto done;
     }
 
+    /* Check wither the flags we were passed tell us not to use this
+     * mapping. */
     switch (ent->source) {
       case ADDRMAPSRC_DNS:
         {
@@ -431,6 +440,8 @@ addressmap_rewrite(char *address, size_t maxlen,
         goto done;
     }
 
+    /* Now fill in the address with the new address. That might be via
+     * appending some new stuff to the end, or via just replacing it. */
     if (ent->dst_wildcard && !exact_match) {
       strlcat(address, ".", maxlen);
       strlcat(address, ent->new_address, maxlen);
@@ -438,6 +449,7 @@ addressmap_rewrite(char *address, size_t maxlen,
       strlcpy(address, ent->new_address, maxlen);
     }
 
+    /* Is this now a .exit address?  If so, remember where we got it.*/
     if (!strcmpend(address, ".exit") &&
         strcmpend(addr_orig, ".exit") &&
         exit_source == ADDRMAPSRC_NONE) {

src/or/circuituse.c

@@ -14,7 +14,7 @@
  * module keeps track of which streams can be attached to which circuits (in
  * circuit_get_best()), and attaches streams to circuits (with
  * circuit_try_attaching_streams(), connection_ap_handshake_attach_circuit(),
- * and connection_ap_handshake_attach_chosen_circuit().
+ * and connection_ap_handshake_attach_chosen_circuit() ).
  *
  * This module also makes sure that we are building circuits for all of the
  * predicted ports, using circuit_remove_handled_ports(),
@@ -1876,16 +1876,22 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
             c->state, conn_state_to_string(c->type, c->state));
   }
   tor_assert(ENTRY_TO_CONN(conn)->state == AP_CONN_STATE_CIRCUIT_WAIT);
+
+  /* Will the exit policy of the exit node apply to this stream? */
   check_exit_policy =
       conn->socks_request->command == SOCKS_COMMAND_CONNECT &&
       !conn->use_begindir &&
       !connection_edge_is_rendezvous_stream(ENTRY_TO_EDGE_CONN(conn));
+
+  /* Does this connection want a one-hop circuit? */
   want_onehop = conn->want_onehop;
 
+  /* Do we need a high-uptime circuit? */
   need_uptime = !conn->want_onehop && !conn->use_begindir &&
                 smartlist_contains_int_as_string(options->LongLivedPorts,
                                           conn->socks_request->port);
 
+  /* Do we need an "internal" circuit? */
   if (desired_circuit_purpose != CIRCUIT_PURPOSE_C_GENERAL)
     need_internal = 1;
   else if (conn->use_begindir || conn->want_onehop)
@@ -1893,21 +1899,31 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
   else
     need_internal = 0;
 
-  circ = circuit_get_best(conn, 1, desired_circuit_purpose,
+  /* We now know what kind of circuit we need.  See if there is an
+   * open circuit that we can use for this stream */
+  circ = circuit_get_best(conn, 1 /* Insist on open circuits */,
+                          desired_circuit_purpose,
                           need_uptime, need_internal);
 
   if (circ) {
+    /* We got a circuit that will work for this stream!  We can return it. */
     *circp = circ;
     return 1; /* we're happy */
   }
 
+  /* Okay, there's no circuit open that will work for this stream. Let's
+   * see if there's an in-progress circuit or if we have to launch one */
+
+  /* Do we know enough directory info to build circuits at all? */
   int have_path = have_enough_path_info(!need_internal);
 
   if (!want_onehop && (!router_have_minimum_dir_info() || !have_path)) {
+    /* If we don't have enough directory information, we can't build
+     * multihop circuits.
+     */
     if (!connection_get_by_type(CONN_TYPE_DIR)) {
       int severity = LOG_NOTICE;
-      /* FFFF if this is a tunneled directory fetch, don't yell
+      /* Retry some stuff that might help the connection work. */
-       * as loudly. the user doesn't even know it's happening. */
       if (entry_list_is_constrained(options) &&
           entries_known_but_down(options)) {
         log_fn(severity, LD_APP|LD_DIR,
@@ -1928,14 +1944,16 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
         routerlist_retry_directory_downloads(time(NULL));
       }
     }
-    /* the stream will be dealt with when router_have_minimum_dir_info becomes
+    /* Since we didn't have enough directory info, we can't attach now.  The
-     * 1, or when all directory attempts fail and directory_all_unreachable()
+     * stream will be dealt with when router_have_minimum_dir_info becomes 1,
+     * or when all directory attempts fail and directory_all_unreachable()
      * kills it.
      */
     return 0;
   }
 
-  /* Do we need to check exit policy? */
+  /* Check whether the exit policy of the chosen exit, or the exit policies
+   * of _all_ nodes, would forbid this node. */
   if (check_exit_policy) {
     if (!conn->chosen_exit_name) {
       struct in_addr in;
@@ -1976,16 +1994,25 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
     }
   }
 
-  /* is one already on the way? */
+  /* Now, check whether there already a circuit on the way that could handle
-  circ = circuit_get_best(conn, 0, desired_circuit_purpose,
+   * this stream. This check matches the one above, but this time we
+   * do not require that the circuit will work. */
+  circ = circuit_get_best(conn, 0 /* don't insist on open circuits */,
+                          desired_circuit_purpose,
                           need_uptime, need_internal);
   if (circ)
     log_debug(LD_CIRC, "one on the way!");
+
   if (!circ) {
+    /* No open or in-progress circuit could handle this stream!  We
+     * will have to launch one!
+     */
+
+    /* THe chosen exit node, if there is one. */
     extend_info_t *extend_info=NULL;
-    uint8_t new_circ_purpose;
     const int n_pending = count_pending_general_client_circuits();
 
+    /* Do we have too many pending circuits? */
     if (n_pending >= options->MaxClientCircuitsPending) {
       static ratelim_t delay_limit = RATELIM_INIT(10*60);
       char *m;
@@ -1999,6 +2026,8 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
       return 0;
     }
 
+    /* If this is a hidden service trying to start an introduction point,
+     * handle that case. */
     if (desired_circuit_purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) {
       /* need to pick an intro point */
       rend_data_t *rend_data = ENTRY_TO_EDGE_CONN(conn)->rend_data;
@@ -2036,7 +2065,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
                      "Discarding this circuit.", conn->chosen_exit_name);
             return -1;
           }
-        } else {
+        } else  { /* ! (r && node_has_descriptor(r)) */
           log_debug(LD_DIR, "considering %d, %s",
                     want_onehop, conn->chosen_exit_name);
           if (want_onehop && conn->chosen_exit_name[0] == '$') {
@@ -2059,7 +2088,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
             extend_info = extend_info_new(conn->chosen_exit_name+1,
                                           digest, NULL, NULL, &addr,
                                           conn->socks_request->port);
-          } else {
+          } else { /* ! (want_onehop && conn->chosen_exit_name[0] == '$') */
             /* We will need an onion key for the router, and we
              * don't have one. Refuse or relax requirements. */
             log_fn(opt ? LOG_INFO : LOG_WARN, LD_APP,
@@ -2077,8 +2106,10 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
           }
         }
       }
-    }
+    } /* Done checking for general circutis with chosen exits. */
 
+    /* What purpose do we need to launch this circuit with? */
+    uint8_t new_circ_purpose;
     if (desired_circuit_purpose == CIRCUIT_PURPOSE_C_REND_JOINED)
       new_circ_purpose = CIRCUIT_PURPOSE_C_ESTABLISH_REND;
     else if (desired_circuit_purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT)
@@ -2087,6 +2118,8 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
       new_circ_purpose = desired_circuit_purpose;
 
 #ifdef ENABLE_TOR2WEB_MODE
+    /* If tor2Web is on, then hidden service requests should be one-hop.
+     */
     if (options->Tor2webMode &&
         (new_circ_purpose == CIRCUIT_PURPOSE_C_ESTABLISH_REND ||
          new_circ_purpose == CIRCUIT_PURPOSE_C_INTRODUCING)) {
@@ -2094,6 +2127,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
     }
 #endif
 
+    /* Determine what kind of a circuit to launch, and actually launch it. */
     {
       int flags = CIRCLAUNCH_NEED_CAPACITY;
       if (want_onehop) flags |= CIRCLAUNCH_ONEHOP_TUNNEL;
@@ -2105,6 +2139,8 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
 
     extend_info_free(extend_info);
 
+    /* Now trigger things that need to happen when we launch circuits */
+
     if (desired_circuit_purpose == CIRCUIT_PURPOSE_C_GENERAL) {
       /* We just caused a circuit to get built because of this stream.
        * If this stream has caused a _lot_ of circuits to be built, that's
@@ -2128,6 +2164,10 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
       }
     }
   } /* endif (!circ) */
+
+  /* We either found a good circuit, or launched a new circuit, or failed to
+   * do so. Report success, and delay. */
+
   if (circ) {
     /* Mark the circuit with the isolation fields for this connection.
      * When the circuit arrives, we'll clear these flags: this is
@@ -2327,7 +2367,9 @@ connection_ap_handshake_attach_chosen_circuit(entry_connection_t *conn,
 
   pathbias_count_use_attempt(circ);
 
+  /* Now, actually link the connection. */
   link_apconn_to_circ(conn, circ, cpath);
+
   tor_assert(conn->socks_request);
   if (conn->socks_request->command == SOCKS_COMMAND_CONNECT) {
     if (!conn->use_begindir)
@@ -2342,12 +2384,11 @@ connection_ap_handshake_attach_chosen_circuit(entry_connection_t *conn,
   return 1;
 }
 
-/** Try to find a safe live circuit for CONN_TYPE_AP connection conn. If
+/** Try to find a safe live circuit for stream <b>conn</b>.  If we find one,
- * we don't find one: if conn cannot be handled by any known nodes,
+ * attach the stream, send appropriate cells, and return 1.  Otherwise,
- * warn and return -1 (conn needs to die, and is maybe already marked);
+ * try to launch new circuit(s) for the stream.  If we can launch
- * else launch new circuit (if necessary) and return 0.
+ * circuits, return 0.  Otherwise, if we simply can't proceed with
- * Otherwise, associate conn with a safe live circuit, do the
+ * this stream, return -1. (conn needs to die, and is maybe already marked).
- * right next step, and return 1.
  */
 /* XXXX this function should mark for close whenever it returns -1;
  * its callers shouldn't have to worry about that. */
@@ -2366,6 +2407,7 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
 
   conn_age = (int)(time(NULL) - base_conn->timestamp_created);
 
+  /* Is this connection so old that we should give up on it? */
   if (conn_age >= get_options()->SocksTimeout) {
     int severity = (tor_addr_is_null(&base_conn->addr) && !base_conn->port) ?
       LOG_INFO : LOG_NOTICE;
@@ -2376,12 +2418,14 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
     return -1;
   }
 
+  /* We handle "general" (non-onion) connections much more straightforwardly.
+   */
   if (!connection_edge_is_rendezvous_stream(ENTRY_TO_EDGE_CONN(conn))) {
     /* we're a general conn */
     origin_circuit_t *circ=NULL;
 
     /* Are we linked to a dir conn that aims to fetch a consensus?
-     * We check here because this conn might no longer be needed. */
+     * We check here because the conn might no longer be needed. */
     if (base_conn->linked_conn &&
         base_conn->linked_conn->type == CONN_TYPE_DIR &&
         base_conn->linked_conn->purpose == DIR_PURPOSE_FETCH_CONSENSUS) {
@@ -2399,6 +2443,9 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
       }
     }
 
+    /* If we have a chosen exit, we need to use a circuit that's
+     * open to that exit. See what exit we meant, and whether we can use it.
+     */
     if (conn->chosen_exit_name) {
       const node_t *node = node_get_by_nickname(conn->chosen_exit_name, 1);
       int opt = conn->chosen_exit_optional;
@@ -2412,6 +2459,7 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
                "Requested exit point '%s' is not known. %s.",
                conn->chosen_exit_name, opt ? "Trying others" : "Closing");
         if (opt) {
+          /* If we are allowed to ignore the .exit request, do so */
           conn->chosen_exit_optional = 0;
           tor_free(conn->chosen_exit_name);
           return 0;
@@ -2424,6 +2472,7 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
                "would refuse request. %s.",
                conn->chosen_exit_name, opt ? "Trying others" : "Closing");
         if (opt) {
+          /* If we are allowed to ignore the .exit request, do so */
           conn->chosen_exit_optional = 0;
           tor_free(conn->chosen_exit_name);
           return 0;
@@ -2432,11 +2481,15 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
       }
     }
 
-    /* find the circuit that we should use, if there is one. */
+    /* Find the circuit that we should use, if there is one. Otherwise
+     * launch it. */
     retval = circuit_get_open_circ_or_launch(
         conn, CIRCUIT_PURPOSE_C_GENERAL, &circ);
-    if (retval < 1) // XXXX++ if we totally fail, this still returns 0 -RD
+    if (retval < 1) {
+      /* We were either told "-1" (complete failure) or 0 (circuit in
+       * progress); we can't attach this stream yet. */
       return retval;
+    }
 
     log_debug(LD_APP|LD_CIRC,
               "Attaching apconn to circ %u (stream %d sec old).",
@@ -2445,7 +2498,8 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
      * sucking. */
     circuit_log_path(LOG_INFO,LD_APP|LD_CIRC,circ);
 
-    /* We have found a suitable circuit for our conn. Hurray. */
+    /* We have found a suitable circuit for our conn. Hurray.  Do
+     * the attachment. */
     return connection_ap_handshake_attach_chosen_circuit(conn, circ, NULL);
 
   } else { /* we're a rendezvous conn */

src/or/connection_edge.c

@@ -830,7 +830,8 @@ connection_ap_rescan_and_attach_pending(void)
 #endif
 
 /** Tell any AP streams that are listed as waiting for a new circuit to try
- * again, either attaching to an available circ or launching a new one.
+ * again.  If there is an available circuit for a stream, attach it. Otherwise,
+ * launch a new circuit.
  *
  * If <b>retry</b> is false, only check the list if it contains at least one
  * streams that we have not yet tried to attach to a circuit.
@@ -845,8 +846,9 @@ connection_ap_attach_pending(int retry)
   if (untried_pending_connections == 0 && !retry)
     return;
 
-  /* Don't allow modifications to pending_entry_connections while we are
+  /* Don't allow any modifications to list while we are iterating over
-   * iterating over it. */
+   * it.  We'll put streams back on this list if we can't attach them
+   * immediately. */
   smartlist_t *pending = pending_entry_connections;
   pending_entry_connections = smartlist_new();
 
@@ -873,6 +875,7 @@ connection_ap_attach_pending(int retry)
       continue;
     }
 
+    /* Okay, we're through the sanity checks. Try to handle this stream. */
     if (connection_ap_handshake_attach_circuit(entry_conn) < 0) {
       if (!conn->marked_for_close)
         connection_mark_unattached_ap(entry_conn,
@@ -882,12 +885,17 @@ connection_ap_attach_pending(int retry)
     if (! conn->marked_for_close &&
         conn->type == CONN_TYPE_AP &&
         conn->state == AP_CONN_STATE_CIRCUIT_WAIT) {
+      /* Is it still waiting for a circuit? If so, we didn't attach it,
+       * so it's still pending.  Put it back on the list.
+       */
       if (!smartlist_contains(pending_entry_connections, entry_conn)) {
         smartlist_add(pending_entry_connections, entry_conn);
         continue;
       }
     }
 
+    /* If we got here, then we either closed the connection, or
+     * we attached it. */
     UNMARK();
   } SMARTLIST_FOREACH_END(entry_conn);
 
@@ -1186,6 +1194,8 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
 
   /* Remember the original address so we can tell the user about what
    * they actually said, not just what it turned into. */
+  /* XXX yes, this is the same as out->orig_address above. One is
+   * in the output, and one is in the connection. */
   if (! conn->original_dest_address) {
     /* Is the 'if' necessary here? XXXX */
     conn->original_dest_address = tor_strdup(conn->socks_request->address);
@@ -1193,7 +1203,7 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
 
   /* First, apply MapAddress and MAPADDRESS mappings. We need to do
    * these only for non-reverse lookups, since they don't exist for those.
-   * We need to do this before we consider automapping, since we might
+   * We also need to do this before we consider automapping, since we might
    * e.g. resolve irc.oftc.net into irconionaddress.onion, at which point
    * we'd need to automap it. */
   if (socks->command != SOCKS_COMMAND_RESOLVE_PTR) {
@@ -1205,9 +1215,12 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
     }
   }
 
-  /* Now, handle automapping.  Automapping happens when we're asked to
+  /* Now see if we need to create or return an existing Hostname->IP
-   * resolve a hostname, and AutomapHostsOnResolve is set, and
+   * automapping.  Automapping happens when we're asked to resolve a
-   * the hostname has a suffix listed in AutomapHostsSuffixes.
+   * hostname, and AutomapHostsOnResolve is set, and the hostname has a
+   * suffix listed in AutomapHostsSuffixes.  It's a handy feature
+   * that lets you have Tor assign e.g. IPv6 addresses for .onion
+   * names, and return them safely from DNSPort.
    */
   if (socks->command == SOCKS_COMMAND_RESOLVE &&
       tor_addr_parse(&addr_tmp, socks->address)<0 &&
@@ -1247,7 +1260,8 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
   }
 
   /* Now handle reverse lookups, if they're in the cache.  This doesn't
-   * happen too often, since client-side DNS caching is off by default. */
+   * happen too often, since client-side DNS caching is off by default,
+   * and very deprecated. */
   if (socks->command == SOCKS_COMMAND_RESOLVE_PTR) {
     unsigned rewrite_flags = 0;
     if (conn->entry_cfg.use_cached_ipv4_answers)
@@ -1292,11 +1306,12 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
     }
   }
 
-  /* If we didn't automap it before, then this is still the address
+  /* If we didn't automap it before, then this is still the address that
-   * that came straight from the user, mapped according to any
+   * came straight from the user, mapped according to any
-   * MapAddress/MAPADDRESS commands.  Now other mappings, including
+   * MapAddress/MAPADDRESS commands.  Now apply other mappings,
-   * previously registered Automap entries, TrackHostExits entries,
+   * including previously registered Automap entries (IP back to
-   * and client-side DNS cache entries (not recommended).
+   * hostname), TrackHostExits entries, and client-side DNS cache
+   * entries (if they're turned on).
    */
   if (socks->command != SOCKS_COMMAND_RESOLVE_PTR &&
       !out->automap) {
@@ -1361,11 +1376,14 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   time_t now = time(NULL);
   rewrite_result_t rr;
 
+  /* First we'll do the rewrite part.  Let's see if we get a reasonable
+   * answer.
+   */
   memset(&rr, 0, sizeof(rr));
   connection_ap_handshake_rewrite(conn,&rr);
 
   if (rr.should_close) {
-    /* connection_ap_handshake_rewrite told us to close the connection,
+    /* connection_ap_handshake_rewrite told us to close the connection:
      * either because it sent back an answer, or because it sent back an
      * error */
     connection_mark_unattached_ap(conn, rr.end_reason);
@@ -1379,8 +1397,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   const int automap = rr.automap;
   const addressmap_entry_source_t exit_source = rr.exit_source;
 
-  /* Parse the address provided by SOCKS.  Modify it in-place if it
+  /* Now, we parse the address to see if it's an .onion or .exit or
-   * specifies a hidden-service (.onion) or particular exit node (.exit).
+   * other special address.
    */
   const hostname_type_t addresstype = parse_extended_hostname(socks->address);
 
@@ -1394,8 +1412,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   }
 
   /* If this is a .exit hostname, strip off the .name.exit part, and
-   * see whether we're going to connect there, and otherwise handle it.
+   * see whether we're willing to connect there, and and otherwise handle the
-   * (The ".exit" part got stripped off by "parse_extended_hostname").
+   * .exit address.
    *
    * We'll set chosen_exit_name and/or close the connection as appropriate.
    */
@@ -1407,7 +1425,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     const node_t *node = NULL;
 
     /* If this .exit was added by an AUTOMAP, then it came straight from
-     * a user.  Make sure that options->AllowDotExit permits that. */
+     * a user.  Make sure that options->AllowDotExit permits that! */
     if (exit_source == ADDRMAPSRC_AUTOMAP && !options->AllowDotExit) {
       /* Whoops; this one is stale.  It must have gotten added earlier,
        * when AllowDotExit was on. */
@@ -1436,7 +1454,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     }
 
     tor_assert(!automap);
-    /* Now, find the character before the .(name) part. */
+
+    /* Now, find the character before the .(name) part.
+     * (The ".exit" part got stripped off by "parse_extended_hostname").
+     *
+     * We're going to put the exit name into conn->chosen_exit_name, and
+     * look up a node correspondingly. */
     char *s = strrchr(socks->address,'.');
     if (s) {
       /* The address was of the form "(stuff).(name).exit */
@@ -1492,10 +1515,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
        implies no. */
   }
 
-  /* Now, handle everything that isn't a .onion address. */
+  /* Now, we handle everything that isn't a .onion address. */
   if (addresstype != ONION_HOSTNAME) {
     /* Not a hidden-service request.  It's either a hostname or an IP,
-     * possibly with a .exit that we stripped off. */
+     * possibly with a .exit that we stripped off.  We're going to check
+     * if we're allowed to connect/resolve there, and then launch the
+     * appropriate request. */
 
     /* Check for funny characters in the address. */
     if (address_is_invalid_destination(socks->address, 1)) {
@@ -1542,7 +1567,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     }
 
     /* Then check if we have a hostname or IP address, and whether DNS or
-     * the IP address family are permitted */
+     * the IP address family are permitted.  Reject if not. */
     tor_addr_t dummy_addr;
     int socks_family = tor_addr_parse(&dummy_addr, socks->address);
     /* family will be -1 for a non-onion hostname that's not an IP */
@@ -1564,8 +1589,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
                safe_str_client(socks->address));
       connection_mark_unattached_ap(conn, END_STREAM_REASON_ENTRYPOLICY);
       return -1;
+    } else {
+      tor_assert_nonfatal_unreached_once();
     }
-    /* No else, we've covered all possible returned value. */
 
     /* See if this is a hostname lookup that we can answer immediately.
      * (For example, an attempt to look up the IP address for an IP address.)
@@ -1585,7 +1611,10 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       }
       tor_assert(!automap);
       rep_hist_note_used_resolve(now); /* help predict this next time */
-    } else if (socks->command == SOCKS_COMMAND_CONNECT) {
+    }
+
+    /* Now see if this is a connect request that we can reject immediately */
+    if (socks->command == SOCKS_COMMAND_CONNECT) {
       /* Special handling for attempts to connect */
       tor_assert(!automap);
       /* Don't allow connections to port 0. */
@@ -1639,7 +1668,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       } /* end "if we should check for internal addresses" */
 
       /* Okay.  We're still doing a CONNECT, and it wasn't a private
-       * address.  Do special handling for literal IP addresses */
+       * address.  Here we do special handling for literal IP addresses,
+       * to see if we should reject this preemptively, and to set up
+       * fields in conn->entry_cfg to tell the exit what AF we want. */
       {
         tor_addr_t addr;
         /* XXX Duplicate call to tor_addr_parse. */
@@ -1682,11 +1713,15 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         }
       }
 
+      /* we never allow IPv6 answers on socks4. (TODO: Is this smart?) */
       if (socks->socks_version == 4)
         conn->entry_cfg.ipv6_traffic = 0;
 
       /* Still handling CONNECT. Now, check for exit enclaves.  (Which we
-       * don't do on BEGINDIR, or there is a chosen exit.)
+       * don't do on BEGINDIR, or when there is a chosen exit.)
+       *
+       * TODO: Should we remove this?  Exit enclaves are nutty and don't
+       * work very well
        */
       if (!conn->use_begindir && !conn->chosen_exit_name && !circ) {
         /* see if we can find a suitable enclave exit */
@@ -1710,7 +1745,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         if (consider_plaintext_ports(conn, socks->port) < 0)
           return -1;
 
-      /* Remember the port so that we do predicted requests there. */
+      /* Remember the port so that we will predict that more requests
+         there will happen in the future. */
       if (!conn->use_begindir) {
         /* help predict this next time */
         rep_hist_note_used_port(now, socks->port);
@@ -1735,6 +1771,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     if (circ) {
       rv = connection_ap_handshake_attach_chosen_circuit(conn, circ, cpath);
     } else {
+      /* We'll try to attach it at the next event loop, or whenever
+       * we call connection_ap_attach_pending() */
       connection_ap_mark_as_pending_circuit(conn);
       rv = 0;
     }
@@ -1811,8 +1849,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     log_info(LD_REND,"Got a hidden service request for ID '%s'",
              safe_str_client(rend_data->onion_address));
 
-    /* Lookup the given onion address. If invalid, stop right now else we
+    /* Lookup the given onion address. If invalid, stop right now.
-     * might have it in the cache or not, it will be tested later on. */
+     * Otherwise, we might have it in the cache or not. */
     unsigned int refetch_desc = 0;
     rend_cache_entry_t *entry = NULL;
     const int rend_cache_lookup_result =
@@ -1826,6 +1864,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
         return -1;
       case ENOENT:
+        /* We didn't have this; we should look it up. */
         refetch_desc = 1;
         break;
       default:
@@ -1835,8 +1874,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       }
     }
 
-    /* Help predict this next time. We're not sure if it will need
+    /* Help predict that we'll want to do hidden service circuits in the
-     * a stable circuit yet, but we know we'll need *something*. */
+     * future. We're not sure if it will need a stable circuit yet, but
+     * we know we'll need *something*. */
     rep_hist_note_used_internal(now, 0, 1);
 
     /* Now we have a descriptor but is it usable or not? If not, refetch.
@@ -1851,9 +1891,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       return 0;
     }
 
-    /* We have the descriptor so launch a connection to the HS. */
+    /* We have the descriptor!  So launch a connection to the HS. */
     base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
     log_info(LD_REND, "Descriptor is here. Great.");
+
+    /* We'll try to attach it at the next event loop, or whenever
+     * we call connection_ap_attach_pending() */
     connection_ap_mark_as_pending_circuit(conn);
     return 0;
   }