Opened 6 years ago

Closed 6 years ago

#10758 closed enhancement (implemented)

Stop serving version 2 directory information by default

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

Description

The third and last directory authority stopped serving version 2 directory information a week ago, and apparently the Tor network survived. We should make it the new default that directory authorities don't serve version 2 directory information, so that the three directory authorities don't need the "DisableV2DirectoryInfo_ 1" torrc lines anymore.

Child Tickets

Change History (12)

comment:1 Changed 6 years ago by karsten

Milestone: Tor: 0.2.5.x-final
Status: newneeds_review

Please find my branch bug10758 with the trivial fix.

comment:2 Changed 6 years ago by arma

Status: needs_reviewneeds_revision

That patch will trigger this code for all non-authorities:

  if (options->DisableV2DirectoryInfo_ && ! authdir_mode(options)) {
    REJECT("DisableV2DirectoryInfo_ set, but we aren't an authority.");
  }

so, please do not merge :)

Nick, what would you think about a more comprehensive patch that rips out the v2 directory stuff? Is that an 0.2.5 thing or an 0.2.6 thing?

(Since there aren't that many authorities, I don't think it's so critical to have an easily configurable "oops we'd better turn that on" option. If for some reason we end up changing our mind down the road, we can get patches to the directory authorities operators quickly enough.)

comment:3 in reply to:  2 Changed 6 years ago by karsten

Replying to arma:

That patch will trigger this code for all non-authorities:

  if (options->DisableV2DirectoryInfo_ && ! authdir_mode(options)) {
    REJECT("DisableV2DirectoryInfo_ set, but we aren't an authority.");
  }

so, please do not merge :)

Argh! Sorry, I totally missed that.

Nick, what would you think about a more comprehensive patch that rips out the v2 directory stuff? Is that an 0.2.5 thing or an 0.2.6 thing?

I thought about that, too. Want me to write such a patch, or fix the patch above? (I even promise to test either patch, which I thought wasn't necessary for the trivial patch I wrote earlier today.)

comment:4 Changed 6 years ago by nickm

Ripping out more v2 code is fine by me, on a merge-whenever-it's-done basis. That's assuming that the patch in question is pretty clean wrt the parts of the code that it isn't just ripping out.

comment:5 Changed 6 years ago by nickm

I started poking at this, and it's a nontrivial job. I'll have a patch soon

comment:6 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

See branch "bug10758" in my public repo. Please review soon; this code will rot.

In particular, I want to know whether the removed code in nodelist.c and in routerlist.c describe any functionality that we should really be reimplementing with consensus directories.

comment:7 Changed 6 years ago by arma

Ha -- put V1_DIRINFO and related on the chopping block after this one is resolved, yes?

comment:8 Changed 6 years ago by nickm

Yeah, assuming we can do so safely.

comment:9 Changed 6 years ago by karsten

Your branch bug10758 looks good. I tweaked it a bit in my force-updated bug10758 branch.

I also ran my bug10758 branch in a Shadow network with 20 relays which worked without obvious issues.

With respect to nodelist.c and routerlist.c, I'm not really sure which functionality you'd want to keep. Can you be more specific, e.g., which functions would you want to reimplement for v3?

comment:10 in reply to:  9 ; Changed 6 years ago by nickm

Replying to karsten:

Your branch bug10758 looks good. I tweaked it a bit in my force-updated bug10758 branch.

Looks okay to me.

I also ran my bug10758 branch in a Shadow network with 20 relays which worked without obvious issues.

Okay. That's not likely to turn up all possible errors, since any weird stuff that breaks is going to break because of:

  • Older versions of Tor
  • Nodes what not all authorities find out about
  • Directories going up and down
  • Other stuff

But it's better than no testing at all.

With respect to nodelist.c and routerlist.c, I'm not really sure which functionality you'd want to keep. Can you be more specific, e.g., which functions would you want to reimplement for v3?

Well, I'm mostly hoping for a review of all the complex pieces and subtle little lines there to make sure they didn't do something crucial that wasn't related to the v2 networkstatus system.

Here are some examples:

  • v2_ns_dl_status didn't have some extra unexpected meaning, did it?
  • Did anything bad just happen to routerlist_remove_old_routers?

I think the answers are "no", but that was some complex logic I just chopped out.

comment:11 in reply to:  10 Changed 6 years ago by karsten

Replying to nickm:

Well, I'm mostly hoping for a review of all the complex pieces and subtle little lines there to make sure they didn't do something crucial that wasn't related to the v2 networkstatus system.

Here are some examples:

  • v2_ns_dl_status didn't have some extra unexpected meaning, did it?
  • Did anything bad just happen to routerlist_remove_old_routers?

I think the answers are "no", but that was some complex logic I just chopped out.

I just took another look at the two examples and once more at the entire patch. I didn't spot anything in the removed v2 code or changed functions that would break v3. But obviously, that doesn't necessarily mean there is no such code. A fresh pair of eyes would probably be much more reliable than mine.

comment:12 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I've looked through again and can't find anything either. Merging this to master.

Note: See TracTickets for help on using tickets.