Routers count as down when they change ORPort, too

rransom noticed that a change of ORPort is just as bad as a change of IP
address from a client's perspective, because both mean that the relay is
not available to them while the new information hasn't propagated.
Change the bug1035 fix accordingly.

Also make sure we don't log a bridge's IP address (which might happen
when we are the bridge authority).

4 files changed, 27 insertions, 17 deletions

diff --git a/changes/bug1035 b/changes/bug1035
index 041e3b3bb6..3d86330e63 100644
--- a/changes/bug1035
+++ b/changes/bug1035
@@ -1,9 +1,10 @@
   o Minor features (authorities)
-    - Take altered router IPs into account when determining router stability.
-      Previously, if a router changed its IP, the authorities would not
-      treat it as having any downtime for the purposes of stability
-      calculation, whereas clients would experience downtime since the
-      IP could take a while to propagate to them.  Resolves issue 1035.
+    - Take altered router IP addresses and ORPorts into account when
+      determining router stability.  Previously, if a router changed
+      its IP or ORPort, the authorities would not treat it as having
+      any downtime for the purposes of stability calculation, whereas
+      clients would experience downtime since the change could take a
+      while to propagate to them.  Resolves issue 1035.
   o Minor bugfixes (authorities)
     - Try to be more robust to hops back in time when calculating
       router stability.  Previously, if a run of uptime or downtime
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 52e59cd9d1..f426881440 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -3118,11 +3118,13 @@ dirserv_orconn_tls_done(const char *address,
       /* correct digest. mark this router reachable! */
       if (!bridge_auth || ri->purpose == ROUTER_PURPOSE_BRIDGE) {
         tor_addr_t addr, *addrp=NULL;
-        log_info(LD_DIRSERV, "Found router %s to be reachable at %s. Yay.",
-                 ri->nickname, address);
+        log_info(LD_DIRSERV, "Found router %s to be reachable at %s:%d. Yay.",
+                 ri->nickname, address, ri->or_port );
         if (tor_addr_from_str(&addr, ri->address) != -1)
           addrp = &addr;
-        rep_hist_note_router_reachable(digest_rcvd, addrp, now);
+        else
+          log_warn(LD_BUG, "Couldn't parse IP address \"%s\"", ri->address);
+        rep_hist_note_router_reachable(digest_rcvd, addrp, or_port, now);
         ri->last_reachable = now;
       }
     }
diff --git a/src/or/rephist.c b/src/or/rephist.c
index 28699901fa..7c570e26a6 100644
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -78,6 +78,9 @@ typedef struct or_history_t {
    * successfully. */
   tor_addr_t last_reached_addr;
 
+  /** The port at which we most recently connected to this OR successfully */
+  uint16_t last_reached_port;
+
   /* === For MTBF tracking: */
   /** Weighted sum total of all times that this router has been online.
    */
@@ -296,17 +299,18 @@ rep_hist_note_connection_died(const char* id, time_t when)
  * reachable, meaning we will give it a "Running" flag for the next while. */
 void
 rep_hist_note_router_reachable(const char *id, const tor_addr_t *at_addr,
-                               time_t when)
+                               const uint16_t at_port, time_t when)
 {
   or_history_t *hist = get_or_history(id);
   int was_in_run = 1;
   char tbuf[ISO_TIME_LEN+1];
-  int addr_changed;
+  int addr_changed, port_changed;
 
   tor_assert(hist);
 
   addr_changed = at_addr &&
     tor_addr_compare(at_addr, &hist->last_reached_addr, CMP_EXACT) != 0;
+  port_changed = at_port && at_port != hist->last_reached_port;
 
   if (!started_tracking_stability)
     started_tracking_stability = time(NULL);
@@ -326,7 +330,7 @@ rep_hist_note_router_reachable(const char *id, const tor_addr_t *at_addr,
     down_length = when - hist->start_of_downtime;
     hist->total_weighted_time += down_length;
     hist->start_of_downtime = 0;
-  } else if (addr_changed) {
+  } else if (addr_changed || port_changed) {
     /* If we're reachable, but the address changed, treat this as some
      * downtime. */
     int penalty = get_options()->TestingTorNetwork ? 240 : 3600;
@@ -342,12 +346,13 @@ rep_hist_note_router_reachable(const char *id, const tor_addr_t *at_addr,
       penalty = (int)(fresh_interval + live_interval) / 2;
     }
     format_local_iso_time(tbuf, hist->start_of_run);
-    log_info(LD_HIST,"Router %s still seems Running, but its address appears "
-             "to have changed since the last time it was reachable.  I'm "
-             "going to treat it as having been down for %d seconds",
-             hex_str(id, DIGEST_LEN), penalty);
+    if (!authdir_mode_bridge(get_options()))
+      log_info(LD_HIST,"Router %s still seems Running, but its address appears "
+               "to have changed since the last time it was reachable.  I'm "
+               "going to treat it as having been down for %d seconds",
+               hex_str(id, DIGEST_LEN), penalty);
     rep_hist_note_router_unreachable(id, when-penalty);
-    rep_hist_note_router_reachable(id, NULL, when);
+    rep_hist_note_router_reachable(id, NULL, 0, when);
   } else {
     format_local_iso_time(tbuf, hist->start_of_run);
     if (was_in_run)
@@ -359,6 +364,8 @@ rep_hist_note_router_reachable(const char *id, const tor_addr_t *at_addr,
   }
   if (at_addr)
     tor_addr_copy(&hist->last_reached_addr, at_addr);
+  if (at_port)
+    hist->last_reached_port = at_port;
 }
 
 /** We have just decided that this router is unreachable, meaning
diff --git a/src/or/rephist.h b/src/or/rephist.h
index 1ddbac4dad..610c1a0704 100644
--- a/src/or/rephist.h
+++ b/src/or/rephist.h
@@ -34,7 +34,7 @@ int rep_hist_load_state(or_state_t *state, char **err);
 void rep_history_clean(time_t before);
 
 void rep_hist_note_router_reachable(const char *id, const tor_addr_t *at_addr,
-                                    time_t when);
+                                    uint16_t at_port, time_t when);
 void rep_hist_note_router_unreachable(const char *id, time_t when);
 int rep_hist_record_mtbf_data(time_t now, int missing_means_down);
 int rep_hist_load_mtbf_data(time_t now);