Opened 10 months ago

Closed 9 months ago

#25577 closed defect (fixed)

cmux: CircuitPriorityHalflife value is never taken from the consensus

Reported by: dgoulet Owned by: nickm
Priority: High Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: regression, 034-triage-20180328, 034-must, regression fast-fix
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Commit 6b1dba214db introduced an issue that makes the cmux EWMA subsystem to never use the CircuitPriorityHalflife option from the consensus resulting in a warning at bootup:

Mar 21 22:31:24.001 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.

The default config is:

  V(CircuitPriorityHalflife,     DOUBLE,  "-1.0"), /*negative:'Use default'*/

Which means that in get_circuit_priority_halflife() which is called at bootup when loading the config file and when a new consensus arrives, always skip the consensus param check due to this wrong condition (EPSILON=0.0001):

+  if (options && options->CircuitPriorityHalflife < EPSILON) {
+    halflife = options->CircuitPriorityHalflife;
+    *source_msg = "CircuitPriorityHalflife in configuration";
+    goto end;
+  }

Originally, the condition was this which resulted in false with -1.0 and thus trying the consensus param instead:

-  if (options && options->CircuitPriorityHalflife >= -EPSILON) {
-    halflife = options->CircuitPriorityHalflife;
-    source = "CircuitPriorityHalflife in configuration";

The good news is that we didn't release that commit yet! and the default value is what currently the consensus is using (30000) :). Nevertheless, this should be fixed asap.

Child Tickets

Change History (9)

comment:1 in reply to:  description Changed 10 months ago by sysrqb

Replying to dgoulet:

resulting in a warning at bootup

This warning occurs after we download a new consensus, too.

Mar 23 07:26:20.000 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.
Mar 23 08:16:20.000 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.
Mar 23 09:01:20.000 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.
Mar 23 10:05:20.000 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.
Mar 23 11:08:20.000 [warn] CircuitPriorityHalflife is too small (-1.000000). Adjusting to the smallest value allowed: 30.000000.

comment:2 Changed 10 months ago by dgoulet

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

Woa, this is only in 034, my mistake.

comment:3 Changed 10 months ago by nickm

Keywords: 034-triage-20180328 added

comment:4 Changed 10 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:5 Changed 10 months ago by dgoulet

Keywords: 034-must added; 034-removed-20180328 removed

This is a regression that needs to go in 034.

comment:6 Changed 9 months ago by nickm

Owner: changed from dgoulet to nickm
Status: assignedaccepted

comment:7 Changed 9 months ago by nickm

Keywords: fast-fix added
Status: acceptedneeds_review

I think this is a one-liner: please review bug25577_034?

comment:8 in reply to:  7 Changed 9 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

Replying to nickm:

I think this is a one-liner: please review bug25577_034?

Yes exactly that is the fix! Thanks!

comment:9 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.