[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:
                                                 |  needs_revision
 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, review-      |
  group-13, actual-review-points=2               |
Parent ID:                                       |         Points:
 Reviewer:  chelseakomlo, asn                    |        Sponsor:
                                                 |  SponsorU-must
-------------------------------------------------+-------------------------

Comment (by nickm):

 Replying to [comment:17 chelseakomlo]:
 > Hey Nick,
 >
 > Here are some general thoughts, feel free to take/leave what is useful.
 I added notes to the gitlab review as well.
 >
 > - Naming conventions are a bit confusing, as we use entrynodes,
 entryguards, and guards. If this naming signifies pre and post prop271,
 maybe we can add in legacy namespacing to make it more clear (see below).
 Also, the bridge-specific module looks really great- if there is guard-
 specific code that is distinct from entrynodes, maybe this belongs in a
 separate guard module.
 >
 > - Making a clear distinction between pre and post prop271 code would
 definitely make reviewing easier, as well as eventually migrating away
 from legacy code... I liked asn's recommendation of namespacing, we could
 also move legacy code to it's own module, etc.

 I've tried adding #ifdefs around the old code, in
 472a621ccc8a90bf90d8394210519974ad235848.

 For namespacing, I think everything should wind up in one entryguards_*
 namespace once the legacy things are removed, and the rest of the code is
 cleaned up.  The guards_* functions are there as generic frontends to the
 old and new code.

 > - Some functions are quite large, such as
 sampled_guards_update_from_consensus and
 entry_guards_upgrade_waiting_circuits. I see some todos about splitting
 these up, which is great.

 Agreed.

 > - Would it make sense to put parsing code into a parser.c?

 I'd like to extract it into something more general; for now it's pretty
 specific, though it could become more generally useful.  I've added it
 #20919.

 > - I didn't see a lot of tests here:
 https://gitlab.com/nickm_tor/tor/merge_requests/11 - is there another set
 of commits for these?

 I think we discussed this on IRC, and decided you needed to hit "expand"
 on gitlab, and then things got better.

 > - bridges.c doesn't have test coverage :(

 Right. At least, that code is no less covered than it was before I moved
 it. :/

 > - It looks like there are several remaining todos that need to be
 completed. Which of these should be completed as part of this patch? For
 example, in entrynodes.c, update_guard_selection_choice has a comment
 about needing to expire existing circuits (line 653).

 That's taken care of, so I removed the comment and updated the doxygen in
 6a6625c632248c. Resolving todos in in general is #20718 under #20822

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