Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#13239 closed defect (fixed)

Maybe we want three preemptive internal circs for hidden services?

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, tor-hs, TorCoreTeam201605
Cc: teor Actual Points:
Parent ID: #5271 Points: 1
Reviewer: asn Sponsor: SponsorR-can

Description (last modified by arma)

  /* Fourth, see if we need any more hidden service (client) circuits. */
  if (rep_hist_get_predicted_internal(now, &hidserv_needs_uptime,
                                      &hidserv_needs_capacity) &&
      ((num_uptime_internal<2 && hidserv_needs_uptime) ||
        num_internal<2)) {

Right now we build 2 preemptive internal circuits if we've accessed a hidden service lately.

But when we click on a new .onion address, we use three circuits -- one to do the hsdesc fetch, one for the rend point, and one for the intro point.

You can see it in this walkthrough:
https://trac.torproject.org/projects/tor/wiki/doc/TorControlPortWalkthrough-HS
where we cannibalize the hsdesc fetch circ, cannibalize the rend point, and then have to build the intro point from scratch.

We could raise the above '2' to '3'. I'm not sure if it's worthwhile in practice though. Sure would be nice to have some way of answering that question.

Child Tickets

Change History (19)

comment:1 Changed 5 years ago by arma

Keywords: SponsorR tor-client added

comment:2 Changed 4 years ago by arma

Description: modified (diff)

comment:3 Changed 4 years ago by sysrqb

When a client cannibalizes a circuit for establishing a rend point, it doesn't modify the circuit and uses it as-is, so it remains 3 hops.

circuituse.c:circuit_launch_by_extend_info()

switch (purpose) {
        case CIRCUIT_PURPOSE_C_ESTABLISH_REND:
          /* it's ready right now */
          break;
        case CIRCUIT_PURPOSE_C_INTRODUCING:
        case CIRCUIT_PURPOSE_S_CONNECT_REND:
        case CIRCUIT_PURPOSE_C_GENERAL:
        case CIRCUIT_PURPOSE_S_ESTABLISH_INTRO:
          /* need to add a new hop */
          tor_assert(extend_info);
          if (circuit_extend_to_new_exit(circ, extend_info) < 0)
            return NULL;
          break;
        default:

This seems like a better way to improve performance than explicitly establishing a rendezvous circuit for each onion service connections (#13222).

Branch bug13239 pushed. It also tries to make the logic a little more readable.

comment:4 Changed 4 years ago by sysrqb

Status: newneeds_review

comment:5 Changed 4 years ago by teor

Cc: teor added

comment:6 Changed 4 years ago by dgoulet

Keywords: tor-hs added
Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

comment:7 Changed 4 years ago by nickm

Keywords: SponsorR removed
Sponsor: SponsorR

Bulk-replace SponsorR keyword with SponsorR sponsor field in Tor component.

comment:8 Changed 4 years ago by nickm

Points: small

comment:9 Changed 4 years ago by nickm

Keywords: pre028-patch added

comment:10 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???
Severity: Normal

This patch isn't 0.2.8 material until/unless we find out that it will actually be significantly helpful. We should open another ticket to do the investigation here.

comment:11 Changed 3 years ago by dgoulet

Keywords: pre028-patch removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Parent ID: #5271
Sponsor: SponsorRSponsorR-can

comment:12 Changed 3 years ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:13 Changed 3 years ago by dgoulet

Reviewer: asn

comment:14 in reply to:  3 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Replying to 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.

comment:15 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

Ok, to move this one forward, here is the _minimal_ fix suggested. Please make sure that this one liner is correct because there are two "need_uptime" thingy...

See branch: bug13239_029_01

Thought: what sysrqb did here is imo useful that is move hardcoded values to defines and improve code flow. Maybe we should open a ticket for "let's refactor/improve circuit_predict_and_launch_new()" for which what sysrqb did is not lost?

comment:16 in reply to:  15 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Replying to dgoulet:

Ok, to move this one forward, here is the _minimal_ fix suggested. Please make sure that this one liner is correct because there are two "need_uptime" thingy...

See branch: bug13239_029_01

Thought: what sysrqb did here is imo useful that is move hardcoded values to defines and improve code flow. Maybe we should open a ticket for "let's refactor/improve circuit_predict_and_launch_new()" for which what sysrqb did is not lost?

Your patch bug13239_029_01 looks good to me. It also seems to agree with sysrqb's patch in terms of the parameter changed.

I also created a ticket for refactoring the function entirely at #18873. I filed it under Tor: 0.2.?? because I was not sure what milestone to file it under.

comment:17 Changed 3 years ago by asn

Keywords: TorCoreTeam201605 added; TorCoreTeam201604 removed

comment:18 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged into master.

I'd also like to see the magic numbers replaced with constants; I'm adding a note on that to #18873

comment:19 Changed 3 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.