[tor-bugs] #12538 [Tor]: Make all relays automatically be dir caches

[Tor Bug Tracker & Wiki]

#12538: Make all relays automatically be dir caches
-------------------------+-------------------------------------------------
     Reporter:           |      Owner:
  cypherpunks            |     Status:  needs_review
         Type:  task     |  Milestone:  Tor: 0.2.6.x-final
     Priority:  normal   |    Version:  Tor: unspecified
    Component:  Tor      |   Keywords:  tor-guard, tor-relay, prop237
   Resolution:           |  026-triaged-1
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+-------------------------------------------------

Comment (by sysrqb):

 Pushed bug12538_prop_squash_1, adding function rename and unit test.


 Replying to [comment:19 dgoulet]:
 > I'm wondering if it would be useful to have "is_directory_server()"
 function that could take a routerinfo_t and test that object to see if
 it's a directory instead of the "options" object parsing?
 >
 > It could then be used with "desc_routerinfo" object representing our
 router information thus fixing the semantic "issue".

 I considered taking a routerinfo_t as a parameter, but I decided against
 it because 1) I wanted to match the signature of server_mode() as closely
 as possible, and 2) this function is only called when a router must decide
 if it serves dir requests, so an or_options_t should be sufficient.

 Until this changes in the future, simply checking the is_v2_dir field of
 another router's routerinfo_t is all that is needed to determine if it is
 a dir server. I think it would be unnecessarily complicated otherwise, so
 I think it's okay that we have different interfaces for answering the
 questions "am I a directory server?" verses "is she a directory server?".

 >
 > Also, I'm a big fan of namespacing function with the file name so this
 function is in "router.c" and public thus maybe should be called
 "router_is_directory_server()" ? (unless there is a coding rules I'm not
 aware of)

 I agree, but I'll follow Nick's suggestions below in this case.


 Replying to [comment:20 nickm]:
 > - In set_routerstatus_from_routerinfo, shouldn't it also set is_v2_dir
 to true if ri->dir_port is nonzero? That's what the proposal says, I
 think.

 {{{
 @@ -2090,6 +2090,7 @@ set_routerstatus_from_routerinfo(routerstatus_t *rs,
    strlcpy(rs->nickname, ri->nickname, sizeof(rs->nickname));
    rs->or_port = ri->or_port;
    rs->dir_port = ri->dir_port;
 +  rs->is_v2_dir = ri->supports_tunnelled_dir_requests;
 }}}

 I don't think it's necessary because supports_tunnelled_dir_requests is
 only set when the descriptor is parsed in router_parse_entry_from_string
 and we set it true if either the dirport is set or "tunnelled-dir-server"
 was in the descriptor. Is there a case I'm missing?

 {{{
 +  /* This router accepts tunnelled directory requests via begindir if it
 has
 +   * an open dirport or it included "tunnelled-dir-server". */
 +  if (find_opt_by_keyword(tokens, K_DIR_TUNNELLED) || router->dir_port >
 0) {
 +    router->supports_tunnelled_dir_requests = 1;
 +  }
 }}}

 > - dir_server_mode() or running_as_dir_server() would be a better name
 for is_directory_server()
 Agreed, let's use the former so it matches server_mode() and
 public_server_mode().

 > - Do we have a test for router_pick_directory_server_impl()? If not,
 should we get one somehow?
 No, and yes.

 It's not a very elegant solution (directly including routerlist.c).
 Alternatively I can make router_pick_directory_server_impl and
 routerlist_insert STATIC or create TOR_UNIT_TESTS-specific wrappers around
 them. The existing non-static wrappers added unnecessary complexity
 considering I'm trying to test a single function, but if using a wrapper
 is more mergeable then I can expand the unit test so that will work. Which
 do you prefer?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12538#comment:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online