Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#24769 closed defect (implemented)

Increase client idle and connection timeouts to reduce network load

Reported by: teor Owned by: mikeperry
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: tor-client, dos-resistance, 032-backport, 031-backport, 029-backport, 025-backport-maybe, review-group-32, 033-must
Cc: mikeperry Actual Points:
Parent ID: Points: 1
Reviewer: nickm Sponsor:

Description

These changes were introduced in d5a151a in 0.3.1.1.

Maybe we should:

  • revert the changes, or increase the values
  • make consensus parameters for them
+/** If we haven't yet decided on a good timeout value for circuit
+ * building, we close idle circuits aggressively so we can get more
+ * data points. */
+#define IDLE_TIMEOUT_WHILE_LEARNING (1*60)
-/** If we haven't yet decided on a good timeout value for circuit
- * building, we close idles circuits aggressively so we can get more
- * data points. */
-#define IDLE_TIMEOUT_WHILE_LEARNING (10*60)
+#define CONNTIMEOUT_CLIENTS_BASE 180 // 3 to 4.5 min
+    timeout = CONNTIMEOUT_CLIENTS_BASE
+        + crypto_rand_int(CONNTIMEOUT_CLIENTS_BASE/2);

Child Tickets

Change History (18)

comment:1 Changed 7 months ago by arma

Woah. Yeah. So, right now we close circuits 60 seconds after making them, even if we opt to make testing circuits much less frequently? That is, when we have any predicted ports, we're going to end up churning through all our circuits every minute, anyway? That's crummy. It seems like the idle-timeout-while-learning should be a function of the cbttestfreq?

comment:2 in reply to:  1 Changed 7 months ago by teor

Keywords: 032-backport 031-backport 029-backport 025-backport-maybe added

Replying to arma:

Woah. Yeah. So, right now we close circuits 60 seconds after making them, even if we opt to make testing circuits much less frequently? That is, when we have any predicted ports, we're going to end up churning through all our circuits every minute, anyway? That's crummy. It seems like the idle-timeout-while-learning should be a function of the cbttestfreq?

Yes, I think we need to make it configurable, and possibly backport the change.
(But not to 0.2.8, because it's not supported as of 1 January 2018.)

I'm not sure what to do about the regular connection timeout. We should check what it was before, and maybe see if it needs to be longer.

comment:3 Changed 7 months ago by dgoulet

Cc: mikeperry added

comment:4 Changed 6 months ago by arma

This one is important, and we shouldn't forget about it.

comment:5 Changed 6 months ago by teor

Keywords: must-fix-before-033-stable added
Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Priority: MediumHigh

We have tags for that,

comment:6 Changed 6 months ago by teor

Parent ID: #24716

comment:7 Changed 6 months ago by mikeperry

Owner: set to mikeperry
Status: newassigned

This also came up in #24228, where we found that Tor was creating a new circuit about every 6 seconds with this timeout.

The goal is to learn a circuit build timeout within 30 minutes, so that unused orconn connections aren't padded by the netflow padidng for too long while we learn this timeout (which wastes bandwidth for clients that want less padding). In #24228, it looked like we may actually learn it within 10. So we could make this default 3X slower.

I will look at this and work on a patch to change the default and make it a consensus param.

comment:8 Changed 6 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Mark a lot of assigned/needs_revision tickets as 0.3.4. If you think this should happen in 0.3.3 instead, just let me know?

comment:9 Changed 6 months ago by mikeperry

Status: assignedneeds_review

Patches are in mikeperry/bug24769. I also have spec updates in bug24769 of my tor-spec remote.

comment:10 Changed 5 months ago by nickm

Keywords: review-group-32 added

comment:11 Changed 5 months ago by teor

Keywords: 033-must added; must-fix-before-033-stable removed

must-fix-before-033-stable is now 033-must

comment:12 Changed 5 months ago by teor

Milestone: Tor: 0.3.4.x-finalTor: 0.3.3.x-final

We really do want to fix this in 033

comment:13 Changed 5 months ago by nickm

Reviewer: nickm

comment:14 Changed 5 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

The code looks safe enough to me, and the defaults don't seem unreasonable.

My only worry would be that maybe networkstatus_get_param() might get called too often, but I don't think that will be the case in these contexts.

Merging to master!

comment:15 Changed 5 months ago by nickm

(Your torspec branch had some other stuff in it -- make sure your master is clean? I cherry-picked the relevant commit there.)

comment:16 Changed 5 months ago by nickm

Oh! These are incompatible:

+#define DFLT_IDLE_TIMEOUT_WHILE_LEARNING (3*60)
+      cbtlearntimeout
+        Default: 360

I'll edit the spec to match the implementation.

comment:17 Changed 5 months ago by mikeperry

Ugh sorry for the sloppiness in the spec repo. I did that rather quickly. :/

comment:18 Changed 5 months ago by nickm

no problem; that's why we review ;)

Note: See TracTickets for help on using tickets.