[tor-bugs] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-------------------------------------------------+-------------------------
 Reporter:  toralf                               |          Owner:
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_review
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |        Version:  Tor:
  TorCoreTeam201604                              |  0.2.8.1-alpha
Parent ID:                                       |     Resolution:
 Reviewer:  dgoulet                              |  Actual Points:  16
                                                 |  hours
                                                 |         Points:  medium
                                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  needs_revision => needs_review
 * reviewer:   => dgoulet
 * points:   => medium
 * actualpoints:   => 16 hours


Comment:

 I think part of this bug was due to #18623, and the logging is being
 tracked in #18849.

 This leaves the fixes for #12538 and #18050:

 Replying to [comment:18 teor]:
 > I think I can update the patch so it cleanly answers the following
 questions:
 > * do we have a dirport open?

 get_options()->DirPort_set needs no changes.

 >   * if we have a dirport open, is the DirPort reachable?

 check_whether_dirport_reachable() returns 0 when we need to do a
 reachability check,
 and 1 if we've successfully done a reachability check, or if there's no
 reason to do a reachability check. It needs changes for clarity.

 004e1c27 refactors common parts of the ORPort and DirPort
 check_*_reachable functions into router_reachability_checks_disabled(),
 and updates the function comments. It disables OR reahcability checks when
 net_is_disabled(), for consistency with Dir reachability checks.

 #18851 updates the control-spec to clarify this behaviour.

 >     * if the DirPort is reachable, do we want to advertise it?

 decide_to_advertise_dirport() needs no changes.

  * do we have an orport open?

 get_options()->ORPort_set needs no changes.

 > * would we answer begindir requests?

 directory_permits_begindir_requests() needs no changes, but should be used
 to set supports_tunnelled_dir_requests. (See 4dda75fc below.)

 This resolves a nasty edge case with bridges, which should always support
 begindir.

 >   * ~~if we would answer begindir requests,~~ is the ORPort reachable?

 check_whether_orport_reachable() is refactored in 004e1c27, see notes
 above

 >     * if the ORPort is reachable, do we want to advertise begindir
 support?

 4dda75fc creates decide_to_advertise_begindir(), like
 decide_to_advertise_dirport(), which should clean up a lot of nasty edge
 cases:
 * directory authorities should always support and advertise begindir,
 * ORs with disabled networks shouldn't advertise support for begindir
 (possibly not an issue, but done for consistency with DirPort),
 * ORPort reachability is required for advertisement (possibly not an
 issue, but done for consistency with DirPort),
 * we now call router_should_be_directory_server() once per descriptor
 upload, which I hope satisfies nickm's request to call it infrequently
 (NM1).

 > As nickm notes in comment 11, I need to make sure we do the reachability
 and advertisability checks, store the results, and then use the results to
 answer these questions.

 NM1: This now happens once per descriptor upload, which means tor only
 logs when actually making the decision for each descriptor.

 > As dgoulet notes in 13 & 15, I should also remove redundant function
 calls.

 DG1: I don't believe there are any redundant function calls in this patch.

 Also, a28d98f4 adds the options used by these functions to the list of
 options we check to see if we need to rebuild our descriptor.

 Finally, we need to update our descriptor when we change our mind about
 advertising the dirport or begindir support due to accounting max. We
 didn't do this in the past for DirPort, and we do it every 18 hours
 anyway, so I'm splitting it off into #18852 in case we want to defer it to
 0.2.9 or later.

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