[tor-bugs] #19877 [Core Tor/Tor]: Implement new guard selection algorithm of prop 271

[Tor Bug Tracker & Wiki]

#19877: Implement new guard selection algorithm of prop 271
-------------------------------------------------+-------------------------
 Reporter:  andrea                               |          Owner:  nickm
     Type:  task                                 |         Status:
                                                 |  assigned
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.0.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  unspecified
 Severity:  Normal                               |     Resolution:
 Keywords:  isaremoved, nickwants029, tor-       |  Actual Points:
  guards-revamp, TorCoreTeam201611               |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  SponsorU-must
-------------------------------------------------+-------------------------

Comment (by asn):

 Hello, I did a very rough initial review based on the WIP branch
 `prop271-wip`. Haven't run it yet. Great work so far, very excited about
 splitting bridge code to another file as well.

 Some of these comments might be too low level for this stage of
 implementation, so feel free to ignore them. Also, I have not re-loaded
 the whole proposal in my brain yet, so I didn't go too deep in the nitty
 gritty details.

 - There are a few big scary functions that could be splitted into smaller
 functions to make them more manageable. e.g.
 `sampled_guards_update_from_consensus()` and
 `entry_guards_update_primary`.

   Also the fundamental function `select_entry_guard_for_circuit()` could
 be split into three smaller functions, each for one spec case.

 - Agreed that the `entry_guard_succeeded()` tristate is ugly and makes the
 `circuit_send_next_onion_skin()` logic harder to read. Perhaps the retval
 could be turned into an enum?

 - If `add_bridge_as_entry_guard()` is a pub function of the entrynodes
 API, should it have a prefix? Or we are not at that level of tidyness yet?

 - Why do we need the spaceless ISO time functions? Can't we use the
 spaceful ones for state? Or is it to maintain backwards compat?

 - Parsing guards from state seems a bit of a pain now. For example,
 `entry_guard_parse_from_state()` does ad-hoc string parsing. Would it be
 possible to use the config parsing API for that?

 - Could `smartlist_remove_keeporder()` be implemented with
 `smartlist_pos()` and `smartlist_del_keeporder()` to avoid writing more
 smartlist code?

 Finally, how should one robustly test this branch? Do you use iptable
 scripts or something?

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