[tor-bugs] #5049 [Tor Client]: When LearnCircuitBuildTimeout is off, we should ignore (most?)( CBT parameters from the consensus

[Tor Bug Tracker & Wiki]

#5049: When LearnCircuitBuildTimeout is off, we should ignore (most?)( CBT
parameters from the consensus
------------------------+---------------------------------------------------
 Reporter:  nickm       |          Owner:                    
     Type:  defect      |         Status:  needs_revision    
 Priority:  normal      |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Client  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------
Changes (by nickm):

  * status:  needs_review => needs_revision


Comment:

 Looks good, I think!  Here are some thoughts, in no particular order.

 I wonder if the debug logs should be if (!LearnCircuitBuildTimeout) {
 log_debug(LD_BUG,... ) } instead of log_debug(LD_CIRC,).  LD_BUG is what
 we generally use for stuff that will never get reached.

 Can you explain the process you used to make sure that you covered all the
 cases here?  Did you just add the debugging statements then modify the
 code until they stopped appearing, or did you do a call graph analysis to
 make sure the functions couldn't be reached when LearnCircuitBuildTimeout
 is 0, or both?

 It is probably okay, if I am reading the code right, to just tor_assert()
 that circuit_build_times_recent_circuit_count() is greater than 0: Its use
 of networkstatus_get_param() clips the range of its output so that it
 should always lie between CBT_MIN_RECENT_CIRCUITS and
 CBT_MAX_RECENT_CIRCUITS.

 The tor_free() macro sets its argument (an lvalue) to NULL; I think that
 our current convention is not to include an additional assign-to-NULL
 afterwards.

 Perhaps the code that frees and clears the circuit-build-history data
 should get extracted into its own function?

 Tor's house style is K&R C, so we stick the else on the same line as both
 surrounding braces, as in
 {{{
   if (foo) {
      ...
   } else if (bar) {
      ...
   } else {
      ...
   }
 }}}

 Now, this one is an alternative design, not a problem:

 Does anything still call circuit_build_times_get_initial_timeout() when
 LearnCBT is off?  It seems that it's used to set the initial value of
 close_ms and timeout_ms, which are later used in circuituse.c when we are
 computing circuit timeouts.  Perhaps instead of having the global
 "circ_times" be what use in circuituse.c, we should instead have a
 function that returns close_ms and timeout_ms (when LearnCBT is 1) or
 returns options->CircuitBuildTimeout * 1000 (when LearnCBT is 0).  We
 could call this function in place of the current invocations of close_ms
 and timeout_ms in circuituse.c.  Do you think that would be cleaner?

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