[tor-bugs] #21425 [Core Tor/Tor]: entry_list_is_constrained() should look at the guard_selection_t object

[Tor Bug Tracker & Wiki]

#21425: entry_list_is_constrained() should look at the guard_selection_t object
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.2.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  guards, 031-deferred-20170425,       |  Actual Points:
  review-group-18                                |
Parent ID:  #20822                               |         Points:  .5
 Reviewer:  asn                                  |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Hello, I'm not a huge fan of the attached patch.

 I think Roger's comment:2 is key here: we use
 `entry_list_is_constrained()` to check whether we only have a hardcoded
 set of already-sampled nodes to choose from. We use it to aggressively
 retry guards (since we can't sample new ones), or to ignore certain
 reachability failures (so that we can retry dirfetches).

 The proposed patch does the following:
 {{{
 +entry_list_is_constrained(const or_options_t *options, guard_selection_t
 *gs)
  {
 -  // XXXX #21425 look at the current selection.
 -  if (options->EntryNodes)
 -    return 1;
 -  if (options->UseBridges)
 +  if (gs->primary_guards_up_to_date)
      return 1;
    return 0;
 }}}
 which seems kinda random (what do primary guards have to do with this?),
 and definitely not equivalent to the previous functionality of that
 function.

 I think the current version of this function is much better than the
 proposed alternative, and I can't come up with an obvious improvement
 here.

 I'm somewhere between `WONTFIX`ing this, and waiting some more to see if
 problems come up with the current code.

 Otherwise we should try to definite more precisely what
 `entry_list_is_constrained()` is supposed to do and see if we are missing
 any obvious cases here.

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