Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2799 closed defect (fixed)

Use cbt results when deciding to launch parallel introduction circuit?

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now, circuituse.c has this lovely snippet in circuit_get_best:

/* XXX022 make this 15 be a function of circuit finishing times we've
 * seen lately, a la Fallon Chen's GSoC work -RD */
#define REND_PARALLEL_INTRO_DELAY 15
    if (purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT &&
             !must_be_open && circ->state != CIRCUIT_STATE_OPEN &&
             circ->timestamp_created + REND_PARALLEL_INTRO_DELAY < now) {
      intro_going_on_but_too_old = 1;
      continue;
    }

Now that we've got our circ_times stuff, we can totally get a better estimate of "that time after which we should launch another introduction circuit but still give this one time to finish." Yes?

Parenthetically, I have no idea what "!must_be_open" is doing there.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by mikeperry

Do you think we should use the normal 3 hop timeout for this? If so, this is simple, just replace that #define with tor_lround(circ_times.timeout_ms/1000).

comment:2 Changed 8 years ago by nickm

Why would we use the 3-hop timeout for these 4-hop circuits? I'm hoping you'll tell me what timeout this should be. :)

Also, incidentally, will all hell break out if circ_times.timeout_ms is ever less than 500 ? Having a timeout that rounds to 0 seconds seems like it will do horribly somewhere. Are we solving this?

comment:3 Changed 8 years ago by nickm

Status: newneeds_review

I was bored, so I tried a hack to replace timestamp_created with highres_created for circuits. Once I did that, I tried your suggestion.

Mike, what do you think of branch cbt_hires in my public repo? It still needs a few things, including:

  • Implementions of timercmp and timersub on platforms that lack them.
  • Changes files.

comment:4 Changed 8 years ago by mikeperry

Looks ok. But should we be calling gettimeofday in all these functions as opposed to passing the value in? I suppose they aren't called more than once per second in all cases anyways, right?

Also, re your previous comments, yes we ensure the timeout_ms is never below 500, and kick out a warn if it is. See CBT_MIN_TIMEOUT_MIN_VALUE. It is also possible to set a consensus parameter to override this define.

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay. I fixed up the remaining issues I knew about (missing changes files, possibly missing timeradd/timersub) issues with a few more commits to cbt_hires, then rebased in cbt_hires_2, then merged that. I'm leaving the gettimeofday calls as-is for accuracy, since this isn't in the criticial path (as you note). We can raise them back out if we're wrong.

Thanks for the review!

comment:6 Changed 8 years ago by mikeperry

We can also verify this with either info logs or with a control port listener logging the BUILDTIMEOUT_SET event. There should be a TIMEOUT_RATE parameter that remains around 0.2 before/after this change. Torperf also automatically logs this result now too, so all we have to do is keep an eye on the torperf output when those clients first upgrade to this diff.

What versions will this appear in?

comment:7 Changed 8 years ago by nickm

Subsequent 0.2.2.x releases, and all 0.2.3.x releases.

comment:8 Changed 7 years ago by nickm

Keywords: tor-hs added

comment:9 Changed 7 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.