Merge branch 'bug4862_027_04_squashed'

changes/bug4862

@@ -0,0 +1,8 @@
+  o Major feature (Hidden Service):
+    - Remove the introduction point adaptative algorithm which is leaking
+      popularity by changing the amount of introduction points depending on
+      the amount of traffic the HS sees. With this, we stick to only 3
+      introduction points.
+    - Add the torrc option HiddenServiceNumIntroductionPoints for an
+      operatory to specify a fix amount of introduction points. Maximum
+      value is 10 and default is 3.

doc/tor.1.txt

@@ -2177,6 +2177,10 @@ The following options are used to configure a hidden service.
     only owner is able to read the hidden service directory. (Default: 0)
     Has no effect on Windows.
 
+[[HiddenServiceNumIntroductionPoints]] **HiddenServiceNumIntroductionPoints** __NUM__::
+    Number of introduction points the hidden service will have. You can't
+    have more than 10. (Default: 3)
+
 TESTING NETWORK OPTIONS
 -----------------------
 

src/or/config.c

@@ -288,6 +288,7 @@ static config_var_t option_vars_[] = {
   VAR("HiddenServiceAllowUnknownPorts",LINELIST_S, RendConfigLines, NULL),
   VAR("HiddenServiceMaxStreams",LINELIST_S, RendConfigLines, NULL),
   VAR("HiddenServiceMaxStreamsCloseCircuit",LINELIST_S, RendConfigLines, NULL),
+  VAR("HiddenServiceNumIntroductionPoints", LINELIST_S, RendConfigLines, NULL),
   V(HiddenServiceStatistics,     BOOL,     "0"),
   V(HidServAuth,                 LINELIST, NULL),
   V(CloseHSClientCircuitsImmediatelyOnTimeout, BOOL, "0"),

src/or/directory.c

@@ -23,6 +23,7 @@
 #include "relay.h"
 #include "rendclient.h"
 #include "rendcommon.h"
+#include "rendservice.h"
 #include "rephist.h"
 #include "router.h"
 #include "routerlist.h"
@@ -2196,6 +2197,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
                  "Uploading rendezvous descriptor: finished with status "
                  "200 (%s)", escaped(reason));
         control_event_hs_descriptor_uploaded(conn->identity_digest);
+        rend_service_desc_has_uploaded(conn->rend_data);
         break;
       case 400:
         log_warn(LD_REND,"http status 400 (%s) response from dirserver "

src/or/or.h

@@ -4875,6 +4875,11 @@ typedef struct rend_encoded_v2_service_descriptor_t {
  * XXX023 Should this be configurable? */
 #define INTRO_POINT_LIFETIME_MAX_SECONDS (24*60*60)
 
+/** The maximum number of circuit creation retry we do to an intro point
+ * before giving up. We try to reuse intro point that fails during their
+ * lifetime so this is a hard limit on the amount of time we do that. */
+#define MAX_INTRO_POINT_CIRCUIT_RETRIES 3
+
 /** Introduction point information.  Used both in rend_service_t (on
  * the service side) and in rend_service_descriptor_t (on both the
  * client and service side). */
@@ -4899,11 +4904,6 @@ typedef struct rend_intro_point_t {
    * included in the last HS descriptor we generated. */
   unsigned int listed_in_last_desc : 1;
 
-  /** (Service side only) Flag indicating that
-   * rend_service_note_removing_intro_point has been called for this
-   * intro point. */
-  unsigned int rend_service_note_removing_intro_point_called : 1;
-
   /** (Service side only) A replay cache recording the RSA-encrypted parts
    * of INTRODUCE2 cells this intro point's circuit has received.  This is
    * used to prevent replay attacks. */
@@ -4930,15 +4930,16 @@ typedef struct rend_intro_point_t {
    * point should expire. */
   time_t time_to_expire;
 
-  /** (Service side only) The time at which we decided that this intro
-   * point should start expiring, or -1 if this intro point is not yet
-   * expiring.
-   *
-   * This field also serves as a flag to indicate that we have decided
-   * to expire this intro point, in case intro_point_should_expire_now
-   * flaps (perhaps due to a clock jump; perhaps due to other
-   * weirdness, or even a (present or future) bug). */
-  time_t time_expiring;
+  /** (Service side only) The amount of circuit creation we've made to this
+   * intro point. This is incremented every time we do a circuit relaunch on
+   * this object which is triggered when the circuit dies but the node is
+   * still in the consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give
+   * up on it. */
+  unsigned int circuit_retries;
+
+  /** (Service side only) Set if this intro point has an established circuit
+   * and unset if it doesn't. */
+  unsigned int circuit_established:1;
 } rend_intro_point_t;
 
 #define REND_PROTOCOL_VERSION_BITMASK_WIDTH 16

src/or/rendservice.c

@@ -31,9 +31,12 @@
 #include "routerparse.h"
 #include "routerset.h"
 
+struct rend_service_t;
 static origin_circuit_t *find_intro_circuit(rend_intro_point_t *intro,
                                             const char *pk_digest);
 static rend_intro_point_t *find_intro_point(origin_circuit_t *circ);
+static rend_intro_point_t *find_expiring_intro_point(
+    struct rend_service_t *service, origin_circuit_t *circ);
 
 static extend_info_t *find_rp_for_intro(
     const rend_intro_cell_t *intro,
@@ -42,7 +45,6 @@ static extend_info_t *find_rp_for_intro(
 static int intro_point_accepted_intro_count(rend_intro_point_t *intro);
 static int intro_point_should_expire_now(rend_intro_point_t *intro,
                                          time_t now);
-struct rend_service_t;
 static int rend_service_derive_key_digests(struct rend_service_t *s);
 static int rend_service_load_keys(struct rend_service_t *s);
 static int rend_service_load_auth_keys(struct rend_service_t *s,
@@ -87,8 +89,11 @@ struct rend_service_port_config_s {
 
 /** Try to maintain this many intro points per service by default. */
 #define NUM_INTRO_POINTS_DEFAULT 3
-/** Maintain no more than this many intro points per hidden service. */
+/** Maximum number of intro points per service. */
 #define NUM_INTRO_POINTS_MAX 10
+/** Number of extra intro points we launch if our set of intro nodes is
+ * empty. See proposal 155, section 4. */
+#define NUM_INTRO_POINTS_EXTRA 2
 
 /** If we can't build our intro circuits, don't retry for this long. */
 #define INTRO_CIRC_RETRY_PERIOD (60*5)
@@ -102,10 +107,6 @@ struct rend_service_port_config_s {
  * rendezvous point before giving up? */
 #define MAX_REND_TIMEOUT 30
 
-/** How many seconds should we wait for new HS descriptors to reach
- * our clients before we close an expiring intro point? */
-#define INTRO_POINT_EXPIRATION_GRACE_PERIOD (5*60)
-
 /** Represents a single hidden service running at this OP. */
 typedef struct rend_service_t {
   /* Fields specified in config file */
@@ -126,6 +127,10 @@ typedef struct rend_service_t {
   char pk_digest[DIGEST_LEN]; /**< Hash of permanent hidden-service key. */
   smartlist_t *intro_nodes; /**< List of rend_intro_point_t's we have,
                              * or are trying to establish. */
+  /** List of rend_intro_point_t that are expiring. They are removed once
+   * the new descriptor is successfully uploaded. A node in this list CAN
+   * NOT appear in the intro_nodes list. */
+  smartlist_t *expiring_nodes;
   time_t intro_period_started; /**< Start of the current period to build
                                 * introduction points. */
   int n_intro_circuits_launched; /**< Count of intro circuits we have
@@ -177,17 +182,6 @@ num_rend_services(void)
   return smartlist_len(rend_service_list);
 }
 
-/** Return a string identifying <b>service</b>, suitable for use in a
- * log message.  The result does not need to be freed, but may be
- * overwritten by the next call to this function. */
-static const char *
-rend_service_describe_for_log(rend_service_t *service)
-{
-  /* XXX024 Use this function throughout rendservice.c. */
-  /* XXX024 Return a more useful description? */
-  return safe_str_client(service->service_id);
-}
-
 /** Helper: free storage held by a single service authorized client entry. */
 static void
 rend_authorized_client_free(rend_authorized_client_t *client)
@@ -228,6 +222,11 @@ rend_service_free(rend_service_t *service)
       rend_intro_point_free(intro););
     smartlist_free(service->intro_nodes);
   }
+  if (service->expiring_nodes) {
+    SMARTLIST_FOREACH(service->expiring_nodes, rend_intro_point_t *, intro,
+                      rend_intro_point_free(intro););
+    smartlist_free(service->expiring_nodes);
+  }
 
   rend_service_descriptor_free(service->desc);
   if (service->clients) {
@@ -265,6 +264,7 @@ rend_add_service(rend_service_t *service)
   rend_service_port_config_t *p;
 
   service->intro_nodes = smartlist_new();
+  service->expiring_nodes = smartlist_new();
 
   if (service->max_streams_per_circuit < 0) {
     log_warn(LD_CONFIG, "Hidden service (%s) configured with negative max "
@@ -590,7 +590,22 @@ rend_config_services(const or_options_t *options, int validate_only)
       log_info(LD_CONFIG,
                "HiddenServiceMaxStreamsCloseCircuit=%d for %s",
                (int)service->max_streams_close_circuit, service->directory);
-
+    } else if (!strcasecmp(line->key, "HiddenServiceNumIntroductionPoints")) {
+      service->n_intro_points_wanted =
+        (unsigned int) tor_parse_long(line->value, 10,
+                                      NUM_INTRO_POINTS_DEFAULT,
+                                      NUM_INTRO_POINTS_MAX, &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceNumIntroductionPoints "
+                 "should be between %d and %d, not %s",
+                 NUM_INTRO_POINTS_DEFAULT, NUM_INTRO_POINTS_MAX,
+                 line->value);
+        rend_service_free(service);
+        return -1;
+      }
+      log_info(LD_CONFIG, "HiddenServiceNumIntroductionPoints=%d for %s",
+               service->n_intro_points_wanted, service->directory);
     } else if (!strcasecmp(line->key, "HiddenServiceAuthorizeClient")) {
       /* Parse auth type and comma-separated list of client names and add a
        * rend_authorized_client_t for each client to the service's list
@@ -765,6 +780,8 @@ rend_config_services(const or_options_t *options, int validate_only)
             !strcmp(old->directory, new->directory)) {
           smartlist_add_all(new->intro_nodes, old->intro_nodes);
           smartlist_clear(old->intro_nodes);
+          smartlist_add_all(new->expiring_nodes, old->expiring_nodes);
+          smartlist_clear(old->expiring_nodes);
           smartlist_add(surviving_services, old);
           break;
         }
@@ -956,11 +973,6 @@ rend_service_update_descriptor(rend_service_t *service)
     /* This intro point won't be listed in the descriptor... */
     intro_svc->listed_in_last_desc = 0;
 
-    if (intro_svc->time_expiring != -1) {
-      /* This intro point is expiring.  Don't list it. */
-      continue;
-    }
-
     circ = find_intro_circuit(intro_svc, service->pk_digest);
     if (!circ || circ->base_.purpose != CIRCUIT_PURPOSE_S_INTRO) {
       /* This intro point's circuit isn't finished yet.  Don't list it. */
@@ -1417,107 +1429,6 @@ rend_check_authorization(rend_service_t *service,
   return 1;
 }
 
-/** Called when <b>intro</b> will soon be removed from
- * <b>service</b>'s list of intro points. */
-static void
-rend_service_note_removing_intro_point(rend_service_t *service,
-                                       rend_intro_point_t *intro)
-{
-  time_t now = time(NULL);
-
-  /* Don't process an intro point twice here. */
-  if (intro->rend_service_note_removing_intro_point_called) {
-    return;
-  } else {
-    intro->rend_service_note_removing_intro_point_called = 1;
-  }
-
-  /* Update service->n_intro_points_wanted based on how long intro
-   * lasted and how many introductions it handled. */
-  if (intro->time_published == -1) {
-    /* This intro point was never used.  Don't change
-     * n_intro_points_wanted. */
-  } else {
-
-    /* We want to increase the number of introduction points service
-     * operates if intro was heavily used, or decrease the number of
-     * intro points if intro was lightly used.
-     *
-     * We consider an intro point's target 'usage' to be
-     * maximum of INTRODUCE2 cells divided by
-     * INTRO_POINT_LIFETIME_MIN_SECONDS seconds.  To calculate intro's
-     * fraction of target usage, we divide the amount of INTRODUCE2 cells
-     * that it has handled by the fraction of _LIFETIME_MIN_SECONDS for
-     * which it existed.
-     *
-     * Then we multiply that fraction of desired usage by a fudge
-     * factor of 1.5, to decide how many new introduction points
-     * should ideally replace intro (which is now closed or soon to be
-     * closed).  In theory, assuming that introduction load is
-     * distributed equally across all intro points and ignoring the
-     * fact that different intro points are established and closed at
-     * different times, that number of intro points should bring all
-     * of our intro points exactly to our target usage.
-     *
-     * Then we clamp that number to a number of intro points we might
-     * be willing to replace this intro point with and turn it into an
-     * integer. then we clamp it again to the number of new intro
-     * points we could establish now, then we adjust
-     * service->n_intro_points_wanted and let rend_services_introduce
-     * create the new intro points we want (if any).
-     */
-    const double intro_point_usage =
-      intro_point_accepted_intro_count(intro) /
-      (double)(now - intro->time_published);
-    const double intro_point_target_usage =
-      intro->max_introductions /
-      (double)INTRO_POINT_LIFETIME_MIN_SECONDS;
-    const double fractional_n_intro_points_wanted_to_replace_this_one =
-      (1.5 * (intro_point_usage / intro_point_target_usage));
-    unsigned int n_intro_points_wanted_to_replace_this_one;
-    unsigned int n_intro_points_wanted_now;
-    unsigned int n_intro_points_really_wanted_now;
-    int n_intro_points_really_replacing_this_one;
-
-    if (fractional_n_intro_points_wanted_to_replace_this_one >
-        NUM_INTRO_POINTS_MAX) {
-      n_intro_points_wanted_to_replace_this_one = NUM_INTRO_POINTS_MAX;
-    } else if (fractional_n_intro_points_wanted_to_replace_this_one < 0) {
-      n_intro_points_wanted_to_replace_this_one = 0;
-    } else {
-      n_intro_points_wanted_to_replace_this_one = (unsigned)
-        fractional_n_intro_points_wanted_to_replace_this_one;
-    }
-
-    n_intro_points_wanted_now =
-      service->n_intro_points_wanted +
-      n_intro_points_wanted_to_replace_this_one - 1;
-
-    if (n_intro_points_wanted_now < NUM_INTRO_POINTS_DEFAULT) {
-      /* XXXX This should be NUM_INTRO_POINTS_MIN instead.  Perhaps
-       * another use of NUM_INTRO_POINTS_DEFAULT should be, too. */
-      n_intro_points_really_wanted_now = NUM_INTRO_POINTS_DEFAULT;
-    } else if (n_intro_points_wanted_now > NUM_INTRO_POINTS_MAX) {
-      n_intro_points_really_wanted_now = NUM_INTRO_POINTS_MAX;
-    } else {
-      n_intro_points_really_wanted_now = n_intro_points_wanted_now;
-    }
-
-    n_intro_points_really_replacing_this_one =
-      n_intro_points_really_wanted_now - service->n_intro_points_wanted + 1;
-
-    log_info(LD_REND, "Replacing closing intro point for service %s "
-             "with %d new intro points (wanted %g replacements); "
-             "service will now try to have %u intro points",
-             rend_service_describe_for_log(service),
-             n_intro_points_really_replacing_this_one,
-             fractional_n_intro_points_wanted_to_replace_this_one,
-             n_intro_points_really_wanted_now);
-
-    service->n_intro_points_wanted = n_intro_points_really_wanted_now;
-  }
-}
-
 /******
  * Handle cells
  ******/
@@ -1594,12 +1505,15 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request,
 
   intro_point = find_intro_point(circuit);
   if (intro_point == NULL) {
-    log_warn(LD_BUG,
-             "Internal error: Got an INTRODUCE2 cell on an "
-             "intro circ (for service %s) with no corresponding "
-             "rend_intro_point_t.",
-             escaped(serviceid));
-    goto err;
+    intro_point = find_expiring_intro_point(service, circuit);
+    if (intro_point == NULL) {
+      log_warn(LD_BUG,
+               "Internal error: Got an INTRODUCE2 cell on an "
+               "intro circ (for service %s) with no corresponding "
+               "rend_intro_point_t.",
+               escaped(serviceid));
+      goto err;
+    }
   }
 
   log_info(LD_REND, "Received INTRODUCE2 cell for service %s on circ %u.",
@@ -2779,12 +2693,27 @@ rend_service_launch_establish_intro(rend_service_t *service,
   return 0;
 }
 
-/** Return the number of introduction points that are or have been
- * established for the given service address in <b>query</b>. */
-static int
-count_established_intro_points(const char *query)
+/** Return the number of introduction points that are established for the
+ * given service. */
+static unsigned int
+count_established_intro_points(const rend_service_t *service)
+{
+  unsigned int num = 0;
+
+  SMARTLIST_FOREACH(service->intro_nodes, rend_intro_point_t *, intro,
+    num += intro->circuit_established
+  );
+  return num;
+}
+
+/** Return the number of introduction points that are or are being
+ * established for the given service. This function iterates over all
+ * circuit and count those that are linked to the service and are waiting
+ * for the intro point to respond. */
+static unsigned int
+count_intro_point_circuits(const rend_service_t *service)
 {
-  int num_ipos = 0;
+  unsigned int num_ipos = 0;
   SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) {
     if (!circ->marked_for_close &&
         circ->state == CIRCUIT_STATE_OPEN &&
@@ -2792,7 +2721,8 @@ count_established_intro_points(const char *query)
          circ->purpose == CIRCUIT_PURPOSE_S_INTRO)) {
       origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
       if (oc->rend_data &&
-          !rend_cmp_service_ids(query, oc->rend_data->onion_address))
+          !rend_cmp_service_ids(service->service_id,
+                                oc->rend_data->onion_address))
         num_ipos++;
     }
   }
@@ -2835,10 +2765,21 @@ rend_service_intro_has_opened(origin_circuit_t *circuit)
   }
 
   /* If we already have enough introduction circuits for this service,
-   * redefine this one as a general circuit or close it, depending. */
-  if (count_established_intro_points(serviceid) >
-      (int)service->n_intro_points_wanted) { /* XXX023 remove cast */
+   * redefine this one as a general circuit or close it, depending.
+   * Substract the amount of expiring nodes here since the circuits are
+   * still opened. */
+  if ((count_intro_point_circuits(service) -
+       smartlist_len(service->expiring_nodes)) >
+      service->n_intro_points_wanted) {
     const or_options_t *options = get_options();
+    /* Remove the intro point associated with this circuit, it's being
+     * repurposed or closed thus cleanup memory. */
+    rend_intro_point_t *intro = find_intro_point(circuit);
+    if (intro != NULL) {
+      smartlist_remove(service->intro_nodes, intro);
+      rend_intro_point_free(intro);
+    }
+
     if (options->ExcludeNodes) {
       /* XXXX in some future version, we can test whether the transition is
          allowed or not given the actual nodes in the circuit.  But for now,
@@ -2937,6 +2878,7 @@ rend_service_intro_established(origin_circuit_t *circuit,
                                size_t request_len)
 {
   rend_service_t *service;
+  rend_intro_point_t *intro;
   char serviceid[REND_SERVICE_ID_LEN_BASE32+1];
   (void) request;
   (void) request_len;
@@ -2954,6 +2896,19 @@ rend_service_intro_established(origin_circuit_t *circuit,
              (unsigned)circuit->base_.n_circ_id);
     goto err;
   }
+  /* We've just successfully established a intro circuit to one of our
+   * introduction point, account for it. */
+  intro = find_intro_point(circuit);
+  if (intro == NULL) {
+    log_warn(LD_REND,
+             "Introduction circuit established without a rend_intro_point_t "
+             "object for service %s on circuit %u",
+             safe_str_client(serviceid), (unsigned)circuit->base_.n_circ_id);
+    goto err;
+  }
+  intro->circuit_established = 1;
+  /* We might not have every introduction point ready but at this point we
+   * know that the descriptor needs to be uploaded. */
   service->desc_is_dirty = time(NULL);
   circuit_change_purpose(TO_CIRCUIT(circuit), CIRCUIT_PURPOSE_S_INTRO);
 
@@ -3128,6 +3083,23 @@ find_intro_circuit(rend_intro_point_t *intro, const char *pk_digest)
   return NULL;
 }
 
+/** Return the corresponding introdution point using the circuit <b>circ</b>
+ * found in the <b>service</b>. NULL is returned if not found. */
+static rend_intro_point_t *
+find_expiring_intro_point(rend_service_t *service, origin_circuit_t *circ)
+{
+  tor_assert(service);
+  tor_assert(TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO ||
+             TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_INTRO);
+
+  SMARTLIST_FOREACH(service->intro_nodes, rend_intro_point_t *, intro_point,
+    if (crypto_pk_eq_keys(intro_point->intro_key, circ->intro_key)) {
+      return intro_point;
+  });
+
+  return NULL;
+}
+
 /** Return a pointer to the rend_intro_point_t corresponding to the
  * service-side introduction circuit <b>circ</b>. */
 static rend_intro_point_t *
@@ -3194,6 +3166,7 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc,
       char desc_id_base32[REND_DESC_ID_V2_LEN_BASE32 + 1];
       char *hs_dir_ip;
       const node_t *node;
+      rend_data_t *rend_data;
       hs_dir = smartlist_get(responsible_dirs, j);
       if (smartlist_contains_digest(renddesc->successful_uploads,
                                 hs_dir->identity_digest))
@@ -3209,12 +3182,19 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc,
         continue;
       }
       /* Send publish request. */
-      directory_initiate_command_routerstatus(hs_dir,
-                                              DIR_PURPOSE_UPLOAD_RENDDESC_V2,
-                                              ROUTER_PURPOSE_GENERAL,
-                                              DIRIND_ANONYMOUS, NULL,
-                                              desc->desc_str,
-                                              strlen(desc->desc_str), 0);
+
+      /* We need the service ID to identify which service did the upload
+       * request. Lookup is made in rend_service_desc_has_uploaded(). */
+      rend_data = rend_data_client_create(service_id, desc->desc_id, NULL,
+                                          REND_NO_AUTH);
+      directory_initiate_command_routerstatus_rend(hs_dir,
+                                                   DIR_PURPOSE_UPLOAD_RENDDESC_V2,
+                                                   ROUTER_PURPOSE_GENERAL,
+                                                   DIRIND_ANONYMOUS, NULL,
+                                                   desc->desc_str,
+                                                   strlen(desc->desc_str),
+                                                   0, rend_data);
+      rend_data_free(rend_data);
       base32_encode(desc_id_base32, sizeof(desc_id_base32),
                     desc->desc_id, DIGEST_LEN);
       hs_dir_ip = tor_dup_ip(hs_dir->addr);
@@ -3397,12 +3377,6 @@ intro_point_should_expire_now(rend_intro_point_t *intro,
     return 0;
   }
 
-  if (intro->time_expiring != -1) {
-    /* We've already started expiring this intro point.  *Don't* let
-     * this function's result 'flap'. */
-    return 1;
-  }
-
   if (intro_point_accepted_intro_count(intro) >=
       intro->max_introductions) {
     /* This intro point has been used too many times.  Expire it now. */
@@ -3428,48 +3402,162 @@ intro_point_should_expire_now(rend_intro_point_t *intro,
   return (now >= intro->time_to_expire);
 }
 
+/** Iterate over intro points in the given service and remove the invalid
+ * ones. For an intro point object to be considered invalid, the circuit
+ * _and_ node need to have disappeared.
+ *
+ * If the intro point should expire, it's placed into the expiring_nodes
+ * list of the service and removed from the active intro nodes list.
+ *
+ * If <b>exclude_nodes</b> is not NULL, add the valid nodes to it.
+ *
+ * If <b>retry_nodes</b> is not NULL, add the valid node to it if the
+ * circuit disappeared but the node is still in the consensus. */
+static void
+remove_invalid_intro_points(rend_service_t *service,
+                            smartlist_t *exclude_nodes,
+                            smartlist_t *retry_nodes, time_t now)
+{
+  tor_assert(service);
+
+  SMARTLIST_FOREACH_BEGIN(service->intro_nodes, rend_intro_point_t *,
+                          intro) {
+    /* Find the introduction point node object. */
+    const node_t *node =
+      node_get_by_id(intro->extend_info->identity_digest);
+    /* Find the intro circuit, this might be NULL. */
+    origin_circuit_t *intro_circ =
+      find_intro_circuit(intro, service->pk_digest);
+
+    /* Add the valid node to the exclusion list so we don't try to establish
+     * an introduction point to it again. */
+    if (node && exclude_nodes) {
+      smartlist_add(exclude_nodes, (void*) node);
+    }
+
+    /* First, make sure we still have a valid circuit for this intro point.
+     * If we dont, we'll give up on it and make a new one. */
+    if (intro_circ == NULL) {
+      log_info(LD_REND, "Attempting to retry on %s as intro point for %s"
+               " (circuit disappeared).",
+               safe_str_client(extend_info_describe(intro->extend_info)),
+               safe_str_client(service->service_id));
+      /* We've lost the circuit for this intro point, flag it so it can be
+       * accounted for when considiring uploading a descriptor. */
+      intro->circuit_established = 0;
+
+      /* Node is gone or we've reached our maximum circuit creationg retry
+       * count, clean up everything, we'll find a new one. */
+      if (node == NULL ||
+          intro->circuit_retries >= MAX_INTRO_POINT_CIRCUIT_RETRIES) {
+        rend_intro_point_free(intro);
+        SMARTLIST_DEL_CURRENT(service->intro_nodes, intro);
+        /* We've just killed the intro point, nothing left to do. */
+        continue;
+      }
+
+      /* The intro point is still alive so let's try to use it again because
+       * we have a published descriptor containing it. Keep the intro point
+       * in the intro_nodes list because it's still valid, we are rebuilding
+       * a circuit to it. */
+      if (retry_nodes) {
+        smartlist_add(retry_nodes, intro);
+      }
+    }
+    /* else, the circuit is valid so in both cases, node being alive or not,
+     * we leave the circuit and intro point object as is. Closing the
+     * circuit here would leak new consensus timing and freeing the intro
+     * point object would make the intro circuit unusable. */
+
+    /* Now, check if intro point should expire. If it does, queue it so
+     * it can be cleaned up once it has been replaced properly. */
+    if (intro_point_should_expire_now(intro, now)) {
+      log_info(LD_REND, "Expiring %s as intro point for %s.",
+               safe_str_client(extend_info_describe(intro->extend_info)),
+               safe_str_client(service->service_id));
+      smartlist_add(service->expiring_nodes, intro);
+      SMARTLIST_DEL_CURRENT(service->intro_nodes, intro);
+      /* Intro point is expired, we need a new one thus don't consider it
+       * anymore has a valid established intro point. */
+      intro->circuit_established = 0;
+    }
+  } SMARTLIST_FOREACH_END(intro);
+}
+
+/** A new descriptor has been successfully uploaded for the given
+ * <b>rend_data</b>. Remove and free the expiring nodes from the associated
+ * service. */
+void
+rend_service_desc_has_uploaded(const rend_data_t *rend_data)
+{
+  rend_service_t *service;
+
+  tor_assert(rend_data);
+
+  service = rend_service_get_by_service_id(rend_data->onion_address);
+  if (service == NULL) {
+    log_warn(LD_REND, "Service %s not found after descriptor upload",
+             safe_str_client(rend_data->onion_address));
+    return;
+  }
+
+  SMARTLIST_FOREACH_BEGIN(service->expiring_nodes, rend_intro_point_t *,
+                          intro) {
+    origin_circuit_t *intro_circ =
+      find_intro_circuit(intro, service->pk_digest);
+    if (intro_circ != NULL) {
+      circuit_mark_for_close(TO_CIRCUIT(intro_circ),
+                             END_CIRC_REASON_FINISHED);
+    }
+    SMARTLIST_DEL_CURRENT(service->expiring_nodes, intro);
+    rend_intro_point_free(intro);
+  } SMARTLIST_FOREACH_END(intro);
+}
+
 /** For every service, check how many intro points it currently has, and:
+ *  - Invalidate introdution points based on specific criteria, see
+ *  remove_invalid_intro_points comments.
  *  - Pick new intro points as necessary.
  *  - Launch circuits to any new intro points.
+ *
+ * This is called once a second by the main loop.
  */
 void
 rend_services_introduce(void)
 {
-  int i,j,r;
-  const node_t *node;
-  rend_service_t *service;
-  rend_intro_point_t *intro;
-  int intro_point_set_changed, prev_intro_nodes;
-  unsigned int n_intro_points_unexpired;
-  unsigned int n_intro_points_to_open;
+  int i;
   time_t now;
   const or_options_t *options = get_options();
-  /* List of nodes we need to _exclude_ when choosing a new node to establish
-   * an intro point to. */
+  /* List of nodes we need to _exclude_ when choosing a new node to
+   * establish an intro point to. */
   smartlist_t *exclude_nodes;
+  /* List of nodes we need to retry to build a circuit on them because the
+   * node is valid but circuit died. */
+  smartlist_t *retry_nodes;
 
   if (!have_completed_a_circuit())
     return;
 
   exclude_nodes = smartlist_new();
+  retry_nodes = smartlist_new();
   now = time(NULL);
 
-  for (i=0; i < smartlist_len(rend_service_list); ++i) {
+  SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, service) {
+    int r;
+    /* Number of intro points we want to open and add to the intro nodes
+     * list of the service. */
+    unsigned int n_intro_points_to_open;
+    /* Have an unsigned len so we can use it to compare values else gcc is
+     * not happy with unmatching signed comparaison. */
+    unsigned int intro_nodes_len;
+    /* Different service are allowed to have the same introduction point as
+     * long as they are on different circuit thus why we clear this list. */
     smartlist_clear(exclude_nodes);
-    service = smartlist_get(rend_service_list, i);
-
-    tor_assert(service);
-
-    /* intro_point_set_changed becomes non-zero iff the set of intro
-     * points to be published in service's descriptor has changed. */
-    intro_point_set_changed = 0;
-
-    /* n_intro_points_unexpired collects the number of non-expiring
-     * intro points we have, so that we know how many new intro
-     * circuits we need to launch for this service. */
-    n_intro_points_unexpired = 0;
+    smartlist_clear(retry_nodes);
 
-    if (now > service->intro_period_started+INTRO_CIRC_RETRY_PERIOD) {
+    /* This retry period is important here so we don't stress circuit
+     * creation. */
+    if (now > service->intro_period_started + INTRO_CIRC_RETRY_PERIOD) {
       /* One period has elapsed; we can try building circuits again. */
       service->intro_period_started = now;
       service->n_intro_circuits_launched = 0;
@@ -3480,116 +3568,58 @@ rend_services_introduce(void)
       continue;
     }
 
-    /* Find out which introduction points we have in progress for this
-       service. */
-    SMARTLIST_FOREACH_BEGIN(service->intro_nodes, rend_intro_point_t *,
-                            intro) {
-      origin_circuit_t *intro_circ =
-        find_intro_circuit(intro, service->pk_digest);
-
-      if (intro->time_expiring + INTRO_POINT_EXPIRATION_GRACE_PERIOD > now) {
-        /* This intro point has completely expired.  Remove it, and
-         * mark the circuit for close if it's still alive. */
-        if (intro_circ != NULL &&
-            intro_circ->base_.purpose != CIRCUIT_PURPOSE_PATH_BIAS_TESTING) {
-          circuit_mark_for_close(TO_CIRCUIT(intro_circ),
-                                 END_CIRC_REASON_FINISHED);
-        }
-        rend_intro_point_free(intro);
-        intro = NULL; /* SMARTLIST_DEL_CURRENT takes a name, not a value. */
-        SMARTLIST_DEL_CURRENT(service->intro_nodes, intro);
-        /* We don't need to set intro_point_set_changed here, because
-         * this intro point wouldn't have been published in a current
-         * descriptor anyway. */
-        continue;
-      }
+    /* Cleanup the invalid intro points and save the node objects, if apply,
+     * in the exclude_nodes and retry_nodes list. */
+    remove_invalid_intro_points(service, exclude_nodes, retry_nodes, now);
 
-      node = node_get_by_id(intro->extend_info->identity_digest);
-      if (!node || !intro_circ) {
-        int removing_this_intro_point_changes_the_intro_point_set = 1;
-        log_info(LD_REND, "Giving up on %s as intro point for %s"
-                 " (circuit disappeared).",
+    /* Let's try to rebuild circuit on the nodes we want to retry on. */
+    SMARTLIST_FOREACH_BEGIN(retry_nodes, rend_intro_point_t *, intro) {
+      r = rend_service_launch_establish_intro(service, intro);
+      if (r < 0) {
+        log_warn(LD_REND, "Error launching circuit to node %s for service %s.",
                  safe_str_client(extend_info_describe(intro->extend_info)),
                  safe_str_client(service->service_id));
-        rend_service_note_removing_intro_point(service, intro);
-        if (intro->time_expiring != -1) {
-          log_info(LD_REND, "We were already expiring the intro point; "
-                   "no need to mark the HS descriptor as dirty over this.");
-          removing_this_intro_point_changes_the_intro_point_set = 0;
-        } else if (intro->listed_in_last_desc) {
-          log_info(LD_REND, "The intro point we are giving up on was "
-                   "included in the last published descriptor. "
-                   "Marking current descriptor as dirty.");
-          service->desc_is_dirty = now;
-        }
+        /* Unable to launch a circuit to that intro point, remove it from
+         * the valid list so we can create a new one. */
+        smartlist_remove(service->intro_nodes, intro);
         rend_intro_point_free(intro);
-        intro = NULL; /* SMARTLIST_DEL_CURRENT takes a name, not a value. */
-        SMARTLIST_DEL_CURRENT(service->intro_nodes, intro);
-        if (removing_this_intro_point_changes_the_intro_point_set)
-          intro_point_set_changed = 1;
-      }
-
-      if (intro != NULL && intro_point_should_expire_now(intro, now)) {
-        log_info(LD_REND, "Expiring %s as intro point for %s.",
-                 safe_str_client(extend_info_describe(intro->extend_info)),
-                 safe_str_client(service->service_id));
-
-        rend_service_note_removing_intro_point(service, intro);
-
-        /* The polite (and generally Right) way to expire an intro
-         * point is to establish a new one to replace it, publish a
-         * new descriptor that doesn't list any expiring intro points,
-         * and *then*, once our upload attempts for the new descriptor
-         * have ended (whether in success or failure), close the
-         * expiring intro points.
-         *
-         * Unfortunately, we can't find out when the new descriptor
-         * has actually been uploaded, so we'll have to settle for a
-         * five-minute timer.  Start it.  XXXX024 This sucks. */
-        intro->time_expiring = now;
-
-        intro_point_set_changed = 1;
+        continue;
       }
-
-      if (intro != NULL && intro->time_expiring == -1)
-        ++n_intro_points_unexpired;
-
-      /* Add the valid node to the exclusion list so we don't try to establish
-       * an introduction point to it again. */
-      if (node)
-        smartlist_add(exclude_nodes, (void*)node);
+      intro->circuit_retries++;
     } SMARTLIST_FOREACH_END(intro);
 
-    if (!intro_point_set_changed &&
-        (n_intro_points_unexpired >= service->n_intro_points_wanted)) {
+    /* Avoid mismatched signed comparaison below. */
+    intro_nodes_len = (unsigned int) smartlist_len(service->intro_nodes);
+
+    /* Quiescent state, no node expiring and we have more or the amount of
+     * wanted node for this service. Proceed to the next service. Could be
+     * more because we launch two preemptive circuits if our intro nodes
+     * list is empty. */
+    if (smartlist_len(service->expiring_nodes) == 0 &&
+        intro_nodes_len >= service->n_intro_points_wanted) {
       continue;
     }
 
-    /* Remember how many introduction circuits we started with.
-     *
-     * prev_intro_nodes serves a different purpose than
-     * n_intro_points_unexpired -- this variable tells us where our
-     * previously-created intro points end and our new ones begin in
-     * the intro-point list, so we don't have to launch the circuits
-     * at the same time as we create the intro points they correspond
-     * to.  XXXX This is daft. */
-    prev_intro_nodes = smartlist_len(service->intro_nodes);
-
-    /* We have enough directory information to start establishing our
-     * intro points. We want to end up with n_intro_points_wanted
-     * intro points, but if we're just starting, we launch two extra
-     * circuits and use the first n_intro_points_wanted that complete.
-     *
-     * The ones after the first three will be converted to 'general'
-     * internal circuits in rend_service_intro_has_opened(), and then
-     * we'll drop them from the list of intro points next time we
-     * go through the above "find out which introduction points we have
-     * in progress" loop. */
-    n_intro_points_to_open = (service->n_intro_points_wanted +
-                              (prev_intro_nodes == 0 ? 2 : 0));
-    for (j = (int)n_intro_points_unexpired;
-         j < (int)n_intro_points_to_open;
-         ++j) { /* XXXX remove casts */
+    /* Number of intro points we want to open which is the wanted amount
+     * minus the current amount of valid nodes. */
+    n_intro_points_to_open = service->n_intro_points_wanted - intro_nodes_len;
+    if (intro_nodes_len == 0) {
+      /* We want to end up with n_intro_points_wanted intro points, but if
+       * we have no intro points at all (chances are they all cycled or we
+       * are starting up), we launch NUM_INTRO_POINTS_EXTRA extra circuits
+       * and use the first n_intro_points_wanted that complete. See proposal
+       * #155, section 4 for the rationale of this which is purely for
+       * performance.
+       *
+       * The ones after the first n_intro_points_to_open will be converted
+       * to 'general' internal circuits in rend_service_intro_has_opened(),
+       * and then we'll drop them from the list of intro points. */
+      n_intro_points_to_open += NUM_INTRO_POINTS_EXTRA;
+    }
+
+    for (i = 0; i < (int) n_intro_points_to_open; i++) {
+      const node_t *node;
+      rend_intro_point_t *intro;
       router_crn_flags_t flags = CRN_NEED_UPTIME|CRN_NEED_DESC;
       if (get_options()->AllowInvalid_ & ALLOW_INVALID_INTRODUCTION)
         flags |= CRN_ALLOW_INVALID;
@@ -3597,16 +3627,15 @@ rend_services_introduce(void)
                                        options->ExcludeNodes, flags);
       if (!node) {
         log_warn(LD_REND,
-                 "Could only establish %d introduction points for %s; "
+                 "We only have %d introduction points established for %s; "
                  "wanted %u.",
                  smartlist_len(service->intro_nodes),
                  safe_str_client(service->service_id),
                  n_intro_points_to_open);
         break;
       }
-      intro_point_set_changed = 1;
-      /* Add the choosen node to the exclusion list in order to avoid to pick
-       * it again in the next iteration. */
+      /* Add the choosen node to the exclusion list in order to avoid to
+       * pick it again in the next iteration. */
       smartlist_add(exclude_nodes, (void*)node);
       intro = tor_malloc_zero(sizeof(rend_intro_point_t));
       intro->extend_info = extend_info_from_node(node, 0);
@@ -3615,7 +3644,6 @@ rend_services_introduce(void)
       tor_assert(!fail);
       intro->time_published = -1;
       intro->time_to_expire = -1;
-      intro->time_expiring = -1;
       intro->max_introductions =
         crypto_rand_int_range(INTRO_POINT_MIN_LIFETIME_INTRODUCTIONS,
                               INTRO_POINT_MAX_LIFETIME_INTRODUCTIONS);
@@ -3623,24 +3651,20 @@ rend_services_introduce(void)
       log_info(LD_REND, "Picked router %s as an intro point for %s.",
                safe_str_client(node_describe(node)),
                safe_str_client(service->service_id));
-    }
-
-    /* If there's no need to launch new circuits, stop here. */
-    if (!intro_point_set_changed)
-      continue;
-
-    /* Establish new introduction points. */
-    for (j=prev_intro_nodes; j < smartlist_len(service->intro_nodes); ++j) {
-      intro = smartlist_get(service->intro_nodes, j);
+      /* Establish new introduction circuit to our chosen intro point. */
       r = rend_service_launch_establish_intro(service, intro);
-      if (r<0) {
+      if (r < 0) {
         log_warn(LD_REND, "Error launching circuit to node %s for service %s.",
                  safe_str_client(extend_info_describe(intro->extend_info)),
                  safe_str_client(service->service_id));
+        /* This funcion will be called again by the main loop so this intro
+         * point without a intro circuit will be retried on or removed after
+         * a maximum number of attempts. */
       }
     }
-  }
+  } SMARTLIST_FOREACH_END(service);
   smartlist_free(exclude_nodes);
+  smartlist_free(retry_nodes);
 }
 
 #define MIN_REND_INITIAL_POST_DELAY (30)
@@ -3675,9 +3699,13 @@ rend_consider_services_upload(time_t now)
       service->next_upload_time =
         now + rendinitialpostdelay + crypto_rand_int(2*rendpostperiod);
     }
-    if (service->next_upload_time < now ||
+    /* Does every introduction points have been established? */
+    unsigned int intro_points_ready =
+      count_established_intro_points(service) >= service->n_intro_points_wanted;
+    if (intro_points_ready &&
+        (service->next_upload_time < now ||
         (service->desc_is_dirty &&
-         service->desc_is_dirty < now-rendinitialpostdelay)) {
+         service->desc_is_dirty < now-rendinitialpostdelay))) {
       /* if it's time, or if the directory servers have a wrong service
        * descriptor and ours has been stable for rendinitialpostdelay seconds,
        * upload a new one of each format. */

src/or/rendservice.h

@@ -125,6 +125,7 @@ int rend_service_del_ephemeral(const char *service_id);
 void directory_post_to_hs_dir(rend_service_descriptor_t *renddesc,
                               smartlist_t *descs, smartlist_t *hs_dirs,
                               const char *service_id, int seconds_valid);
+void rend_service_desc_has_uploaded(const rend_data_t *rend_data);
 
 #endif