Opened 4 years ago

Last modified 23 months ago

#17903 needs_revision enhancement

router_pick_trusteddirserver_impl should distinguish between fallbacks and authorities

Reported by: teor Owned by: attila
Priority: Very Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Minor Keywords: review-group-18
Cc: #17908 Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

router_pick_directory_server_impl currently prioritises directories in the following order:

  • tunneled
    • mirrors
    • overloaded mirrors
    • authorities (trusted)
  • direct
    • mirrors
    • overloaded mirrors
    • authorities (trusted)

But router_pick_trusteddirserver_impl uses the following order:

  • tunneled
    • fallbacks + authorities
    • overloaded fallbacks + authorities
  • direct
    • fallbacks + authorities
    • overloaded fallbacks + authorities

I suggest we change it to:

  • tunneled
    • fallbacks
    • overloaded fallbacks
    • authorities
  • direct
    • fallbacks
    • overloaded fallbacks
    • authorities

We should do this for consistency, and to help with #17847 (unifying these two functions).

(This doesn't make much practical difference, because authorities are weighted 1.0, and fallbacks are weighted ~100,000.0, so authorities get selected very rarely. However, this would modify the effect of the DirAuthorityFallbackRate torrc option.)

If we do this, let's refactor the code so it's all in one place, probably using a macro.

Child Tickets

Attachments (1)

0001-Ticket-17903-taken-up-as-the-coding-task-for-the-int.patch (16.2 KB) - added by attila 2 years ago.
patch from git format-patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by teor

Cc: #17908 added; #17847 removed
Priority: MediumVery Low
Severity: NormalMinor

This affects the stats calculations in #17907 and #17908.

That said, it doesn't make much practical difference in 0.2.8. It would be nice for refactoring, but we can defer it as long as we like.

comment:2 Changed 4 years ago by teor

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

comment:3 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:4 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

Changed 2 years ago by attila

patch from git format-patch

comment:5 Changed 2 years ago by attila

I just attached a patch for this to complete the coding task portion of the Core Developer interview process.

comment:6 Changed 2 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.1.x-final

comment:7 Changed 2 years ago by nickm

Status: newneeds_review

comment:8 Changed 2 years ago by nickm

Owner: set to attila
Status: needs_reviewassigned

setting "owner" field on trac.

comment:9 Changed 2 years ago by nickm

Status: assignedneeds_review

comment:10 Changed 2 years ago by nickm

Reviewer: nickm

comment:11 Changed 2 years ago by nickm

Keywords: review-group-17 added

comment:12 Changed 2 years ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:13 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 2 years ago by nickm

Keywords: review-group-17 removed

comment:15 Changed 2 years ago by nickm

Keywords: review-group-18 added

comment:16 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

Eek, the attached patch is way too heavy on the macro side. I feel like the resulting code is much harder to read/review even tho there is less code dup.

Let's try to come up with a solution here that uses functions. It's far from trivial but I don't think it's impossible, and it will result in more maintainable code IMO.

Moving this to needs_revision for now, but feel free to switch it back if you disagree with my evaluation.

comment:17 Changed 23 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark some needs_revision tickets as unspecified. If/when the revisions happen, they can go back into a live milestone.

Note: See TracTickets for help on using tickets.