Merge remote branch 'mikeperry/bug1952-merge' into maint-0.2.2

changes/bug1952

@@ -0,0 +1,5 @@
+  o Major bugfixes:
+    - Alter how consensus bandwidth-weights are computed using new constraints
+      that should succeed in all cases. Also alter directory authorities to not
+      include the bandwidth-weights line if they fail to produce valid values.
+      Fixes bug 1952; bugfix on 0.2.2.10-alpha.

doc/spec/dir-spec.txt

@@ -1632,6 +1632,7 @@
      "7" -- Provides keyword=integer pairs of consensus parameters
      "8" -- Provides microdescriptor summaries
      "9" -- Provides weights for selecting flagged routers in paths
+     "10" -- Fixes edge case bugs in router flag selection weights
 
    Before generating a consensus, an authority must decide which consensus
    method to use.  To do this, it looks for the highest version number
@@ -1694,22 +1695,25 @@
       Wme*E + Wee*E == E                          (aka: Wee = 1-Wme)
 
   We are short 2 constraints with the above set. The remaining constraints
-  come from examining different cases of network load.
+  come from examining different cases of network load. The following
+  constraints are used in consensus method 10 and above. There are another
+  incorrect and obsolete set of constraints used for these same cases in
+  consensus method 9. For those, see dir-spec.txt in Tor 0.2.2.10-alpha
+  to 0.2.2.16-alpha.
 
   Case 1: E >= T/3 && G >= T/3 (Neither Exit nor Guard Scarce)
 
-    In this case, the additional two constraints are: Wme*E == Wmd*D and
-    Wgd == 0, which maximizes Exit-flagged bandwidth in the middle position.
+    In this case, the additional two constraints are: Wmg == Wmd,
+    Wed == 1/3.
 
     This leads to the solution:
-
-       Wgg = (weight_scale*(D+E+G+M))/(3*G)
-       Wmd = (weight_scale*(2*D + 2*E - G - M))/(6*D)
-       Wme = (weight_scale*(2*D + 2*E - G - M))/(6*E)
-       Wee = (weight_scale*(-2*D + 4*E + G + M))/(6*E)
-       Wmg = weight_scale - Wgg
-       Wed = weight_scale - Wmd
-       Wgd = 0
+        Wgd = weight_scale/3
+        Wed = weight_scale/3
+        Wmd = weight_scale/3
+        Wee = (weight_scale*(E+G+M))/(3*E)
+        Wme = weight_scale - Wee
+        Wmg = (weight_scale*(2*G-E-M))/(3*G)
+        Wgg = weight_scale - Wmg
 
   Case 2: E < T/3 && G < T/3 (Both are scarce)
 
@@ -1733,25 +1737,35 @@
     Subcase b: R+D >= S
 
       In this case, if M <= T/3, we have enough bandwidth to try to achieve
-      a balancing condition, and add the constraints Wgg == 1 and
-      Wme*E == Wmd*D:
+      a balancing condition.
 
-         Wgg = weight_scale
-         Wgd = (weight_scale*(D + E - 2*G + M))/(3*D)      (T/3 >= G (Ok))
-         Wmd = (weight_scale*(D + E + G - 2*M))/(6*D)      (T/3 >= M)
-         Wme = (weight_scale*(D + E + G - 2*M))/(6*E)
-         Wee = (weight_scale*(-D + 5*E - G + 2*M))/(6*E)   (2E+M >= T/3)
-         Wmg = 0;
-         Wed = weight_scale - Wgd - Wmd
+      Add constraints Wgg = 1, Wmd == Wgd to maximize bandwidth in the guard
+      position while still allowing exits to be used as middle nodes:
 
-      If M >= T/3, the above solution will not be valid (one of the weights
-      will be < 0 or > 1). In this case, we use:
+        Wee = (weight_scale*(E - G + M))/E
+        Wed = (weight_scale*(D - 2*E + 4*G - 2*M))/(3*D)
+        Wme = (weight_scale*(G-M))/E
+        Wmg = 0
+        Wgg = weight_scale
+        Wmd = (weight_scale - Wed)/2
+        Wgd = (weight_scale - Wed)/2
+
+      If this system ends up with any values out of range (ie negative, or
+      above weight_scale), use the constraints Wgg == 1 and Wee == 1, since
+      both those positions are scarce:
 
          Wgg = weight_scale
          Wee = weight_scale
-         Wmg = Wme = Wmd = 0
-         Wgd = (weight_scale*(D+E-G))/(2*D)
-         Wed = weight_scale - Wgd
+         Wed = (weight_scale*(D - 2*E + G + M))/(3*D)
+         Wmd = (weight_Scale*(D - 2*M + G + E))/(3*D)
+         Wme = 0
+         Wmg = 0
+         Wgd = weight_scale - Wed - Wmd
+
+      If M > T/3, then the Wmd weight above will become negative. Set it to 0
+      in this case:
+         Wmd = 0
+         Wgd = weight_scale - Wed
 
   Case 3: One of E < T/3 or G < T/3
 
@@ -1759,36 +1773,44 @@
 
     Subcase a: (S+D) < T/3:
       if G=S:
-          Wgg = Wgd = weight_scale;
-          Wmd = Wed = Wmg = 0;
-          Wme = (weight_scale*(E-M))/(2*E);
-          Wee = weight_scale-Wme;
+        Wgg = Wgd = weight_scale;
+        Wmd = Wed = Wmg = 0;
+        // Minor subcase, if E is more scarce than M,
+        // keep its bandwidth in place.
+        if (E < M) Wme = 0;
+        else Wme = (weight_scale*(E-M))/(2*E);
+        Wee = weight_scale-Wme;
       if E=S:
-          Wee = Wed = weight_scale;
-          Wmd = Wgd = Wmg = 0;
-          Wmg = (weight_scale*(G-M))/(2*G);
-          Wgg = weight_scale-Wmg;
+        Wee = Wed = weight_scale;
+        Wmd = Wgd = Wme = 0;
+        // Minor subcase, if G is more scarce than M,
+        // keep its bandwidth in place.
+        if (G < M) Wmg = 0;
+        else Wmg = (weight_scale*(G-M))/(2*G);
+        Wgg = weight_scale-Wmg;
 
     Subcase b: (S+D) >= T/3
       if G=S:
-        Add constraints Wmg = 0, Wme*E == Wmd*D to maximize exit bandwidth
-        in the middle position:
-          Wgd = (weight_scale*(D + E - 2*G + M))/(3*D);
-          Wmd = (weight_scale*(D + E + G - 2*M))/(6*D);
-          Wme = (weight_scale*(D + E + G - 2*M))/(6*E);
-          Wee = (weight_scale*(-D + 5*E - G + 2*M))/(6*E);
-          Wgg = weight_scale;
-          Wmg = 0;
-          Wed = weight_scale - Wgd - Wmd;
+        Add constraints Wgg = 1, Wmd == Wed to maximize bandwidth
+        in the guard position, while still allowing exits to be
+        used as middle nodes:
+          Wgg = weight_scale
+          Wgd = (weight_scale*(D - 2*G + E + M))/(3*D)
+          Wmg = 0
+          Wee = (weight_scale*(E+M))/(2*E)
+          Wme = weight_scale - Wee
+          Wmd = (weight_scale - Wgd)/2
+          Wed = (weight_scale - Wgd)/2
       if E=S:
-        Add constraints Wgd = 0, Wme*E == Wmd*D:
-          Wgg = (weight_scale*(D + E + G + M))/(3*G);
-          Wmd = (weight_scale*(2*D + 2*E - G - M))/(6*D);
-          Wme = (weight_scale*(2*D + 2*E - G - M))/(6*E);
-          Wee = (weight_scale*(-2*D + 4*E + G + M))/(6*E);
-          Wgd = 0;
+        Add constraints Wee == 1, Wmd == Wgd to maximize bandwidth
+        in the exit position:
+          Wee = weight_scale;
+          Wed = (weight_scale*(D - 2*E + G + M))/(3*D);
+          Wme = 0;
+          Wgg = (weight_scale*(G+M))/(2*G);
           Wmg = weight_scale - Wgg;
-          Wed = weight_scale - Wmd;
+          Wmd = (weight_scale - Wed)/2;
+          Wgd = (weight_scale - Wed)/2;
 
   To ensure consensus, all calculations are performed using integer math
   with a fixed precision determined by the bwweightscale consensus

src/or/dirvote.c

@@ -50,7 +50,7 @@ static int dirvote_publish_consensus(void);
 static char *make_consensus_method_list(int low, int high, const char *sep);
 
 /** The highest consensus method that we currently support. */
-#define MAX_SUPPORTED_CONSENSUS_METHOD 9
+#define MAX_SUPPORTED_CONSENSUS_METHOD 10
 
 /** Lowest consensus method that contains a 'directory-footer' marker */
 #define MIN_METHOD_FOR_FOOTER 9
@@ -766,15 +766,275 @@ networkstatus_check_weights(int64_t Wgg, int64_t Wgd, int64_t Wmg,
   if (berr) {
     log_info(LD_DIR,
              "Bw weight mismatch %d. G="I64_FORMAT" M="I64_FORMAT
-             " E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT,
+             " E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT
+             " Wmd=%d Wme=%d Wmg=%d Wed=%d Wee=%d"
+             " Wgd=%d Wgg=%d Wme=%d Wmg=%d",
              berr,
              I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
-             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T));
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T),
+             (int)Wmd, (int)Wme, (int)Wmg, (int)Wed, (int)Wee,
+             (int)Wgd, (int)Wgg, (int)Wme, (int)Wmg);
   }
 
   return berr;
 }
 
+/**
+ * This function computes the bandwidth weights for consensus method 10.
+ *
+ * It returns true if weights could be computed, false otherwise.
+ */
+static int
+networkstatus_compute_bw_weights_v10(smartlist_t *chunks, int64_t G,
+                                     int64_t M, int64_t E, int64_t D,
+                                     int64_t T, int64_t weight_scale)
+{
+  bw_weights_error_t berr = 0;
+  int64_t Wgg = -1, Wgd = -1;
+  int64_t Wmg = -1, Wme = -1, Wmd = -1;
+  int64_t Wed = -1, Wee = -1;
+  const char *casename;
+  char buf[512];
+  int r;
+
+  if (G <= 0 || M <= 0 || E <= 0 || D <= 0) {
+    log_warn(LD_DIR, "Consensus with empty bandwidth: "
+                     "G="I64_FORMAT" M="I64_FORMAT" E="I64_FORMAT
+                     " D="I64_FORMAT" T="I64_FORMAT,
+             I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T));
+    return 0;
+  }
+
+  /*
+   * Computed from cases in 3.4.3 of dir-spec.txt
+   *
+   * 1. Neither are scarce
+   * 2. Both Guard and Exit are scarce
+   *    a. R+D <= S
+   *    b. R+D > S
+   * 3. One of Guard or Exit is scarce
+   *    a. S+D < T/3
+   *    b. S+D >= T/3
+   */
+  if (3*E >= T && 3*G >= T) { // E >= T/3 && G >= T/3
+    /* Case 1: Neither are scarce.  */
+    casename = "Case 1 (Wgd=Wmd=Wed)";
+    Wgd = weight_scale/3;
+    Wed = weight_scale/3;
+    Wmd = weight_scale/3;
+    Wee = (weight_scale*(E+G+M))/(3*E);
+    Wme = weight_scale - Wee;
+    Wmg = (weight_scale*(2*G-E-M))/(3*G);
+    Wgg = weight_scale - Wmg;
+
+    berr = networkstatus_check_weights(Wgg, Wgd, Wmg, Wme, Wmd, Wee, Wed,
+                                       weight_scale, G, M, E, D, T, 10, 1);
+
+    if (berr) {
+      log_warn(LD_DIR,
+             "Bw Weights error %d for %s v10. G="I64_FORMAT" M="I64_FORMAT
+             " E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT
+             " Wmd=%d Wme=%d Wmg=%d Wed=%d Wee=%d"
+             " Wgd=%d Wgg=%d Wme=%d Wmg=%d weight_scale=%d",
+             berr, casename,
+             I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T),
+             (int)Wmd, (int)Wme, (int)Wmg, (int)Wed, (int)Wee,
+             (int)Wgd, (int)Wgg, (int)Wme, (int)Wmg, (int)weight_scale);
+      return 0;
+    }
+  } else if (3*E < T && 3*G < T) { // E < T/3 && G < T/3
+    int64_t R = MIN(E, G);
+    int64_t S = MAX(E, G);
+    /*
+     * Case 2: Both Guards and Exits are scarce
+     * Balance D between E and G, depending upon
+     * D capacity and scarcity.
+     */
+    if (R+D < S) { // Subcase a
+      Wgg = weight_scale;
+      Wee = weight_scale;
+      Wmg = 0;
+      Wme = 0;
+      Wmd = 0;
+      if (E < G) {
+        casename = "Case 2a (E scarce)";
+        Wed = weight_scale;
+        Wgd = 0;
+      } else { /* E >= G */
+        casename = "Case 2a (G scarce)";
+        Wed = 0;
+        Wgd = weight_scale;
+      }
+    } else { // Subcase b: R+D >= S
+      casename = "Case 2b1 (Wgg=1, Wmd=Wgd)";
+      Wee = (weight_scale*(E - G + M))/E;
+      Wed = (weight_scale*(D - 2*E + 4*G - 2*M))/(3*D);
+      Wme = (weight_scale*(G-M))/E;
+      Wmg = 0;
+      Wgg = weight_scale;
+      Wmd = (weight_scale - Wed)/2;
+      Wgd = (weight_scale - Wed)/2;
+
+      berr = networkstatus_check_weights(Wgg, Wgd, Wmg, Wme, Wmd, Wee, Wed,
+                                       weight_scale, G, M, E, D, T, 10, 1);
+
+      if (berr) {
+        casename = "Case 2b2 (Wgg=1, Wee=1)";
+        Wgg = weight_scale;
+        Wee = weight_scale;
+        Wed = (weight_scale*(D - 2*E + G + M))/(3*D);
+        Wmd = (weight_scale*(D - 2*M + G + E))/(3*D);
+        Wme = 0;
+        Wmg = 0;
+
+        if (Wmd < 0) { // Can happen if M > T/3
+          casename = "Case 2b3 (Wmd=0)";
+          Wmd = 0;
+          log_warn(LD_DIR,
+                   "Too much Middle bandwidth on the network to calculate "
+                   "balanced bandwidth-weights. Consider increasing the "
+                   "number of Guard nodes by lowering the requirements.");
+        }
+        Wgd = weight_scale - Wed - Wmd;
+        berr = networkstatus_check_weights(Wgg, Wgd, Wmg, Wme, Wmd, Wee,
+                  Wed, weight_scale, G, M, E, D, T, 10, 1);
+      }
+      if (berr != BW_WEIGHTS_NO_ERROR &&
+              berr != BW_WEIGHTS_BALANCE_MID_ERROR) {
+        log_warn(LD_DIR,
+             "Bw Weights error %d for %s v10. G="I64_FORMAT" M="I64_FORMAT
+             " E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT
+             " Wmd=%d Wme=%d Wmg=%d Wed=%d Wee=%d"
+             " Wgd=%d Wgg=%d Wme=%d Wmg=%d weight_scale=%d",
+             berr, casename,
+             I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T),
+             (int)Wmd, (int)Wme, (int)Wmg, (int)Wed, (int)Wee,
+             (int)Wgd, (int)Wgg, (int)Wme, (int)Wmg, (int)weight_scale);
+        return 0;
+      }
+    }
+  } else { // if (E < T/3 || G < T/3) {
+    int64_t S = MIN(E, G);
+    // Case 3: Exactly one of Guard or Exit is scarce
+    if (!(3*E < T || 3*G < T) || !(3*G >= T || 3*E >= T)) {
+      log_warn(LD_BUG,
+           "Bw-Weights Case 3 v10 but with G="I64_FORMAT" M="
+           I64_FORMAT" E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT,
+               I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+               I64_PRINTF_ARG(D), I64_PRINTF_ARG(T));
+    }
+
+    if (3*(S+D) < T) { // Subcase a: S+D < T/3
+      if (G < E) {
+        casename = "Case 3a (G scarce)";
+        Wgg = Wgd = weight_scale;
+        Wmd = Wed = Wmg = 0;
+        // Minor subcase, if E is more scarce than M,
+        // keep its bandwidth in place.
+        if (E < M) Wme = 0;
+        else Wme = (weight_scale*(E-M))/(2*E);
+        Wee = weight_scale-Wme;
+      } else { // G >= E
+        casename = "Case 3a (E scarce)";
+        Wee = Wed = weight_scale;
+        Wmd = Wgd = Wme = 0;
+        // Minor subcase, if G is more scarce than M,
+        // keep its bandwidth in place.
+        if (G < M) Wmg = 0;
+        else Wmg = (weight_scale*(G-M))/(2*G);
+        Wgg = weight_scale-Wmg;
+      }
+    } else { // Subcase b: S+D >= T/3
+      // D != 0 because S+D >= T/3
+      if (G < E) {
+        casename = "Case 3bg (G scarce, Wgg=1, Wmd == Wed)";
+        Wgg = weight_scale;
+        Wgd = (weight_scale*(D - 2*G + E + M))/(3*D);
+        Wmg = 0;
+        Wee = (weight_scale*(E+M))/(2*E);
+        Wme = weight_scale - Wee;
+        Wmd = (weight_scale - Wgd)/2;
+        Wed = (weight_scale - Wgd)/2;
+
+        berr = networkstatus_check_weights(Wgg, Wgd, Wmg, Wme, Wmd, Wee,
+                    Wed, weight_scale, G, M, E, D, T, 10, 1);
+      } else { // G >= E
+        casename = "Case 3be (E scarce, Wee=1, Wmd == Wgd)";
+        Wee = weight_scale;
+        Wed = (weight_scale*(D - 2*E + G + M))/(3*D);
+        Wme = 0;
+        Wgg = (weight_scale*(G+M))/(2*G);
+        Wmg = weight_scale - Wgg;
+        Wmd = (weight_scale - Wed)/2;
+        Wgd = (weight_scale - Wed)/2;
+
+        berr = networkstatus_check_weights(Wgg, Wgd, Wmg, Wme, Wmd, Wee,
+                      Wed, weight_scale, G, M, E, D, T, 10, 1);
+      }
+      if (berr) {
+        log_warn(LD_DIR,
+             "Bw Weights error %d for %s v10. G="I64_FORMAT" M="I64_FORMAT
+             " E="I64_FORMAT" D="I64_FORMAT" T="I64_FORMAT
+             " Wmd=%d Wme=%d Wmg=%d Wed=%d Wee=%d"
+             " Wgd=%d Wgg=%d Wme=%d Wmg=%d weight_scale=%d",
+             berr, casename,
+             I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T),
+             (int)Wmd, (int)Wme, (int)Wmg, (int)Wed, (int)Wee,
+             (int)Wgd, (int)Wgg, (int)Wme, (int)Wmg, (int)weight_scale);
+        return 0;
+      }
+    }
+  }
+
+  /* We cast down the weights to 32 bit ints on the assumption that
+   * weight_scale is ~= 10000. We need to ensure a rogue authority
+   * doesn't break this assumption to rig our weights */
+  tor_assert(0 < weight_scale && weight_scale < INT32_MAX);
+
+  /*
+   * Provide Wgm=Wgg, Wmm=1, Wem=Wee, Weg=Wed. May later determine
+   * that middle nodes need different bandwidth weights for dirport traffic,
+   * or that weird exit policies need special weight, or that bridges
+   * need special weight.
+   *
+   * NOTE: This list is sorted.
+   */
+  r = tor_snprintf(buf, sizeof(buf),
+     "bandwidth-weights Wbd=%d Wbe=%d Wbg=%d Wbm=%d "
+     "Wdb=%d "
+     "Web=%d Wed=%d Wee=%d Weg=%d Wem=%d "
+     "Wgb=%d Wgd=%d Wgg=%d Wgm=%d "
+     "Wmb=%d Wmd=%d Wme=%d Wmg=%d Wmm=%d\n",
+     (int)Wmd, (int)Wme, (int)Wmg, (int)weight_scale,
+     (int)weight_scale,
+     (int)weight_scale, (int)Wed, (int)Wee, (int)Wed, (int)Wee,
+     (int)weight_scale, (int)Wgd, (int)Wgg, (int)Wgg,
+     (int)weight_scale, (int)Wmd, (int)Wme, (int)Wmg, (int)weight_scale);
+  if (r<0) {
+    log_warn(LD_BUG,
+             "Not enough space in buffer for bandwidth-weights line.");
+    *buf = '\0';
+    return 0;
+  }
+  smartlist_add(chunks, tor_strdup(buf));
+
+  log_notice(LD_CIRC, "Computed bandwidth weights for %s with v10: "
+             "G="I64_FORMAT" M="I64_FORMAT" E="I64_FORMAT" D="I64_FORMAT
+             " T="I64_FORMAT,
+             casename,
+             I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
+             I64_PRINTF_ARG(D), I64_PRINTF_ARG(T));
+  return 1;
+}
+/**
+ * This function computes the bandwidth weights for consensus method 9.
+ *
+ * It has been obsoleted in favor of consensus method 10.
+ */
 static void
 networkstatus_compute_bw_weights_v9(smartlist_t *chunks, int64_t G, int64_t M,
                               int64_t E, int64_t D, int64_t T,
@@ -1064,7 +1324,7 @@ networkstatus_compute_bw_weights_v9(smartlist_t *chunks, int64_t G, int64_t M,
     *buf = '\0';
   }
   smartlist_add(chunks, tor_strdup(buf));
-  log_notice(LD_CIRC, "Computed bandwidth weights for %s: "
+  log_notice(LD_CIRC, "Computed bandwidth weights for %s with v9: "
              "G="I64_FORMAT" M="I64_FORMAT" E="I64_FORMAT" D="I64_FORMAT
              " T="I64_FORMAT,
              casename,
@@ -1101,6 +1361,7 @@ networkstatus_compute_consensus(smartlist_t *votes,
   const routerstatus_format_type_t rs_format =
     flavor == FLAV_NS ? NS_V3_CONSENSUS : NS_V3_CONSENSUS_MICRODESC;
   char *params = NULL;
+  int added_weights = 0;
   tor_assert(flavor == FLAV_NS || flavor == FLAV_MICRODESC);
   tor_assert(total_authorities >= smartlist_len(votes));
 
@@ -1783,7 +2044,13 @@ networkstatus_compute_consensus(smartlist_t *votes,
       }
     }
 
-    networkstatus_compute_bw_weights_v9(chunks, G, M, E, D, T, weight_scale);
+    if (consensus_method < 10) {
+      networkstatus_compute_bw_weights_v9(chunks, G, M, E, D, T, weight_scale);
+      added_weights = 1;
+    } else {
+      added_weights = networkstatus_compute_bw_weights_v10(chunks, G, M, E, D,
+                                                           T, weight_scale);
+    }
   }
 
   /* Add a signature. */
@@ -1873,7 +2140,7 @@ networkstatus_compute_consensus(smartlist_t *votes,
       return NULL;
     }
     // Verify balancing parameters
-    if (consensus_method >= MIN_METHOD_FOR_BW_WEIGHTS) {
+    if (consensus_method >= MIN_METHOD_FOR_BW_WEIGHTS && added_weights) {
       networkstatus_verify_bw_weights(c);
     }
     networkstatus_vote_free(c);