Opened 9 months ago

Closed 9 months ago

#21594 closed defect (fixed)

Hidden Services with many intro points delay checking circuits on startup

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.9-alpha
Severity: Normal Keywords: tor-hs, 030-backport
Cc: Actual Points: 0.2
Parent ID: #21446 Points: 0.2
Reviewer: Sponsor:

Description

When a hidden service with 8-10 introduction points starts, it delays checking for failed circuits for 5 minutes.

Fixes part of issue 1 in #21446.

Child Tickets

Change History (7)

comment:1 Changed 9 months ago by teor

Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final
Status: newneeds_review

Given this is a very minor change, we might want to put it in 0.3.0.
But it's not a regression, so it's really up to nickm.

Please see my branch bug21594_030, which accounts for the extra introduction points selected on startup, and the equality in the comparison.

comment:2 Changed 9 months ago by dgoulet

Status: needs_reviewneeds_information

Oh wow, that is a good find! So 10 was the maximum before and I bet it was considered to be that value because of "3" intro points. Which means, we would allow two rounds of intro point circuit connection because 3 + 2 (extra for performance).

Following that, I think it would be wise to do something like that which is at the very least do a retry if all 10 circs. fail. Now that makes it a bit more "involving" because it would be a dynamic maximum depending on how many intro points.

So two choices, doing that dynamic thingy (not that crazy, there is one callsite I believe checking that maximum) or we just say (10 + 2) * 2 is our new max per period.

Thoughts?

comment:3 in reply to:  2 Changed 9 months ago by teor

Status: needs_informationneeds_review

Replying to dgoulet:

Oh wow, that is a good find! So 10 was the maximum before and I bet it was considered to be that value because of "3" intro points. Which means, we would allow two rounds of intro point circuit connection because 3 + 2 (extra for performance).

Following that, I think it would be wise to do something like that which is at the very least do a retry if all 10 circs. fail. Now that makes it a bit more "involving" because it would be a dynamic maximum depending on how many intro points.

So two choices, doing that dynamic thingy (not that crazy, there is one callsite I believe checking that maximum) or we just say (10 + 2) * 2 is our new max per period.

Thoughts?

Do the dynamic maximum - that means we tolerate almost all our connections failing in the first 5 minutes, but if they all fail, we wait.

I pushed two fixups to the branch bug21594_030, which replace the macro with a static function.

comment:4 Changed 9 months ago by nickm

This looks safe enough on the high end. But on the low end -- when n_intro_points_wanted is just 3 (the default) -- it takes the number of attempts per period from 10 down to 8. Is that what we want? (Non-rhetorical question. Maybe it is.)

comment:5 in reply to:  4 Changed 9 months ago by teor

Replying to nickm:

This looks safe enough on the high end. But on the low end -- when n_intro_points_wanted is just 3 (the default) -- it takes the number of attempts per period from 10 down to 8. Is that what we want? (Non-rhetorical question. Maybe it is.)

The limit was originally 2*(default + extra) == 10.

But I think we want 2*default + extra == 8, because:

  /* Allow all but one of the initial connections to fail and be
   * retried. (If all fail, we *want* to wait, because something is broken.) */

But I don't really mind either way, if you think it's less risky to maintain it at 10 in 0.3.0.5-rc, it doesn't make that much difference.

comment:6 Changed 9 months ago by teor

Keywords: 030-backport added

comment:7 Changed 9 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

okay, that's persuasive. Merging to 030.

Note: See TracTickets for help on using tickets.