Opened 8 years ago

Closed 6 years ago

#5824 closed defect (fixed)

Stop reporting relay-only statistics as a bridge

Reported by: karsten Owned by: karsten
Priority: Low Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Some bridges report statistics that were originally designed for relays only. We throw these statistics away in the sanitizing process, so they don't end up in the tarballs. But the cleaner approach would be to not report them at all.

Here's how the patch could look like; I can make a proper patch with changes file later:

diff --git a/src/or/config.c b/src/or/config.c
index ab4f160..2179117 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -1680,8 +1680,9 @@ options_act(const or_options_t *old_options)
     time_t now = time(NULL);
     int print_notice = 0;
-    /* If we aren't acting as a server, we can't collect stats anyway. */
-    if (!server_mode(options)) {
+    /* If we aren't acting as a server or if we are a bridge, we can't
+     * collect relay-only stats. */
+    if (!server_mode(options) || options->BridgeRelay) {
       options->CellStatistics = 0;
       options->DirReqStatistics = 0;
       options->EntryStatistics = 0;

Is this something for 0.2.3.x or 0.2.4.x? Or do we not care at all?

Child Tickets

Change History (18)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

I think the check should be public_server_mode(), right? Other than that, it seems okay to me, even for 0.2.3.x.

comment:2 Changed 8 years ago by nickm

Status: newneeds_revision

comment:3 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Merged as 981e896dd2eaf69798bb503c271306ee779dd6d2

comment:4 Changed 8 years ago by karsten

Resolution: fixed
Status: closedreopened

Please revert 981e896dd2eaf69798bb503c271306ee779dd6d2. I want to look closer at dirreq stats reported by bridges before implementing something like #5807.

Sorry, I should have said so in this ticket before.

comment:5 Changed 8 years ago by nickm

Will revert.

So, what's the status of this ticket? If you're sure that the stuff isn't needed, then it turns out that you need this after all, I want to say that probably we don't know what the right answer is here, and we should defer consideration of this till 0.2.4.

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Status: reopenedneeds_revision

(putting this in 0.2.4.x for now. If you think some of it is salvageable for 0.2.3. please go ahead)

comment:7 Changed 8 years ago by karsten

I think it's safe to defer this until 0.2.4.x. I plan to do the #5807 analysis in the next few weeks. I'm cautiously optimistic that we'll be able to use dirreq stats reported by bridges. But for the moment, I'd like to leave things as they are.

Thanks for reverting the patch! And sorry again.

comment:8 Changed 8 years ago by karsten

Making a note here so we don't forget: once we decide which stats are published by relays and/or bridges, we may want to update dir-spec.txt.

comment:9 Changed 8 years ago by nickm

Keywords: tor-bridge added

comment:10 Changed 8 years ago by nickm

Component: Tor BridgeTor

comment:11 Changed 7 years ago by nickm

karsten, is this still something you want to do at some point? If it goes in 0.2.4, it needs to go in soon.

comment:12 Changed 7 years ago by karsten

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

No, it's fine to defer this until 0.2.5.x. Thanks for the heads-up anyway!

comment:13 Changed 6 years ago by karsten

Status: needs_revisionneeds_review

Progress! I updated the previous fix, so that bridges now don't report (cell,entry,exit)-stats anymore, but they still do report dirreq-stats. And I wrote another fix that makes bridges report complete dirreq-stats (which is related to this ticket, but might also go as separate ticket). Both fixes are pretty short but come with detailed commit messages. See my task-5824 branch:

If the patches make sense, I'll set up a test bridge running this branch and find some test clients.

comment:14 Changed 6 years ago by karsten

I'd appreciate code review of the patches before I run this code (or rather, a rebased branch of this one) on a public bridge. Thanks!

comment:15 Changed 6 years ago by nickm

They seem okay to me. Are we sure that the second patch in the branch doesn't turn on anything you don't want it to turn on?

comment:16 Changed 6 years ago by karsten

Owner: set to karsten
Status: needs_reviewassigned

Thanks for looking at the patches.

I'm quite sure that the second patch doesn't do anything we wouldn't want it to do. Quoting from the commit message: "In May 2012 we learned that we didn't fully disable directory request statistics on bridges. Bridges did report directory request statistics, but these statistics contained empty dirreq-v3-ips and dirreq-v3-reqs lines. But the remaining dirreq-* lines have always been non-empty." Publishing non-empty dirreq-v3-ips and dirreq-v3-reqs lines is something we'd want to do. So, I don't expect anything unexpected there.

But let me run a public bridge that doesn't publish its stats to Tonga for 24 hours, and then we'll see what stats it would publish.

Putting into "assigned" state again, because there's currently nothing that needs review.

comment:17 Changed 6 years ago by karsten

Status: assignedneeds_review

Here's what my bridge would have reported to Tonga (which I disabled):

dirreq-stats-end 2014-02-27 14:40:11 (86400 s)
dirreq-v3-ips de=8
dirreq-v3-reqs de=16
dirreq-v3-resp ok=16,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=0,busy=0
dirreq-v3-direct-dl complete=0,timeout=0,running=0
dirreq-v3-tunneled-dl complete=16,timeout=0,running=0,min=290969,d1=290969,d2=322941,q1=353079,d3=498461,d4=520702,md=539223,d6=559760,d7=560552,q3=569060,d8=570058,d9=574681,max=578271
bridge-stats-end 2014-02-27 14:40:15 (86400 s)
bridge-ips de=8
bridge-ip-versions v4=8,v6=0
bridge-ip-transports <OR>=8

For comparison, here's what some other bridge reported to Tonga:

dirreq-stats-end 2014-02-26 19:53:09 (86400 s)
dirreq-v3-resp ok=104,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=16,busy=0
dirreq-v3-direct-dl complete=0,timeout=0,running=0
dirreq-v3-tunneled-dl complete=84,timeout=12,running=4,min=1964,d1=6476,d2=29574,q1=42945,d3=55188,d4=63573,md=86563,d6=105689,d7=150325,q3=183340,d8=202945,d9=239672,max=353788
bridge-stats-end 2014-02-26 19:53:26 (86400 s)
bridge-ips ir=40,??=8,ae=8,ca=8,eg=8,gb=8,gr=8,id=8,in=8,lb=8,lu=8,mo=8,mx=8,nl=8,ph=8,pk=8,ru=8,sa=8,th=8,us=8
bridge-ip-versions v4=72,v6=0

The only difference is that dirreq-v3-ips and dirreq-v3-reqs are not empty lines anymore with this patch.

I think this should be safe to merge into master. Putting into needs_review again. (Thanks!)

comment:18 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merged!

Note: See TracTickets for help on using tickets.