Hidden Services with many intro points delay checking circuits on startup

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 (moved).

changed milestone to %Tor: 0.3.0.x-final

added 030-backport actualpoints::0.2 component::core tor/tor milestone::Tor: 0.3.0.x-final parent::21446 points::0.2 priority::medium resolution::fixed severity::normal status::closed tor-hs type::defect version::tor 0.2.3.9-alpha labels

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.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.0.x-final
Status: new to needs_review

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?

Trac:
Status: needs_review to needs_information

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.

Trac:
Status: needs_information to needs_review

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.)

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.

Trac:
Keywords: N/A deleted, 030-backport added

okay, that's persuasive. Merging to 030.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

changed time estimate to 1h 36m

added 1h 36m of time spent

mentioned in issue #21599 (moved)

mentioned in issue #21622 (moved)

moved to tpo/core/tor#21594 (closed)

mentioned in issue tpo/core/tor#21599 (closed)