Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#22310 closed defect (implemented)

Tor 0.3.x clients won't use Guard-flagged relays as Guards if they don't have V2Dir, throwing off consensus position weights

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: 032-unreached, 031-backport-maybe, 032-backport-maybe, 033-triage-20180320, 033-removed-20180320, fast-fix, 033-roadmap-proposed
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:


Starting in Tor 0.3.0, clients have additional constraints on what counts as a usable guard. Specifically, node_is_possible_guard() now demands node_is_dir(node) before it will consider node as a guard.

But the path position weights in the consensus are based on the Guard flag.

This inconsistency will lead to Guards that don't have the V2Dir flag being way underused, and Guards that do have the V2Dir flag being overused.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by arma

One fix we should consider is simply not assigning the Guard flag if we aren't going to assign the V2Dir flag. Consider:

$ grep "^s " cached-consensus |grep Guard|wc -l
$ grep "^s " cached-consensus |grep Guard|grep -v "V2Dir"|wc -l

I haven't added up the capacity/speed/bandwidth/whatever of these relays for comparison though.

But since they essentially already *aren't* guards for Tor 0.3 and later clients, maybe that makes the choice easier?

comment:2 Changed 3 years ago by arma

If we go this route, we might want to move the milestone up to e.g. Tor 0.3.1, so authorities will do the new behavior sooner, since Tor 0.3.0 clients are already doing their part of the problem.

comment:3 Changed 3 years ago by arma

Here's the simple patch:

diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 2b10a09..c79bdfd 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -2190,6 +2190,7 @@ set_routerstatus_from_routerinfo(routerstatus_t *rs,
   rs->is_valid = node->is_valid;
   if (node->is_fast && node->is_stable &&
+      ri->supports_tunnelled_dir_requests &&
       ((options->AuthDirGuardBWGuarantee &&
         routerbw_kb >= options->AuthDirGuardBWGuarantee/1000) ||
        routerbw_kb >= MIN(guard_bandwidth_including_exits_kb,

I expect that updating the specs and documentation and so on will be a bit more involved.

Also, in a later Tor, we'll want to refactor so we calculate rs->is_v2_dir higher up, and then just use that value in the 'if'. Surely that refactoring won't break anything, but the above diff is still an easier sell.

comment:4 Changed 3 years ago by arma

Also also, once the authorities are doing this new behavior, we should consider teaching clients to assume that all Guards are automatically able to be directory mirrors, so we're not stuck dragging around a consensus flag albatross that doesn't even refer to the current version of the directory protocol. :)

(But, one step at a time.)

comment:5 Changed 3 years ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:6 Changed 3 years ago by arma

I think we should push this fix so the dir auths will have it. I've been running it on moria1 since I made this ticket, but I'm not enough by myself.

comment:7 Changed 3 years ago by teor

Keywords: 031-backport-maybe 032-backport-maybe added
Milestone: Tor: unspecifiedTor: 0.3.3.x-final
Points: 0.1
Version: Tor:

Thinking is not enough. You must tag.

comment:8 Changed 3 years ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:9 Changed 3 years ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:10 Changed 3 years ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

comment:11 in reply to:  3 Changed 3 years ago by arma

Owner: set to arma
Status: newassigned

Replying to arma:

Here's the simple patch:

Ok, my bug22310 branch implements this one-liner.

I expect that updating the specs and documentation and so on will be a bit more involved.

I have pushed a bug22310 branch to my torspec too, which adds this constraint to the definition of the Guard flag.

comment:12 Changed 3 years ago by arma

Keywords: fast-fix 033-roadmap-proposed added
Status: assignedneeds_review

Scheduling this ticket to get attention / discussion in the 033 timeframe.

comment:13 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:14 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

lgtm; merging to 0.3.3 and forward.

comment:15 Changed 3 years ago by arma

Ok, I merged my torspec branch too, on the theory that otherwise we'd leave it behind and forgotten.

Note: See TracTickets for help on using tickets.