[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:9 asn]:
 > Nice code!

 Yes, I think we wrote some good code there together.

 > Small review:
 >
 > * I need to think more about the overflow protection in
 `round_long_to_next_multiple_of()`. I'm not sure what happens if there is
 actually an overflow there and that line is skipped.

 Yes, please do!  I spent quite some time with paper and pencil here to go
 through the possible edge cases.  And then I wrote unit tests for all of
 them that came to mind.  But it's not at all unrealistic that I missed an
 important case.

 > * Where can I find the formula you are using in
 `transform_uniform_random_to_laplace()`?  Also, maybe we could rename that
 function to `sample_laplace_distribution()` or something.

 From [http://en.wikipedia.org/wiki/Laplace_distribution Wikipedia]! :)
 I'll include a comment.  I'm also renaming the function as suggested.

 > * When you do `digestmap_free(hs_stats->onions_seen_this_period, NULL)`
 do the elements of the digestmap get freed? I think you need to pass the
 `tor_free_()` func as the second argument?

 I think freeing elements would actually be harmful, because we're just
 storing void pointers (`(void*)(uintptr_t)1`) in the map.  We cannot
 dereference those pointers and free whatever we find at that address.
 Should we ask a C programmer to get this confirmed?

 > * ` This value, multiplied with EPSILON, is Laplace parameter b. */` I
 think this is divided not multiplied.

 Err, yes.  You mentioned that on IRC and totally wanted to change it.
 Changed now.

 > * An interesting consequence of rounding up negative numbers is that a
 result of 0 doesn't mean that the underlying value was also 0. In our case
 it can be anything from -7 to 0. This is not something bad but something
 to keep in mind.

 Agreed that it's potentially confusing, but it's correct, AFAICT.

 > * BTW, wrt to this unittest
 `tt_assert(round_long_to_next_multiple_of(LONG_MIN,2) == LONG_MIN)`. It is
 the case in all platforms that `LONG_MIN` is even, right?

 Interesting question.  I'm pretty sure.  But even if not, the worst thing
 that can happen is that our unit tests break.

 I made two more changes: I documented the new config option in the man
 page as discussed in #tor-dev, and I changed the default config value to 0
 for two reasons: we should avoid that somebody accidentally turns on these
 statistics when running our branch; and we must avoid that we accidentally
 merge the wrong default value into master.

 Branch task-13192 contains the new changes as additional commits, and
 branch task-13192-2 is heavily rebased following the plan described above.

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