Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2497 closed defect (fixed)

Some relays without dirport have dirreq-(write|read)-history in their extrainfo desc

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

Description

Child Tickets

Change History (17)

comment:1 Changed 8 years ago by karsten

Cc: karsten added
Status: newassigned

comment:2 Changed 8 years ago by karsten

qbi found another instance of this problem:
https://metrics.torproject.org/descriptor.html?desc-id=19fa7b23

comment:3 Changed 8 years ago by Sebastian

I think I understand what's going on. If your relay has a dirport configured, but is also configured to push less than 50KB/s, we don't advertise a dirport but we still internally act like being a dir mirror. Both cases above have a bandwidth of exactly 51200, and 51201 is required to advertise being a dir mirror.

Is this still a bug worth fixing/a bug at all? Does it cause problems?

comment:4 Changed 8 years ago by karsten

Nice analysis! Sounds plausible.

And yes, I think this bug is worth fixing, because empty bandwidth histories from a few weeks in the past will be confusing for people analyzing the descriptor archives.

In theory, it should be sufficient to make sure that bandwidth history lines have at least 1 value in them. If they have not, we can simply omit them.

comment:5 in reply to:  4 Changed 8 years ago by Sebastian

Status: assignedneeds_review

Ok this all has nothing to do with setting a DirPort or not or how much bandwidth you're pushing (occasionally it pays off to read code before speculating). Every relay since 0.2.2.15 publishes dirreq-write-history and dirreq-read-history, whether they have a dirport configured or not. See rep_hist_get_bandwidth_lines() in rephist.c for details. This is going to be a common case. Do we still want to fix it? :-)

comment:6 Changed 8 years ago by Sebastian

Owner: set to karsten
Status: needs_reviewassigned

eh, what the hell, I didn't want to put this into needs-review

comment:7 Changed 8 years ago by karsten

Yes, I think this bug is worth fixing for the reason stated in my last comment. Why do you think it's not worth fixing?

comment:8 Changed 8 years ago by Sebastian

Ok. Next question, it is possible that a 0 is reported. Should this also be stripped, or only empty strings?

comment:9 Changed 8 years ago by karsten

It's possible that a relay reports 0 for one or more intervals. But I think in this case we only want to omit empty strings.

comment:10 Changed 8 years ago by Sebastian

Status: assignedneeds_review

Have this implemented in branch bug2497. I call it a feature, because the old format wasn't broken. This was tested with a relay running the new code that just started up (no -history lines reported) and a new relay that just started up with a state file from a previously active relay (-history lines get reported).

comment:11 Changed 8 years ago by Sebastian

Milestone: Tor: 0.2.2.x-final

We should put this in before the first 0.2.2.x rc, because otherwise we'll have more of those empty lines in old descriptors

comment:12 Changed 8 years ago by Sebastian

pushed update for a silly mistake spotted by karsten. should be bd315e34 now.

comment:13 Changed 8 years ago by karsten

Looks good to me. Thanks!

comment:14 Changed 8 years ago by Sebastian

Pushed another update to fix a bug introduced when looking at karsten's comment. Should be at c1927d7d5 now. This new patch is tested again.

comment:15 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged ; closing; thanks!

comment:16 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:17 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.