[tor-bugs] #24104 [Core Tor/Tor]: Delay descriptor bandwidth reporting on large relays

[Tor Bug Tracker & Wiki]

#24104: Delay descriptor bandwidth reporting on large relays
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  juga
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.5.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  unspecified
 Severity:  Normal                               |     Resolution:
 Keywords:  034-backport-maybe, 033-backport-    |  Actual Points:
  maybe, 032-backport-maybe, 031-backport-       |
  maybe, 029-backport-maybe, security-low,       |
  guard-discovery-stats, chutney-wants, bwauth-  |
  wants, 034-triage-20180328,                    |
  034-removed-20180328, tor-bwauth               |
Parent ID:  #25925                               |         Points:  1
 Reviewer:  ahf                                  |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by arma):

 I continue to like the general idea of this patch.

 Some code review suggestions:

 In the changes file, it says "bigger than 24h" but I think we mean
 "smaller than 24h".

 Juga, you might want to add a config line to your editor that complains to
 you about lines with trailing whitespace. There are some in test_router.c.

 This is a lot of fixups (which is fine), so somebody might want to squash
 it before the merge.

 The change as done here will mean we stop publishing a new descriptor when
 we go hibernating (see how check_descriptor_bandwidth_changed() checks
 we_are_hibernating() and sets cur to 0 if so). One simple fix would be for
 the function to still proceed if we_are_hibernating(), so we still let
 everybody know when we start hibernating. But another fix that I like a
 bit more would be to add a separate mark_my_descriptor_dirty() call when
 we decide to start hibernating... on more thought though, that should be a
 separate fix ("publish a new descriptor explicitly when we begin/end
 hibernating") that can be done separately from this fix and shouldn't
 delay it. So I resume liking the "proceed if we_are_hibernating()" idea as
 the simplest fix.

 Speaking of hibernating, I also checked if this change makes us not
 publish a descriptor when we wake up from hibernating. I think it's ok,
 because see how hibernate_end() calls reset_uptime(), so now
 check_descriptor_bandwidth_changed() will be willing to proceed. All the
 more reason to open the "explicitly mark descriptor dirty when
 beginning/ending hibernation, rather than implicitly doing it as part of
 the bandwidth check" ticket.

 We also reset_uptime(), and thus become willing to publish descriptors
 more readily for bandwidth changes, when our external IP address changes.
 I think that's ok, since it should not be an easy thing for an attacker to
 induce.

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