[tor-bugs] #9321 [Tor]: Load balance right when we have higher guard rotation periods

[Tor Bug Tracker & Wiki]

#9321: Load balance right when we have higher guard rotation periods
-------------------------+-------------------------------------------------
     Reporter:  arma     |      Owner:
         Type:  project  |     Status:  needs_review
     Priority:  normal   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor      |    Version:
   Resolution:           |   Keywords:  needs-proposal, tor-auth, tor-
Actual Points:           |  client, 026-triaged-1
       Points:           |  Parent ID:  #11480
-------------------------+-------------------------------------------------

Comment (by asn):

 Replying to [comment:28 nickm]:

 Thanks for the review. I pushed some more commits that fix your comments.
 Here are some comments on your comments comments:

 > Notes on file format:
 >   * A bad line in the file should probably get ignored, not cause the
 whole file to be unparseable.

 Good point. Done.

 I changed it so that errors in all lines will get ignored. However, the
 first header line (the one containing the date) needs to be correct. I
 don't see any reason for omitting that line in future versions of the
 script and it also allows us to detect expired guardfraction files (which
 in turn allows us to find bugs in the guardfraction script in dirauths)

 To achieve this, I had to change the interface of the parsing function and
 its unittests.

 >   * Errors while parsing the file should probably get reported by line
 number.

 Good point. Done.

 >   * Right now, applying the file to your vote is O(file_size * log
 (n_elts_in_vote)).  As the file gets bigger and bigger, this will take
 more and more time.  I wonder whether it matters.
 >

 Yes, I've been wondering about that too. It seems quite fast atm, and the
 file size should not increase very fast (except if 100k guards enter the
 Tor network). Let's leave it like this, and maybe optimize it in the
 future if there is a need to? (famous last words?)

 > Notes on guardfraction voting:
 >   * Shouldn't we include GuardFraction in consensus votes for all nodes,
 regardless of whether we think they're a Guard?  After all, other
 authorities might decide to vote on whether the node should be a Guard.

 Yes, you are very right. Thanks for catching this. Done.

 This also kills some code, which is even better :)

 >   * Why are guardfraction_percentage and its related flag duplicated in
 routerstatus_t and vote_routerstatus_t?  Remember, vote_routerstatus_t
 contains a routerstatus_t.

 Hm, that's how the measured bw file parsing code worked, and I duplicated
 the logic.
 In my new commits, I started using the routerstatus_t of the vote to save
 this info, and it seems to work fine in my Chutney network. I don't see
 any reason yet for this to be bad.

 This makes the code cleaner.

 >   * In routerstatus_parse_guardfraction , I'd be more comfortable if we
 checked the return value of strchr.
 >

 Done.

 > Notes on bw calculation:
 >   * Maybe guard_get_guardfraction_bandwidth should fill in a structure
 rather than allocating one; it's going to get called a lot.
 >

 Good point. Done.

 BTW, please check `update_total_bandwidth_weights()`. This function will
 be called for each router so it will be called a lot. For now, I placed
 the `guardfraction_bandwidth_t` in the stack even though the function will
 be called many times. I think that's better performance than mallocating
 it everytime it gets called (because it's just going to push the stack a
 few more bytes down in the function prologue). Is that right?

 If not, I can pass the `guardfraction_bandwidth_t` as an argument to that
 function.

 > I need to go back and look at the bandwidth formulas; I didn't check
 them this time around.
 >
 > On XXXs:
 >   * I don't think floor/ceiling matters.
 >   * It's okay not to assert for that invariant.

 OK.

 >   * I don't know about applying guardfraction to weight_for_dir.  Does
 that mean it would apply to choice of directory guards or not?

 Yeah, I was a bit confused about this. Proposal 236 only suggested to use
 guardfraction info when picking middles/exits. However, I think
 considering guardfraction info for directory requests too is  a good idea
 so that we load balance those as well.

 As far as directory guards are concerned, since they are sharing the same
 list as the circuit guards, I suspect that most of the nodes in that list
 will have been picked by the circuit path selection algorithm, so it
 doesn't really matter. I think.

 For now, I'm checking that `rule != WEIGHT_FOR_GUARD` which includes picks
 for middle/exit/directory and also `NO_WEIGHTING` which seems to be unused
 in the code.

 >   * is_possible_guard is not quite equivalent to is_guard; what did you
 mean there?

 Hm, I wanted to assert that the node we are applying guardfraction info on
 is a guard. This should be true because
 `routerstatus_parse_guardfraction()` only sets guardfraction info to
 guards.

 To do that, I would have to assert for `is_guard` in that function.
 However, `is_guard = node->is_possible_guard` which is fine in general,
 but it causes assert failures in directory authorities because the
 `node_t` is not initialized properly (#13297). Hence, I checked for
 `node->rs->is_possible_guard` which I think should be the same since the
 `rs` is assigned in `nodelist_set_consensus()` and then it does
 `node->is_possible_guard = rs->is_possible_guard`.

 :/

 >   * IMO it's fine to ignore smartlist_choose_node_by_bandwidth for now.

 OK.

 > Notes on tests:
 >   * test_helpers.h needs an #ifdef guard.

 Done.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9321#comment:29>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online