Opened 8 months ago

Closed 3 months ago

#28698 closed defect (fixed)

When circuit times are fixed, they can't be "relaxed"

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.7-alpha
Severity: Normal Keywords: easy, log, intro, fast-fix, 035-backport?, consider-backport-after-0404
Cc: Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: mikeperry Sponsor:

Description

In #28639, we had trouble diagnosing a sbws bug. tor said that circuit times were "relaxed", even when LearnCircuitBuildTimeout was 0.

When circuit_build_times_disabled() is true, we should change these log messages so that they make sense:
https://github.com/torproject/tor/blob/b058f64cc002b44e6dd48616ca3163a01c3f3e14/src/core/or/circuituse.c#L591

I think relaxed_timeout is meaningless when circuit_build_times_disabled() is true, but that's another issue.

Child Tickets

Change History (11)

comment:1 Changed 6 months ago by guigom

I've written a little patch to start working on this at https://github.com/JMGuisadoG/tor/tree/ticket28698
commit: https://github.com/JMGuisadoG/tor/commit/9ae8031ac57b6e741230b23c213f904f8d629d1f

Not sure what to write down on the ChangeLog file but stated this as a minor bugfix. Hope everything is ok, as this is my first contribution I'm expecting things will have to be changed a little.

Thank you!

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

Keywords: 040-proposed added
Milestone: Tor: unspecifiedTor: 0.4.0.x-final
Points: 0.2
Status: newneeds_revision

Replying to guigom:

I've written a little patch to start working on this at https://github.com/JMGuisadoG/tor/tree/ticket28698
commit: https://github.com/JMGuisadoG/tor/commit/9ae8031ac57b6e741230b23c213f904f8d629d1f

Thanks for this patch!

Not sure what to write down on the ChangeLog file but stated this as a minor bugfix. Hope everything is ok, as this is my first contribution I'm expecting things will have to be changed a little.

Your changes file looks good.

Here's how we write changes files:
https://github.com/torproject/tor/blob/master/doc/HACKING/CodingStandards.md#how-we-log-changes

Your patch looks good, but we also need to disable the other log message here:
https://github.com/JMGuisadoG/tor/commit/9ae8031ac57b6e741230b23c213f904f8d629d1f#diff-4f24e2b0a904b912701ad3d1efe53485R631

If you open a pull request at https://github.com/torproject/tor , our CI will run on your patch. Then you can see any errors on platforms you don't have.

Notes for the team: We can review this bugfix for 0.4.0. There's no need to discuss it at the team meeting.

comment:3 Changed 6 months ago by guigom

Ok, I've made the required change and opened the pull request at https://github.com/torproject/tor/pull/666/commits/09729649791b6b5f593c95cd232fc52a4bcfd6d8

comment:4 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

comment:5 Changed 5 months ago by dgoulet

Reviewer: mikeperry

comment:6 Changed 5 months ago by mikeperry

Status: needs_reviewmerge_ready

This looks good wrt just disabling the loglines.

(It's also my read of the source that relaxed_timeout doesn't apply if CBT is disabled.. This should mean that only the second log message would get emitted, not the first. I hope that is the case).

comment:7 Changed 5 months ago by nickm

Keywords: 035-backport? added
Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final

Since this is a UX issue, I'm going to say we can consider it for 035 backport. I cherry-picked it to maint-0.3.5 in a new branch bug28698_035 and made a PR for that as https://github.com/torproject/tor/pull/710 .

Then I merged it to 0.4.0 and forward.

comment:8 Changed 5 months ago by nickm

Keywords: 040-proposed removed

comment:9 Changed 4 months ago by teor

Keywords: consider-backport-after-0404-alpha added

These tickets are low-risk changes that were marked for backport after the release of 0.4.0.2-alpha. I want them to get testing in at least one alpha release, so I'll look at them after 0.4.0.4-alpha is released.

comment:10 Changed 3 months ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:11 Changed 3 months ago by teor

Actual Points: 0.2
Resolution: fixed
Status: merge_readyclosed
Version: Tor: 0.2.4.7-alpha

Merged to 0.3.5 and merged forward.

Merged #23790, #29665, #29017, #27199, #29144, #13221, #28698.

Note: See TracTickets for help on using tickets.