Merge remote-tracking branch 'dgoulet/ticket25163_033_01'

changes/ticket25163

@@ -0,0 +1,4 @@
+  o Code simplification and refactoring (rephist):
+    - Remove a series of counters used to track circuit extend attemps and
+      connection status but that in reality we aren't using for anything other
+      than stats logged by a SIGUSR1 signal. Closes ticket 25163.

src/or/channel.c

@@ -1882,7 +1882,6 @@ channel_do_open_actions(channel_t *chan)
 
   if (started_here) {
     circuit_build_times_network_is_live(get_circuit_build_times_mutable());
-    rep_hist_note_connect_succeeded(chan->identity_digest, now);
     router_set_status(chan->identity_digest, 1);
   } else {
     /* only report it to the geoip module if it's not a known router */

src/or/circuitbuild.c

@@ -347,45 +347,6 @@ circuit_log_path(int severity, unsigned int domain, origin_circuit_t *circ)
   tor_free(s);
 }
 
-/** Tell the rep(utation)hist(ory) module about the status of the links
- * in <b>circ</b>.  Hops that have become OPEN are marked as successfully
- * extended; the _first_ hop that isn't open (if any) is marked as
- * unable to extend.
- */
-/* XXXX Someday we should learn from OR circuits too. */
-void
-circuit_rep_hist_note_result(origin_circuit_t *circ)
-{
-  crypt_path_t *hop;
-  const char *prev_digest = NULL;
-  hop = circ->cpath;
-  if (!hop) /* circuit hasn't started building yet. */
-    return;
-  if (server_mode(get_options())) {
-    const routerinfo_t *me = router_get_my_routerinfo();
-    if (!me)
-      return;
-    prev_digest = me->cache_info.identity_digest;
-  }
-  do {
-    const node_t *node = node_get_by_id(hop->extend_info->identity_digest);
-    if (node) { /* Why do we check this?  We know the identity. -NM XXXX */
-      if (prev_digest) {
-        if (hop->state == CPATH_STATE_OPEN)
-          rep_hist_note_extend_succeeded(prev_digest, node->identity);
-        else {
-          rep_hist_note_extend_failed(prev_digest, node->identity);
-          break;
-        }
-      }
-      prev_digest = node->identity;
-    } else {
-      prev_digest = NULL;
-    }
-    hop=hop->next;
-  } while (hop!=circ->cpath);
-}
-
 /** Return 1 iff every node in circ's cpath definitely supports ntor. */
 static int
 circuit_cpath_supports_ntor(const origin_circuit_t *circ)
@@ -1075,7 +1036,6 @@ circuit_build_no_more_hops(origin_circuit_t *circ)
   }
 
   pathbias_count_build_success(circ);
-  circuit_rep_hist_note_result(circ);
   if (is_usable_for_streams)
     circuit_has_opened(circ); /* do other actions as necessary */
 

src/or/circuitbuild.h

@@ -17,7 +17,6 @@ char *circuit_list_path(origin_circuit_t *circ, int verbose);
 char *circuit_list_path_for_controller(origin_circuit_t *circ);
 void circuit_log_path(int severity, unsigned int domain,
                       origin_circuit_t *circ);
-void circuit_rep_hist_note_result(origin_circuit_t *circ);
 origin_circuit_t *origin_circuit_init(uint8_t purpose, int flags);
 origin_circuit_t *circuit_establish_circuit(uint8_t purpose,
                                             extend_info_t *exit,

src/or/circuitlist.c

@@ -1986,8 +1986,7 @@ circuit_mark_all_dirty_circs_as_unusable(void)
  *   - If state is onionskin_pending, remove circ from the onion_pending
  *     list.
  *   - If circ isn't open yet: call circuit_build_failed() if we're
- *     the origin, and in either case call circuit_rep_hist_note_result()
- *     to note stats.
+ *     the origin.
  *   - If purpose is C_INTRODUCE_ACK_WAIT, report the intro point
  *     failure we just had to the hidden service client module.
  *   - If purpose is C_INTRODUCING and <b>reason</b> isn't TIMEOUT,
@@ -2123,7 +2122,6 @@ circuit_about_to_free(circuit_t *circ)
     if (CIRCUIT_IS_ORIGIN(circ)) {
       origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
       circuit_build_failed(ocirc); /* take actions if necessary */
-      circuit_rep_hist_note_result(ocirc);
     }
   }
   if (circ->state == CIRCUIT_STATE_CHAN_WAIT) {

src/or/connection_or.c

@@ -705,7 +705,6 @@ connection_or_finished_connecting(or_connection_t *or_conn)
 void
 connection_or_about_to_close(or_connection_t *or_conn)
 {
-  time_t now = time(NULL);
   connection_t *conn = TO_CONN(or_conn);
 
   /* Tell the controlling channel we're closed */
@@ -725,7 +724,6 @@ connection_or_about_to_close(or_connection_t *or_conn)
     if (connection_or_nonopen_was_started_here(or_conn)) {
       const or_options_t *options = get_options();
       connection_or_note_state_when_broken(or_conn);
-      rep_hist_note_connect_failed(or_conn->identity_digest, now);
       /* Tell the new guard API about the channel failure */
       entry_guard_chan_failed(TLS_CHAN_TO_BASE(or_conn->chan));
       if (conn->state >= OR_CONN_STATE_TLS_HANDSHAKING) {
@@ -741,11 +739,9 @@ connection_or_about_to_close(or_connection_t *or_conn)
   } else if (conn->hold_open_until_flushed) {
     /* We only set hold_open_until_flushed when we're intentionally
      * closing a connection. */
-    rep_hist_note_disconnect(or_conn->identity_digest, now);
     control_event_or_conn_status(or_conn, OR_CONN_EVENT_CLOSED,
                 tls_error_to_orconn_end_reason(or_conn->tls_error));
   } else if (!tor_digest_is_zero(or_conn->identity_digest)) {
-    rep_hist_note_connection_died(or_conn->identity_digest, now);
     control_event_or_conn_status(or_conn, OR_CONN_EVENT_CLOSED,
                 tls_error_to_orconn_end_reason(or_conn->tls_error));
   }

src/or/rephist.c

@@ -110,38 +110,12 @@ uint32_t rephist_total_num=0;
  * 20X as much as one that ended a month ago, and routers that have had no
  * uptime data for about half a year will get forgotten.) */
 
-/** History of an OR-\>OR link. */
-typedef struct link_history_t {
-  /** When did we start tracking this list? */
-  time_t since;
-  /** When did we most recently note a change to this link */
-  time_t changed;
-  /** How many times did extending from OR1 to OR2 succeed? */
-  unsigned long n_extend_ok;
-  /** How many times did extending from OR1 to OR2 fail? */
-  unsigned long n_extend_fail;
-} link_history_t;
-
 /** History of an OR. */
 typedef struct or_history_t {
   /** When did we start tracking this OR? */
   time_t since;
   /** When did we most recently note a change to this OR? */
   time_t changed;
-  /** How many times did we successfully connect? */
-  unsigned long n_conn_ok;
-  /** How many times did we try to connect and fail?*/
-  unsigned long n_conn_fail;
-  /** How many seconds have we been connected to this OR before
-   * 'up_since'? */
-  unsigned long uptime;
-  /** How many seconds have we been unable to connect to this OR before
-   * 'down_since'? */
-  unsigned long downtime;
-  /** If nonzero, we have been connected since this time. */
-  time_t up_since;
-  /** If nonzero, we have been unable to connect since this time. */
-  time_t down_since;
 
   /** The address at which we most recently connected to this OR
    * successfully. */
@@ -163,10 +137,6 @@ typedef struct or_history_t {
   time_t start_of_downtime;
   unsigned long weighted_uptime;
   unsigned long total_weighted_time;
-
-  /** Map from hex OR2 identity digest to a link_history_t for the link
-   * from this OR to OR2. */
-  digestmap_t *link_history_map;
 } or_history_t;
 
 /**
@@ -232,7 +202,6 @@ get_or_history(const char* id)
     hist = tor_malloc_zero(sizeof(or_history_t));
     rephist_total_alloc += sizeof(or_history_t);
     rephist_total_num++;
-    hist->link_history_map = digestmap_new();
     hist->since = hist->changed = time(NULL);
     tor_addr_make_unspec(&hist->last_reached_addr);
     digestmap_set(history_map, id, hist);
@@ -240,66 +209,16 @@ get_or_history(const char* id)
   return hist;
 }
 
-/** Return the link_history_t for the link from the first named OR to
- * the second, creating it if necessary. (ORs are identified by
- * identity digest.)
- */
-static link_history_t *
-get_link_history(const char *from_id, const char *to_id)
-{
-  or_history_t *orhist;
-  link_history_t *lhist;
-  orhist = get_or_history(from_id);
-  if (!orhist)
-    return NULL;
-  if (tor_digest_is_zero(to_id))
-    return NULL;
-  lhist = digestmap_get(orhist->link_history_map, to_id);
-  if (!lhist) {
-    lhist = tor_malloc_zero(sizeof(link_history_t));
-    rephist_total_alloc += sizeof(link_history_t);
-    lhist->since = lhist->changed = time(NULL);
-    digestmap_set(orhist->link_history_map, to_id, lhist);
-  }
-  return lhist;
-}
-
-/** Helper: free storage held by a single link history entry. */
-static void
-free_link_history_(void *val)
-{
-  rephist_total_alloc -= sizeof(link_history_t);
-  tor_free(val);
-}
-
 /** Helper: free storage held by a single OR history entry. */
 static void
 free_or_history(void *_hist)
 {
   or_history_t *hist = _hist;
-  digestmap_free(hist->link_history_map, free_link_history_);
   rephist_total_alloc -= sizeof(or_history_t);
   rephist_total_num--;
   tor_free(hist);
 }
 
-/** Update an or_history_t object <b>hist</b> so that its uptime/downtime
- * count is up-to-date as of <b>when</b>.
- */
-static void
-update_or_history(or_history_t *hist, time_t when)
-{
-  tor_assert(hist);
-  if (hist->up_since) {
-    tor_assert(!hist->down_since);
-    hist->uptime += (when - hist->up_since);
-    hist->up_since = when;
-  } else if (hist->down_since) {
-    hist->downtime += (when - hist->down_since);
-    hist->down_since = when;
-  }
-}
-
 /** Initialize the static data structures for tracking history. */
 void
 rep_hist_init(void)
@@ -309,99 +228,6 @@ rep_hist_init(void)
   predicted_ports_alloc();
 }
 
-/** Helper: note that we are no longer connected to the router with history
- * <b>hist</b>.  If <b>failed</b>, the connection failed; otherwise, it was
- * closed correctly. */
-static void
-mark_or_down(or_history_t *hist, time_t when, int failed)
-{
-  if (hist->up_since) {
-    hist->uptime += (when - hist->up_since);
-    hist->up_since = 0;
-  }
-  if (failed && !hist->down_since) {
-    hist->down_since = when;
-  }
-}
-
-/** Helper: note that we are connected to the router with history
- * <b>hist</b>. */
-static void
-mark_or_up(or_history_t *hist, time_t when)
-{
-  if (hist->down_since) {
-    hist->downtime += (when - hist->down_since);
-    hist->down_since = 0;
-  }
-  if (!hist->up_since) {
-    hist->up_since = when;
-  }
-}
-
-/** Remember that an attempt to connect to the OR with identity digest
- * <b>id</b> failed at <b>when</b>.
- */
-void
-rep_hist_note_connect_failed(const char* id, time_t when)
-{
-  or_history_t *hist;
-  hist = get_or_history(id);
-  if (!hist)
-    return;
-  ++hist->n_conn_fail;
-  mark_or_down(hist, when, 1);
-  hist->changed = when;
-}
-
-/** Remember that an attempt to connect to the OR with identity digest
- * <b>id</b> succeeded at <b>when</b>.
- */
-void
-rep_hist_note_connect_succeeded(const char* id, time_t when)
-{
-  or_history_t *hist;
-  hist = get_or_history(id);
-  if (!hist)
-    return;
-  ++hist->n_conn_ok;
-  mark_or_up(hist, when);
-  hist->changed = when;
-}
-
-/** Remember that we intentionally closed our connection to the OR
- * with identity digest <b>id</b> at <b>when</b>.
- */
-void
-rep_hist_note_disconnect(const char* id, time_t when)
-{
-  or_history_t *hist;
-  hist = get_or_history(id);
-  if (!hist)
-    return;
-  mark_or_down(hist, when, 0);
-  hist->changed = when;
-}
-
-/** Remember that our connection to the OR with identity digest
- * <b>id</b> had an error and stopped working at <b>when</b>.
- */
-void
-rep_hist_note_connection_died(const char* id, time_t when)
-{
-  or_history_t *hist;
-  if (!id) {
-    /* If conn has no identity, it didn't complete its handshake, or something
-     * went wrong.  Ignore it.
-     */
-    return;
-  }
-  hist = get_or_history(id);
-  if (!hist)
-    return;
-  mark_or_down(hist, when, 1);
-  hist->changed = when;
-}
-
 /** We have just decided that this router with identity digest <b>id</b> is
  * reachable, meaning we will give it a "Running" flag for the next while. */
 void
@@ -538,7 +364,6 @@ rep_hist_make_router_pessimal(const char *id, time_t when)
   tor_assert(hist);
 
   rep_hist_note_router_unreachable(id, when);
-  mark_or_down(hist, when, 1);
 
   hist->weighted_run_length = 0;
   hist->weighted_uptime = 0;
@@ -715,57 +540,17 @@ rep_hist_have_measured_enough_stability(void)
   return started_tracking_stability < time(NULL) - 4*60*60;
 }
 
-/** Remember that we successfully extended from the OR with identity
- * digest <b>from_id</b> to the OR with identity digest
- * <b>to_name</b>.
- */
-void
-rep_hist_note_extend_succeeded(const char *from_id, const char *to_id)
-{
-  link_history_t *hist;
-  /* log_fn(LOG_WARN, "EXTEND SUCCEEDED: %s->%s",from_name,to_name); */
-  hist = get_link_history(from_id, to_id);
-  if (!hist)
-    return;
-  ++hist->n_extend_ok;
-  hist->changed = time(NULL);
-}
-
-/** Remember that we tried to extend from the OR with identity digest
- * <b>from_id</b> to the OR with identity digest <b>to_name</b>, but
- * failed.
- */
-void
-rep_hist_note_extend_failed(const char *from_id, const char *to_id)
-{
-  link_history_t *hist;
-  /* log_fn(LOG_WARN, "EXTEND FAILED: %s->%s",from_name,to_name); */
-  hist = get_link_history(from_id, to_id);
-  if (!hist)
-    return;
-  ++hist->n_extend_fail;
-  hist->changed = time(NULL);
-}
-
 /** Log all the reliability data we have remembered, with the chosen
  * severity.
  */
 void
 rep_hist_dump_stats(time_t now, int severity)
 {
-  digestmap_iter_t *lhist_it;
   digestmap_iter_t *orhist_it;
-  const char *name1, *name2, *digest1, *digest2;
+  const char *name1, *digest1;
   char hexdigest1[HEX_DIGEST_LEN+1];
-  char hexdigest2[HEX_DIGEST_LEN+1];
   or_history_t *or_history;
-  link_history_t *link_history;
-  void *or_history_p, *link_history_p;
-  double uptime;
-  char buffer[2048];
-  size_t len;
-  int ret;
-  unsigned long upt, downt;
+  void *or_history_p;
   const node_t *node;
 
   rep_history_clean(now - get_options()->RephistTrackTime);
@@ -785,52 +570,12 @@ rep_hist_dump_stats(time_t now, int severity)
     else
       name1 = "(unknown)";
     base16_encode(hexdigest1, sizeof(hexdigest1), digest1, DIGEST_LEN);
-    update_or_history(or_history, now);
-    upt = or_history->uptime;
-    downt = or_history->downtime;
     s = get_stability(or_history, now);
     stability = (long)s;
-    if (upt+downt) {
-      uptime = ((double)upt) / (upt+downt);
-    } else {
-      uptime=1.0;
-    }
     tor_log(severity, LD_HIST,
-        "OR %s [%s]: %ld/%ld good connections; uptime %ld/%ld sec (%.2f%%); "
-        "wmtbf %lu:%02lu:%02lu",
+        "OR %s [%s]: wmtbf %lu:%02lu:%02lu",
         name1, hexdigest1,
-        or_history->n_conn_ok, or_history->n_conn_fail+or_history->n_conn_ok,
-        upt, upt+downt, uptime*100.0,
         stability/3600, (stability/60)%60, stability%60);
-
-    if (!digestmap_isempty(or_history->link_history_map)) {
-      strlcpy(buffer, "    Extend attempts: ", sizeof(buffer));
-      len = strlen(buffer);
-      for (lhist_it = digestmap_iter_init(or_history->link_history_map);
-           !digestmap_iter_done(lhist_it);
-           lhist_it = digestmap_iter_next(or_history->link_history_map,
-                                          lhist_it)) {
-        digestmap_iter_get(lhist_it, &digest2, &link_history_p);
-        if ((node = node_get_by_id(digest2)) && node_get_nickname(node))
-          name2 = node_get_nickname(node);
-        else
-          name2 = "(unknown)";
-
-        link_history = (link_history_t*) link_history_p;
-
-        base16_encode(hexdigest2, sizeof(hexdigest2), digest2, DIGEST_LEN);
-        ret = tor_snprintf(buffer+len, 2048-len, "%s [%s](%ld/%ld); ",
-                        name2,
-                        hexdigest2,
-                        link_history->n_extend_ok,
-                        link_history->n_extend_ok+link_history->n_extend_fail);
-        if (ret<0)
-          break;
-        else
-          len += ret;
-      }
-      tor_log(severity, LD_HIST, "%s", buffer);
-    }
   }
 }
 
@@ -842,10 +587,9 @@ rep_history_clean(time_t before)
 {
   int authority = authdir_mode(get_options());
   or_history_t *or_history;
-  link_history_t *link_history;
-  void *or_history_p, *link_history_p;
-  digestmap_iter_t *orhist_it, *lhist_it;
-  const char *d1, *d2;
+  void *or_history_p;
+  digestmap_iter_t *orhist_it;
+  const char *d1;
 
   orhist_it = digestmap_iter_init(history_map);
   while (!digestmap_iter_done(orhist_it)) {
@@ -862,19 +606,6 @@ rep_history_clean(time_t before)
       free_or_history(or_history);
       continue;
     }
-    for (lhist_it = digestmap_iter_init(or_history->link_history_map);
-         !digestmap_iter_done(lhist_it); ) {
-      digestmap_iter_get(lhist_it, &d2, &link_history_p);
-      link_history = link_history_p;
-      if (link_history->changed < before) {
-        lhist_it = digestmap_iter_next_rmv(or_history->link_history_map,
-                                           lhist_it);
-        rephist_total_alloc -= sizeof(link_history_t);
-        tor_free(link_history);
-        continue;
-      }
-      lhist_it = digestmap_iter_next(or_history->link_history_map,lhist_it);
-    }
     orhist_it = digestmap_iter_next(history_map, orhist_it);
   }
 }

src/or/rephist.h

@@ -13,13 +13,6 @@
 #define TOR_REPHIST_H
 
 void rep_hist_init(void);
-void rep_hist_note_connect_failed(const char* nickname, time_t when);
-void rep_hist_note_connect_succeeded(const char* nickname, time_t when);
-void rep_hist_note_disconnect(const char* nickname, time_t when);
-void rep_hist_note_connection_died(const char* nickname, time_t when);
-void rep_hist_note_extend_succeeded(const char *from_name,
-                                    const char *to_name);
-void rep_hist_note_extend_failed(const char *from_name, const char *to_name);
 void rep_hist_dump_stats(time_t now, int severity);
 void rep_hist_note_bytes_read(size_t num_bytes, time_t when);
 void rep_hist_note_bytes_written(size_t num_bytes, time_t when);