[tor-bugs] #13192 [Tor]: Collect aggregate stats of total hidden service usage vs total exit usage in Tor network

[Tor Bug Tracker & Wiki]

#13192: Collect aggregate stats of total hidden service usage vs total exit usage
in Tor network
-----------------------------+---------------------------------
     Reporter:  arma         |      Owner:
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:  Tor: 0.2.???
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  SponsorR, tor-relay
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+---------------------------------

Comment (by karsten):

 Replying to [comment:13 dgoulet]:
 > Here is what I can find "code wise":
 >
 > commit 6006c4abe884c2084f98404d06669836a77c9f49
 > * add_laplace_noise() uses sample_laplace_distribution() with a "long"
 value which might be very problematic if a double is expected. You
 basically loose the float part. If this is the intended goal, I would put
 a comment explaining why since "long" and "double" are quite different.

 Yes, it's intended.  Added a short comment.

 > * NAN is a GNU extension which might be problematic without a fallback
 on BSD or/and Windows. I'm not to familiar with that macro outside Linux.

 As discussed in #tor-dev, let's just do tor_assert() instead of returning
 NAN.

 > * +round_long_to_next_multiple_of(): probably a typo but "unsigned
 divisor", is a "long" or "int" is missing here? It's allowed but I really
 think that types should be *always* specified, just for the sake of the
 reviewers :).

 In fact, it's probably even better to use the same type for number and
 divisor.  Changed to long.

 > * round_long_to_next_multiple_of() will "Floating point exception" if
 "divisor" is 0. Modulo with 0 is not really a good thing :). In this case,
 I was able to make it coredump with number == LONG_MIN and divisor set to
 0.

 Added another tor_assert().

 > commit 6c3263b5b56e0b6e953226af240f2f607a2fa415
 > * I see that there are multiple places where "hs_stats" is checked and
 if NULL, it's allocated. Shouldn't we call "rep_hist_hs_stats_init()"
 instead? Seems to me that having the start interval time set is important
 when initializing the digestmap.

 Indeed.  But we can't initialize stats there, because we don't have a
 timestamp.  The better idea is probably to ignore observations when we're
 not collecting stats.  Changed.

 > * rep_hist_hs_stats_init(): double space:
 > {{{
 > if  (!hs_stats) {
 > }}}

 Fixed.

 > The rest seems fine to me for now.

 Great!  Thanks for the review, and feel free to review the new changes.
 Pushing new commits to task-13192-2 after replying to asn below.

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