[tor-bugs] #13239 [Tor]: Maybe we want three preemptive internal circs for hidden services?

[Tor Bug Tracker & Wiki]

#13239: Maybe we want three preemptive internal circs for hidden services?
-------------------------------------------------+-------------------------
 Reporter:  arma                                 |          Owner:
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-client, tor-hs,                  |        Version:
  TorCoreTeam201604                              |     Resolution:
Parent ID:  #5271                                |  Actual Points:
 Reviewer:  asn                                  |         Points:  small
                                                 |        Sponsor:
                                                 |  SponsorR-can
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:3 sysrqb]:
 >
 > Branch bug13239 pushed. It also tries to make the logic a little more
 readable.

 I have mixed feelings about this branch. I think the function it tries to
 change really needs some love, but I'm not sure if the patch goes deep
 enough. It tries to make the function more readable by adding comments and
 macros but without changing any of its logic. Unfortunately, the logic of
 that function is so complex, that this is not an easy task.

 For starters, I'm not sure if it keeps the same functionality as the old
 code. Specifically, the patch adds this code:
 {{{
     if (!(flags & CIRCLAUNCH_IS_INTERNAL))
       return;
 }}}
 where the function will exit if some checks above don't return True. IIUC
 that was not the case  previously, where the function would just proceed
 to the next block of code if those checks did not come true. I don't see
 an explanation for changing this logic, and I'm not sure if it's correct.
 How do we reach the final block of code now?

 Also the code introduces some lower-case macros that are never undefined.
 Is that OK?

 Finally, I'm not sure if the refactoring introduced is worth it. It took
 me like an hour just to understand the changes and the function is still
 majorly messed up. I couldn't make sense of the comments added either, but
 that's mainly because the logic of the function is so wicked.

 I feel that the right refactoring here would take in account the whole
 function and not just those two code blocks. There are some log messages
 and checks (like `router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN`)
 that happen in every codeblock and can be simplified.

 I kinda feel that till a proper refactoring happens, it's easier to just
 change the constants '2' to '3' for the purposes of this ticket, instead
 of trying to semi-refactor it.

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