[tor-bugs] #23100 [Core Tor/Tor]: Circuit Build Timeout needs to count hidden service circuits

[Tor Bug Tracker & Wiki]

#23100: Circuit Build Timeout needs to count hidden service circuits
-------------------------------------------------+-------------------------
 Reporter:  mikeperry                            |          Owner:
                                                 |  mikeperry
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.3.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.2.7
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-hs, path-bias, guard-discovery-  |  Actual Points:
  prop247-controller, needs-proposal, mike-can,  |
  prop247, tor-guard                             |
Parent ID:  #9001                                |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by mikeperry):

 Replying to [comment:3 asn]:
 > Thanks for the patch! This seems important! A few comments:
 >
 > - I don't understand how the mods to
 `circuit_timeout_want_to_count_circ()` helps us achieve the goal of this
 ticket. It kinda seems like we are restricting that function even more
 (doc change implies that too), when we actually want it to be more
 accepting. I think a nice comment is required for people like me to
 understand how that function works wrt HS circs.

 Ok, I will add to the comment to explain this. Basically we're not making
 it more restrictive. We are changing it from considering only *exactly*
 three hop circuits to considering any circuit that has currently less than
 or equal to 3 hops built (and plans to build at least 3 hops, but maybe
 more).

 > - I also don't understand why the
 `circuit_build_times_decide_to_count_circ()` logic was moved from one
 function to another. I think we need some help here to understand, since
 I'm not familiar with the CBT system at all.

 Actually, what happened here was that a bunch of code that was dangling
 off of circuit_build_no_more_hops() got its own function that does only
 one thing (decides to count the circuit). circuit_send_next_onion_skin()
 now calls that function before we decide if we're done adding hops (so we
 cant calculate timeout info at the point where the circuit reaches 3
 hops).

 > - I think `circuit_get_cpath_len()` should be moded instead of adding
 another func `circuit_get_cpath_opened_len()`. We can add an arg
 `only_count_opened` which does that. I think that'd be cleaner.

 In terms of code readability, code quality, and testability, I really
 disagree here. Combining these two would make for a really ugly
 conditional in the loop which will take much more effort to verify is
 correct than two separate functions. Keeping it simpler by having two
 different functions is preferable IMO. It's not like binary size matters
 for us...

 > - `circuit_build_times_decide_to_count_circ()` is undocumented and I'd
 actually like to know what it does.

 Ok I will add a comment here, too.

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