#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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs