Opened 5 years ago

Closed 4 years ago

Last modified 14 months ago

#13192 closed enhancement (fixed)

Collect aggregate stats of total hidden service usage vs total exit usage in Tor network

Reported by: arma Owned by: teor
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Normal Keywords: tor-relay, tor-hs, 028-triaged, pre028-patch, TorCoreTeam201511
Cc: asn Actual Points:
Parent ID: Points: small
Reviewer: Sponsor: SponsorR

Description

I'd like to know what fraction of total Tor usage is hidden service usage, so we have a sense of whether hidden services matter now, and so we can track trends into the future.

For example, it would have been nice in August 2013 to have some metric of hidden service fraction that told us the spike in load and users had to do with hidden services.

Such statistics would also be useful to counter (or who knows, confirm) the analysts who say statements like "97% of Tor use is silk road".

Child Tickets

TicketTypeStatusOwnerSummary
#13193enhancementclosedTag circuits locally by exit vs hidden-service, for stats

Attachments (6)

cells-types.png (27.2 KB) - added by karsten 5 years ago.
Cells handled by circuit position/type
cells-hs.png (21.8 KB) - added by karsten 5 years ago.
Fraction of cells seen on hidden-service circuits
plot.R (1.8 KB) - added by karsten 5 years ago.
R script to plot stats of hidden service usage in the Tor network
0001-CLASSIFIED-Log-statements-and-XXX-comments.patch (2.0 KB) - added by karsten 5 years ago.
hidserv-rend-relayed-cells-2014-12-26.png (45.3 KB) - added by karsten 5 years ago.
hidserv-dir-onions-seen-2014-12-26.png (48.9 KB) - added by karsten 5 years ago.

Download all attachments as: .zip

Change History (92)

comment:1 Changed 5 years ago by arma

My hs-stats branch implements this. I made it print out aggregate stats every thirty minutes, of the form

Sep 23 22:30:00.851 [notice] Heartbeat: 0|0 exit / 9|229 hidden service / 504|22963 middle circuits / 0|1000 other circuits handled.

That's 9 circuits that were hidden service related (and 229 cells on those), and 504 circuits that we saw an extend cell on (and 22963 cells on those).

comment:2 Changed 5 years ago by arma

We should discard the first entry since it's not a full 30 minute period. But after that here's what moria5 has said:

Sep 23 23:00:00.203 [notice] Heartbeat: 0|0 exit / 36|3327 hidden service / 1767|263266 middle circuits / 0|0 other circuits handled.
Sep 23 23:30:00.431 [notice] Heartbeat: 0|0 exit / 24|83 hidden service / 1765|295828 middle circuits / 0|0 other circuits handled.
Sep 24 00:00:00.599 [notice] Heartbeat: 0|0 exit / 24|15840 hidden service / 1890|354877 middle circuits / 0|0 other circuits handled.
Sep 24 00:30:00.820 [notice] Heartbeat: 0|0 exit / 25|17192 hidden service / 1772|148523 middle circuits / 0|3 other circuits handled.
Sep 24 01:00:00.173 [notice] Heartbeat: 0|0 exit / 31|31553 hidden service / 2100|490355 middle circuits / 0|5 other circuits handled.
Sep 24 01:30:00.334 [notice] Heartbeat: 0|0 exit / 32|2047 hidden service / 1966|149403 middle circuits / 0|0 other circuits handled.
Sep 24 02:00:00.472 [notice] Heartbeat: 0|0 exit / 25|70 hidden service / 2039|196771 middle circuits / 0|0 other circuits handled.
Sep 24 02:30:00.854 [notice] Heartbeat: 0|0 exit / 36|5508 hidden service / 1850|104266 middle circuits / 0|0 other circuits handled.
Sep 24 03:00:00.115 [notice] Heartbeat: 0|0 exit / 29|1775 hidden service / 2025|282774 middle circuits / 0|0 other circuits handled.
Sep 24 03:30:00.299 [notice] Heartbeat: 0|0 exit / 29|3865 hidden service / 2063|403588 middle circuits / 0|0 other circuits handled.
Sep 24 04:00:00.482 [notice] Heartbeat: 0|0 exit / 32|947 hidden service / 2125|101653 middle circuits / 0|0 other circuits handled.
Sep 24 04:30:00.601 [notice] Heartbeat: 0|0 exit / 41|3962 hidden service / 2190|505420 middle circuits / 0|0 other circuits handled.
Sep 24 05:00:00.797 [notice] Heartbeat: 0|0 exit / 54|2557 hidden service / 2143|288832 middle circuits / 0|0 other circuits handled.
Sep 24 05:30:00.033 [notice] Heartbeat: 0|0 exit / 36|628 hidden service / 2097|395592 middle circuits / 0|8 other circuits handled.
Sep 24 06:00:00.169 [notice] Heartbeat: 0|0 exit / 47|5555 hidden service / 2040|426657 middle circuits / 0|3 other circuits handled.
Sep 24 06:30:00.271 [notice] Heartbeat: 0|0 exit / 51|7954 hidden service / 2035|300051 middle circuits / 0|0 other circuits handled.
Sep 24 07:00:00.443 [notice] Heartbeat: 0|0 exit / 63|2529 hidden service / 2217|118397 middle circuits / 0|0 other circuits handled.
Sep 24 07:30:00.747 [notice] Heartbeat: 0|0 exit / 43|533 hidden service / 2267|102326 middle circuits / 0|0 other circuits handled.
Sep 24 08:00:00.872 [notice] Heartbeat: 0|0 exit / 37|236 hidden service / 2204|297865 middle circuits / 0|0 other circuits handled.
Sep 24 08:30:00.173 [notice] Heartbeat: 0|0 exit / 29|280 hidden service / 2209|118515 middle circuits / 0|0 other circuits handled.
Sep 24 09:00:00.251 [notice] Heartbeat: 0|0 exit / 31|42 hidden service / 2394|454070 middle circuits / 0|3 other circuits handled.
Sep 24 09:30:00.424 [notice] Heartbeat: 0|0 exit / 35|2447 hidden service / 2376|363384 middle circuits / 0|0 other circuits handled.
Sep 24 10:00:00.662 [notice] Heartbeat: 0|0 exit / 44|1941 hidden service / 2376|239185 middle circuits / 0|6 other circuits handled.
Sep 24 10:30:00.054 [notice] Heartbeat: 0|0 exit / 34|2704 hidden service / 2252|174835 middle circuits / 0|0 other circuits handled.
Sep 24 11:00:00.342 [notice] Heartbeat: 0|0 exit / 30|2692 hidden service / 2374|678771 middle circuits / 0|0 other circuits handled.
Sep 24 11:30:00.477 [notice] Heartbeat: 0|0 exit / 34|448 hidden service / 2382|416790 middle circuits / 0|11 other circuits handled.

comment:3 Changed 5 years ago by mo

9EA317EECA56BDF30CAEB208A253FB456EDAB1A0:

Sep 23 20:30:00.000 [notice] Heartbeat: 14644|26045959 exit / 367|31023 hidden service / 11303|1235370 middle circuits / 0|3826 other circuits handled.
Sep 23 20:30:00.000 [notice] Heartbeat: 28 descs (23/24 stored, 0/135 fetched).
Sep 23 21:00:00.000 [notice] Heartbeat: 14691|37563962 exit / 387|25758 hidden service / 12037|1252763 middle circuits / 0|3658 other circuits handled.
Sep 23 21:00:00.000 [notice] Heartbeat: 42 descs (25/25 stored, 0/71 fetched).
Sep 23 21:30:00.000 [notice] Heartbeat: 14997|48041362 exit / 365|28070 hidden service / 11450|1117559 middle circuits / 0|3665 other circuits handled.
Sep 23 21:30:00.000 [notice] Heartbeat: 42 descs (9/9 stored, 0/33 fetched).
Sep 23 22:00:00.000 [notice] Heartbeat: 15206|45696391 exit / 344|142841 hidden service / 13356|1139594 middle circuits / 0|2790 other circuits handled.
Sep 23 22:00:00.000 [notice] Heartbeat: 42 descs (4/4 stored, 0/9 fetched).
Sep 23 22:30:00.000 [notice] Heartbeat: 15345|46783407 exit / 414|31628 hidden service / 13855|2974371 middle circuits / 0|4011 other circuits handled.
Sep 23 22:30:00.000 [notice] Heartbeat: 24 descs (0/0 stored, 0/2 fetched).
Sep 23 23:00:00.000 [notice] Heartbeat: 15055|52723222 exit / 335|57873 hidden service / 14119|1898492 middle circuits / 0|2713 other circuits handled.
Sep 23 23:00:00.000 [notice] Heartbeat: 24 descs (0/2 stored, 0/1 fetched).
Sep 23 23:30:00.000 [notice] Heartbeat: 15123|50176949 exit / 356|25526 hidden service / 13676|2513458 middle circuits / 0|2734 other circuits handled.
Sep 24 00:00:00.000 [notice] Heartbeat: 15383|50273567 exit / 304|9550 hidden service / 14350|1881783 middle circuits / 0|3052 other circuits handled.
Sep 24 00:00:00.000 [notice] Heartbeat: 24 descs (0/0 stored, 0/1 fetched).
Sep 24 00:30:00.000 [notice] Heartbeat: 15561|48178540 exit / 346|18596 hidden service / 15382|2623593 middle circuits / 0|4041 other circuits handled.
Sep 24 00:30:00.000 [notice] Heartbeat: 12 descs (0/1 stored, 0/1 fetched).
Sep 24 01:00:00.000 [notice] Heartbeat: 15408|46141801 exit / 357|28458 hidden service / 15613|2610501 middle circuits / 0|4218 other circuits handled.
Sep 24 01:30:00.000 [notice] Heartbeat: 15833|53094099 exit / 367|14079 hidden service / 15135|1817266 middle circuits / 0|2699 other circuits handled.
Sep 24 01:30:00.000 [notice] Heartbeat: 12 descs (0/0 stored, 0/1 fetched).
Sep 24 01:53:16.000 [notice] Heartbeat: Tor's uptime is 6:00 hours, with 10168 circuits open. I've sent 309.99 GB and received 302.63 GB.
Sep 24 02:00:00.000 [notice] Heartbeat: 15986|54911439 exit / 455|50049 hidden service / 15643|1520665 middle circuits / 0|2935 other circuits handled.
Sep 24 02:30:00.000 [notice] Heartbeat: 16301|55709588 exit / 501|52725 hidden service / 15195|1525046 middle circuits / 0|2535 other circuits handled.
Sep 24 02:30:00.000 [notice] Heartbeat: 12 descs (0/1 stored, 0/1 fetched).
Sep 24 03:00:00.000 [notice] Heartbeat: 16391|59900728 exit / 434|32352 hidden service / 14803|1664207 middle circuits / 0|3756 other circuits handled.
Sep 24 03:30:00.000 [notice] Heartbeat: 16780|62899539 exit / 410|19157 hidden service / 14966|1419477 middle circuits / 0|2292 other circuits handled.
Sep 24 03:30:00.000 [notice] Heartbeat: 12 descs (0/0 stored, 0/1 fetched).
Sep 24 04:00:00.000 [notice] Heartbeat: 16703|61367287 exit / 517|179502 hidden service / 16636|1625677 middle circuits / 0|4445 other circuits handled.
Sep 24 04:30:00.000 [notice] Heartbeat: 16803|65230689 exit / 446|240807 hidden service / 16398|1653825 middle circuits / 0|2558 other circuits handled.
Sep 24 05:00:00.000 [notice] Heartbeat: 17217|67110689 exit / 489|236797 hidden service / 15897|2269627 middle circuits / 0|6257 other circuits handled.
Sep 24 05:30:00.000 [notice] Heartbeat: 17757|70544960 exit / 409|61233 hidden service / 17133|2051072 middle circuits / 0|4161 other circuits handled.
Sep 24 06:00:00.000 [notice] Heartbeat: 17851|67761321 exit / 470|43343 hidden service / 17573|1948361 middle circuits / 0|4500 other circuits handled.

183AE7AEAD2A2FA0E8690DF0AD7C5532A70A8015:

Sep 23 20:30:00.000 [notice] Heartbeat: 14252|26794663 exit / 234|145667 hidden service / 11280|964837 middle circuits / 0|2331 other circuits handled.
Sep 23 21:00:00.000 [notice] Heartbeat: 14337|36653085 exit / 256|40122 hidden service / 12085|1093629 middle circuits / 0|2153 other circuits handled.
Sep 23 21:30:00.000 [notice] Heartbeat: 14577|41420660 exit / 243|68045 hidden service / 11412|1562953 middle circuits / 0|3094 other circuits handled.
Sep 23 22:00:00.000 [notice] Heartbeat: 14545|42281082 exit / 213|33078 hidden service / 13169|1517675 middle circuits / 0|4005 other circuits handled.
Sep 23 22:30:00.000 [notice] Heartbeat: 14871|49168386 exit / 259|44492 hidden service / 13293|3079992 middle circuits / 0|2498 other circuits handled.
Sep 23 23:00:00.000 [notice] Heartbeat: 14257|45394265 exit / 252|108238 hidden service / 13958|2544678 middle circuits / 0|3726 other circuits handled.
Sep 23 23:30:00.000 [notice] Heartbeat: 14125|51373860 exit / 260|16144 hidden service / 13262|1377293 middle circuits / 0|2189 other circuits handled.
Sep 24 00:00:00.000 [notice] Heartbeat: 14631|52032584 exit / 248|14136 hidden service / 13969|2755744 middle circuits / 0|2641 other circuits handled.
Sep 24 00:30:00.000 [notice] Heartbeat: 15905|53860801 exit / 206|63695 hidden service / 14787|2316633 middle circuits / 0|2468 other circuits handled.
Sep 24 01:00:00.000 [notice] Heartbeat: 15942|49168185 exit / 395|10177 hidden service / 15095|2647601 middle circuits / 0|5554 other circuits handled.
Sep 24 01:30:00.000 [notice] Heartbeat: 16614|54434242 exit / 225|74776 hidden service / 14361|2091028 middle circuits / 0|3926 other circuits handled.
Sep 24 02:00:00.000 [notice] Heartbeat: 16414|56352176 exit / 394|48253 hidden service / 14596|2304062 middle circuits / 0|4863 other circuits handled.
Sep 24 02:30:00.000 [notice] Heartbeat: 15741|57371390 exit / 416|30079 hidden service / 14447|2381328 middle circuits / 0|12788 other circuits handled.
Sep 24 03:00:00.000 [notice] Heartbeat: 15670|54439304 exit / 311|86512 hidden service / 14253|1678004 middle circuits / 0|3515 other circuits handled.
Sep 24 03:30:00.000 [notice] Heartbeat: 15990|53917336 exit / 316|175652 hidden service / 14517|3348468 middle circuits / 0|3672 other circuits handled.
Sep 24 04:00:00.000 [notice] Heartbeat: 15837|62045082 exit / 410|30264 hidden service / 15758|3256207 middle circuits / 0|5044 other circuits handled.
Sep 24 04:30:00.000 [notice] Heartbeat: 16012|53083712 exit / 337|38214 hidden service / 15712|3140300 middle circuits / 0|2970 other circuits handled.
Sep 24 05:00:00.000 [notice] Heartbeat: 16664|54400751 exit / 343|14416 hidden service / 15377|2946644 middle circuits / 0|4244 other circuits handled.
Sep 24 05:30:00.000 [notice] Heartbeat: 17009|60040078 exit / 311|16893 hidden service / 16515|3097233 middle circuits / 0|6800 other circuits handled.
Sep 24 06:00:00.000 [notice] Heartbeat: 17534|59438816 exit / 361|8894 hidden service / 16929|2502606 middle circuits / 0|2327 other circuits handled.

comment:4 Changed 5 years ago by arma

comment:5 Changed 5 years ago by asn

Cc: asn added

Changed 5 years ago by karsten

Attachment: cells-types.png added

Cells handled by circuit position/type

Changed 5 years ago by karsten

Attachment: cells-hs.png added

Fraction of cells seen on hidden-service circuits

Changed 5 years ago by karsten

Attachment: plot.R added

R script to plot stats of hidden service usage in the Tor network

comment:6 Changed 5 years ago by karsten

I just attached two graphs:

  • The first graph shows all cell types in a bar chart. While this sounds great for comparing cells by type, this graph works really poorly for the tiny fraction of hidden-service cells in the network (yes, there's a green line near the x axis, at least for bolobolo1 and wannabe). Maybe it's useful for comparing middle vs. exit.
  • The second graph shows the fraction of hidden-service cells as compared to all cells. moria5 might be somewhat misleading here, because moria5 handles a tiny number of cells compared to the other two relays. The network-wide average is probably much closer to bolobolo1's or wannabe's fraction than moria5's fraction.

I also attached the R code which comes with easy-to-follow instructions.

comment:7 Changed 5 years ago by karsten

Here's a branch that adds obfuscation to asn's bug13192_draft branch: https://gitweb.torproject.org/karsten/tor.git/commit/?h=task-13192

comment:8 Changed 5 years ago by karsten

Status: newneeds_review

asn, I just pushed more commits to my task-13192 branch and ran some tests in Chutney. Please take a look if you like my changes.

The next step would be to rewrite history. So far, only you and I have read this code. But the goal would be to have many more people read it. What we need are a few carefully crafted commits that are trivial to review even for people who are not C programmers. I have the following commit series in mind:

  1. Your constify crypto_pk_get_digest patch.
  2. My obfuscation functions including unit tests.
  3. The two hidden-service statistics including everything from config option to writing stats to disk.
  4. Your log statements and any remaining XXX, including the commented-out code that would include stats in extra-info descriptors. This commit shall not be given out to relay operators who we ask to help with gathering statistics, because they shouldn't accidentally run it. It's just for our testing and further development.

I can change history as described. Just let me know if this sounds reasonable to you.

comment:9 Changed 5 years ago by asn

Nice code!

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.
  • 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.
  • 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?
  • This value, multiplied with EPSILON, is Laplace parameter b. */ I think this is divided not multiplied.
  • 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.
  • 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?

comment:10 in reply to:  9 ; Changed 5 years ago by karsten

Replying to 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 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.

comment:11 in reply to:  10 ; Changed 5 years ago by asn

Replying to karsten:

Replying to asn:

Nice code!

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

Small review:

  • 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?

I'm sorry, I meant the key not the value. They key in our case is memdupped but I think it shouldn't be.

  • 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.

task-13192-2 looks OK to me. I don't see any new commits to the other branch.

FWIW, we should soon (in a week or so) give this code to people to run it in their relays. At that point, the commented code of extrainfo_dump_to_string() should be included and enabled so that stats actually get to us. What do we need to do to enable that piece of code? Revise the proposal and post it to [tor-dev]?

comment:12 in reply to:  11 Changed 5 years ago by karsten

Replying to asn:

I'm sorry, I meant the key not the value. They key in our case is memdupped but I think it shouldn't be.

Oh, you're right. Fixed in new fixup commit on branch task-13192-2, I think.

task-13192-2 looks OK to me. I don't see any new commits to the other branch.

Looks like I forgot to push task-13192 earlier. Pushed now.

FWIW, we should soon (in a week or so) give this code to people to run it in their relays. At that point, the commented code of extrainfo_dump_to_string() should be included and enabled so that stats actually get to us. What do we need to do to enable that piece of code? Revise the proposal and post it to [tor-dev]?

Yes, let's revise the proposal and post the code to tor-dev@, possibly even with the commented-out code in it. Then let's give people a week, or at least a couple of days, to review everything. After that, we could publish the code with the working extra-info code and ask people to run it. Of course, there's always the possibility that people will have feedback that we need to incorporate, or that will force us to change our plans.

comment:13 Changed 5 years ago by 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.
  • 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.
  • +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 :).
  • 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.

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.
  • rep_hist_hs_stats_init(): double space:
    if  (!hs_stats) {
    

The rest seems fine to me for now.

comment:14 Changed 5 years ago by asn

Hm, also maybe the epsilon should not be in the hidserv-stats-end line and should be in both of the stat lines. There might be a good reason for having different epsilons between different stats (for example, because we might care more about HSDir stat obfuscation than HS cell obfuscation))

comment:15 Changed 5 years ago by karsten

Replying to 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.

comment:16 in reply to:  14 Changed 5 years ago by karsten

Replying to asn:

Hm, also maybe the epsilon should not be in the hidserv-stats-end line and should be in both of the stat lines. There might be a good reason for having different epsilons between different stats (for example, because we might care more about HSDir stat obfuscation than HS cell obfuscation))

As discussed in #tor-dev, I changed this.

There, updated task-13192-2 contains all the changes.

comment:17 Changed 5 years ago by karsten

And task-13192-3 contains a rebased version of task-13192-2 with yesterday's changes.

comment:18 Changed 5 years ago by dgoulet

Quick comment also, I was worried about the hs_stats->rp_relay_cells_seen overflow possibility. I did an easy calculation to see how much data needs to go through the RP for that:

This is an unsigned long where on x86 64 bits it's 64 bit thus humongeous. However, on 32 bit it goes up to 2^32 and with each cell at 512 bytes.

((2^32) * 512) = 2199023255552 bytes of total data before overflow.
(((2199023255552/1024)/1024)/1024) = 2048 Gigabytes
                 ^K    ^M    ^G

Thus, one would have to send through ~2TB of data to make that stat goes round and around (on 32 bit system) :).

Code looks good to me! There are still log statement though that I'm not sure we want upstream like below but I'll wait for the big squash :).

+    log_warn(LD_GENERAL, "Saw new RP cell in %u.",
+             TO_OR_CIRCUIT(circ)->p_circ_id);

comment:19 Changed 5 years ago by karsten

Regarding overflow, I think we'd be in trouble after 1 TiB, because we're casting the unsigned long into signed long for adding noise. That's 1 TiB/day or 12.7 MB/s on average. We could switch to int64_t. It's just 32 more bits per relay. And we'll need a new set of obfuscation functions. Should we do that?

The log statement you refer to is in the "CLASSIFIED" commit that's only there for debugging purposes and that should go away before we give out the branch to relay operators. Unless you found another log statement in the other commits?

comment:20 in reply to:  19 Changed 5 years ago by dgoulet

Replying to karsten:

Regarding overflow, I think we'd be in trouble after 1 TiB, because we're casting the unsigned long into signed long for adding noise. That's 1 TiB/day or 12.7 MB/s on average. We could switch to int64_t. It's just 32 more bits per relay. And we'll need a new set of obfuscation functions. Should we do that?

I would definitely go for uint64_t/int64_t here, it's pretty cheap memory wise and can prevent issues that we think are improbable now but are the next day :).

The log statement you refer to is in the "CLASSIFIED" commit that's only there for debugging purposes and that should go away before we give out the branch to relay operators. Unless you found another log statement in the other commits?

Sorry for that, I'll know next time that CLASSIFIED means will disapear :)

comment:21 Changed 5 years ago by cypherpunks

Even my Oracle NetBeans IDE - not an especially privacy-focused piece of software - asks the user beforehand whether they would like to opt in to statistics.

comment:22 Changed 5 years ago by karsten

Changed to int64_t, switched obfuscation methods, and applied them in two separate calls. task-13192-3 contains squash-commits, task-13192-4 is a clean rebase.

comment:23 Changed 5 years ago by dgoulet

Code looks good to me!

Should I start using it ? :)

comment:24 Changed 5 years ago by karsten

For testing purposes, yes please. But the actual measurements won't start in the next 7 to 10 days, and you'll probably have to switch to another branch then.

comment:25 Changed 5 years ago by karsten

Pushed another commit to task-13192-4 that changes delta_f from 1 to 8, as suggested by Aaron Johnson on tor-dev@.

comment:26 Changed 5 years ago by karsten

I enabled the part where we're including hidden-service statistics in extra-info descriptors. task-13192-4 contains additional commits, task-13192-5 is a rebased version of task-13192-4.

I also removed the log messages, because we don't want people to run them without knowing. I'll attach the patch to this ticket in a minute.

comment:27 in reply to:  26 Changed 5 years ago by asn

Replying to karsten:

I enabled the part where we're including hidden-service statistics in extra-info descriptors. task-13192-4 contains additional commits, task-13192-5 is a rebased version of task-13192-4.

I also removed the log messages, because we don't want people to run them without knowing. I'll attach the patch to this ticket in a minute.

I like the code. I wonder if the additive noise should be applied to the right side of the bin, or to the center of the bin. Or if it matters at all.

So for example if binsize is 8. And the binned value is 16. Should the additive noise be applied to 16, or to 12? I'm asking because 12 (the center) might be a more fair value than 16 (which is the max).

comment:28 Changed 5 years ago by karsten

My intuition is that it shouldn't matter whether we add noise to the right side of a bin (which is at the same time the left side of the next bin) or the center of a bin. In either case there's exactly one predefined value in each bin that we add noise to. The adversary knows that, and given any obfuscated value she can say how likely it is that we started at a given bin, regardless of the definition we picked before. The distance between bins stays exactly the same, so this doesn't become harder or easier if we start at bin centers. I'd say let's not change this part in the code and keep using the right side of the bin.

comment:29 Changed 5 years ago by nickm

Looks mostly okay.

Three things we need to document in the manpage and the changes file (which needs to exist):

  • Why you would want to do this;
  • the fact that the statistics are obfuscated;
  • the fact that the statistics are not only stored to disk but also included in the extrainfo document.

comment:30 Changed 5 years ago by nickm

One more small suggestion: rep_hist_seen_maybe_new_hs should probably be called rep_hist_stored_maybe_new_hs.

comment:31 Changed 5 years ago by asn

Thanks for review.

Please see my branch karsten-task-13192-5 in my repository for the respective fixes and additions. Feel free to tweak changes file as you see appropriate.

comment:32 Changed 5 years ago by karsten

Indeed, thanks for the review, nickm! And thanks for the almost instant fixes, asn! I merged asn's changes into my task-13192-5 branch with one trivial change: I turned the changes-file-and-man-page commit into a --squash commit rather than a commit of its own to avoid back-and-forth in Git history.

comment:33 Changed 5 years ago by nickm

Status: needs_reviewneeds_information

squashed and merging. Thanks!

Moving this to needs_information, since it's a stats-collection ticket now. Feel free to close it and open a new ticket if that's more appropriate.

comment:34 Changed 5 years ago by karsten

Status: needs_informationneeds_review

Please review the latest commit in my task-13192-6 branch for a fix to unit tests on 32-bit systems. Oops.

comment:35 Changed 5 years ago by nickm

Status: needs_reviewneeds_information

ok, but long long vs int64 is a gcc vs standards issue. INT64_MIN is the right thig to look at, and we need to make sure it's defined. I've merged that, and another patch.

comment:36 Changed 5 years ago by asn

fix lgtm. thanks!

comment:37 Changed 5 years ago by karsten

Thanks for the better fix and quick response!

comment:38 Changed 5 years ago by mo

Tor 0.2.6.1-alpha-dev (git-808e2b856bd77fa9+245fdd2) now running with HiddenServiceStatistics 1 on the following new exit relays (2x1 Gbit/s max):

mendes 770BE0CDAF2B3C5F3517B72E41B0A6B5D89D8017
kingara 488239E03C4FBC22BA7C5EC76178C4E683A21B0D
RibeiroDaSilva 78213EC898E96E919D19EB9F0F85A59C3DD00D0C
mariagomez BB4290811B315E0BC779AE1368C95A88EB2D2E45
impastato 84567A5BDFE927B35E4D868BE2AA39AF9CD5F5EF
shifidi A0D858F81FAA7CE6B5B746106A35E44ADA0045B9

comment:39 Changed 5 years ago by teor

Cc: teor added

Tor 0.2.6.1-alpha-dev (git-f9ba0b76cd5b447b86efa87eb115d12d6f0be2e7) now running with HiddenServiceStatistics 1 on the following existing non-exit relays

With HSDir flag, 150kB/s max:
teorEastAU B1B0D087E43E2A43913377302DFABBDA28715A7B
teorEastAU2 CC47CC74B7637B683038A9969240991CDD9F48A0

Without HSDir flag, 1MB/s? max (Amazon EC2 t1.micro):
teorEastAU3ec2 425E55A8FA145ACFC01FA58CD4E6F46DD7762AAB

Apologies, they're not very fast. (I'm working on getting some faster ones up.)

comment:40 Changed 5 years ago by teor

I've looked at some of the edge cases in the laplace code. I've created a branch that avoids operations that might trap or overflow.

Branch: laplace-edge-cases
Repository: ​​​​​​https://github.com/teor2345/tor.git

This branch:

  • Avoid division by zero.
  • Avoid taking the log of zero.
  • Silence clang type conversion warnings using round and trunc.
    • The existing values returned by the laplace functions do not change.
  • Consistently check for overflow in round_*_to_next_multiple_of.

I've also added tests for extreme values in all the laplace functions, and created tests for each round_*_to_next_multiple_of function.

Unfortunately, the code isn't as concise or neat as it used to be, due to some of the special cases.

Changed 5 years ago by karsten

Changed 5 years ago by karsten

comment:41 Changed 5 years ago by karsten

(teor, thanks for the code review and patch. Please don't think people are ignoring you. Somebody is going to reply to you in the next few days.)

I just finished a very first analysis of reported hidserv-stats. My analysis code has not been reviewed by anyone. These results might be off by orders of magnitude. Handle with care.

Please find the attached two graphs:

The first graph, Total number of RELAY cells on rendezvous circuits, takes the number of cells that a single relay reports, looks up the relay's mean fraction of consensus weight during the statistics interval, and plots the quotient of the two numbers. The graph shows that most relays except those near x = 0% show quite similar network totals.

Speaking in absolute numbers (see my warning above about possibly being wrong by orders of magnitues), that's roughly 12.5 billion cells on rendezvous circuits per day. At 512 B per cell that's 70.64 MiB/s. With roughly 6,000 MiB/s traffic in the network, that's 1.18% of total traffic.

The second graph, Total number of .onion addresses, uses the reported number of .onion addresses seen by hidden service directories and extrapolates them to the expected number of addresses in the network.

Calculating the fraction of .onion addresses that a relay would see is more complicated though. In this analysis I'm looking at the mean fraction of descriptor space that a directory has handled. That's the difference between the fingerprint of the relay three HSDirs earlier in the ring up to the HSDir's own fingerprint, accounting for traversing ring end/start, divided by the total ring size (1 << 160).

Now, each directory has four chances to see a .onion address during 24 hours: there are two replicas per descriptor, and descriptor identifiers change once every 24 hours which most likely doesn't align with the statistics interval.

The graph shows reported .onion numbers divided by calculated descriptor space fraction divided by four.

In absolute numbers (same warning as above) there are roughly 30,000 .onion addresses in the network.

So, my main message here is that I'm hopeful that we can extrapolate observations by single relays to network totals, at least if these relays are not too slow or see a too small part of descriptor space.

comment:42 Changed 5 years ago by teor

Thanks, karsten - but I wasn't at all concerned. I think everyone deserves a break at least once a year!

comment:43 in reply to:  42 Changed 5 years ago by karsten

Replying to teor:

Thanks, karsten - but I wasn't at all concerned. I think everyone deserves a break at least once a year!

Indeed, time to take a break and review some maths code. :)

b823b7aad83489ffbac068eeb96ec0492cd50a36

  • These fixes look trivially correct. Just the changes file doesn't follow the usual format. Please take a look at the ChangeLog file for examples.

cc8f7ab46378e7520d1003844d59b1bc358a6af0

  • Same remark about the changes file as above.
  • Regarding your last comment in sample_laplace_distribution(): you're thinking about handling extreme values for b, which is obviously better than hoping they're handled correctly. But the even better way, IMHO, is to restrict the interval of acceptable ranges, document that, and assert if we receive values outside of that interval. For b, we can require that it's > 0, not Infinity, and not NaN.
  • In that same comment you're talking about converting to INT_MIN or INT_MAX. This conversion doesn't happen here, and it's a conversion to INT64_MIN and INT64_MAX. Maybe remove this comment.
  • In add_laplace_noise(), you're handling cases when epsilon is zero. IMHO, it's better to assert that this is not permitted, and document that. More precisely, delta_f should be > 0, not Infinity, and not NaN, and epsilon should be > 0 and <= 1. But I'd also like to hear asn's opinion on this, because he has read at least one research paper more on differential privacy than I.
  • In the same function, should there be a tor_trunc(), just in case different architectures/compilers do different things when you call trunc()?
  • Some of your tests pass INT64_MAX as delta_f to add_laplace_noise(), but that function expects a double there. Maybe pass some kind of DOUBLE_MAX there.
  • I found at least one duplicate unit test (right after the comment "does it play nice with INT64_MAX?"). Maybe take another look if there are more duplicates.
  • Rest looks good.

Thanks!

comment:44 Changed 5 years ago by teor

OK, this is next on my list, Karsten, possibly mid-week or next weekend.

What do you want me to do with the calling code (non-test code) that already passes zeroes to the function?
I can try and fix it, but I'm not sure what the semantics should be.
(And it's been a while, so I'd have to check the details again.)

comment:45 Changed 5 years ago by karsten

It shouldn't be necessary to change any non-test code. If you find any such code that needs changing, please point it out.

By the way, while you're looking into this, maybe also take a look at #14090. It seems very related (I asked them to run your branch, but it had the same problem).

Thanks!

comment:46 Changed 5 years ago by dgoulet

I'm trying to fix karsten's comments in teor's branch but the laplace-edge-cases branch doesn't contain the commits karsten mentionned and some other branch has been merged in it (bug13538-donna-signed-shift).

teor, would it be possible for you to rebase your test branch on master without extra commit in it?

comment:47 Changed 5 years ago by dgoulet

Keywords: tor-hs added
Status: needs_informationneeds_review

Along with karsten, we fixed all the things in comment:43. The test passes also.

See branch bug13192_026_01 in my repository.

comment:48 Changed 5 years ago by teor

Apologies, dgoulet, karsten, I dropped the ball on this one.
And I was trying to get the branch to build, and messed it up.
(I broke the git rule - no forced updates to public branches.)

I'll try to remember to make a new branch next time for any changes.
Glad you got it working.

comment:49 Changed 5 years ago by karsten

dgoulet, please find my bug13192_026_02 branch with some fixes to your branch.

comment:50 Changed 5 years ago by dgoulet

  • if (p <= 0.0) {, p can't possibly be below 0.0 because of the assert() before and the hard requirement noted in the comment.
[...] value <b>p</b> from the uniform distribution in [0.0, 1.0[
  • tor_llround(trunc(result));, maybe adding a small comment would be good. I don't quite get why we are doing that? Also, is your original comment still valid about trunc vs tor_trunc that we might want to add that?

Thanks for the change file! I completely forgot to update it :S

comment:51 Changed 5 years ago by karsten

I changed the p == 0.0 part to p <= 0.0 because GCC thinks we shouldn't compare floats with ==. Not sure if we should instead do something like < 0.0000000000000002 (smallest possible positive double value).

I don't know what to do about trunc vs. tor_trunc. I figured it's probably harmless. Want to investigate? And want to add a comment? I think teor's idea there was to silence clang warnings which we'd get when converting using an explicit cast and which we don't get when doing the conversion even more explicitly.

comment:52 in reply to:  51 ; Changed 5 years ago by dgoulet

Replying to karsten:

I changed the p == 0.0 part to p <= 0.0 because GCC thinks we shouldn't compare floats with ==. Not sure if we should instead do something like < 0.0000000000000002 (smallest possible positive double value).

Ah right! Forgot about that! Fun with float!... :)

I don't know what to do about trunc vs. tor_trunc. I figured it's probably harmless. Want to investigate? And want to add a comment? I think teor's idea there was to silence clang warnings which we'd get when converting using an explicit cast and which we don't get when doing the conversion even more explicitly.

I don't think trunc is any useful and I can't get any clang warnings if it's a direct cast so maybe teor can enlight us on that.

However there is an other problem. Consider this example, https://people.torproject.org/~dgoulet/volatile/float.c, please run it and you'll see that even with a double well in between [INT64_MIN, INT64_MAX] when cast to int64_t weird things happen. In this case here, we always underflow even though the value is supposed to be valid which will make sample_laplace_distribution return wrong values because of the cast. (llround or/and trunc don't matter here, see example).

Yawning is playing with this as we speak to understand it...

comment:53 in reply to:  52 Changed 5 years ago by yawning

Replying to dgoulet:

However there is an other problem. Consider this example, https://people.torproject.org/~dgoulet/volatile/float.c, please run it and you'll see that even with a double well in between [INT64_MIN, INT64_MAX] when cast to int64_t weird things happen. In this case here, we always underflow even though the value is supposed to be valid which will make sample_laplace_distribution return wrong values because of the cast. (llround or/and trunc don't matter here, see example).

Yawning is playing with this as we speak to understand it...

Sigh. Ok. You're triggering undefined behavior, because of floating point happy fun times. To be specific, you get 52 explicit bits of mantissa, so you will see rounding effects. The double value of "d" in this case is INT64_MAX + 1 (2(-1 * 0) * 1.0 * 2(0x43e - 1023)), which isn't representable.

This shows one way to do this correctly: https://stackoverflow.com/questions/25857843/how-do-i-convert-an-arbitrary-double-to-an-integer-while-avoiding-undefined-beha

Integers stored as IEEE doubles are only precise between -253 to +253 due to the number of bits in the mantissa. Past that, you are in for "fun", like what happened here.

comment:54 Changed 5 years ago by dgoulet

See branch bug13192_026_03 in my repo. I've added a helper function to cast double to int64 using the method pointed by Yawning.

(function name not final, I'm happy to change it if anyone is not happy with it.)

comment:55 Changed 4 years ago by asn

New code looks good to me. Maybe, as Yawning suggested, a few unit tests for cast_double_to_int64() would be helpful too.

comment:56 Changed 4 years ago by dgoulet

Milestone: Tor: 0.2.???Tor: 0.2.7.x-final

comment:57 Changed 4 years ago by karsten

Please find my branch bug13192_026_03 for some unit tests.

comment:58 Changed 4 years ago by asn

Tests look good and pass on my machine (can someone also try on a x86-32bit just to make sure?).

Thanks!

comment:59 Changed 4 years ago by karsten

vagrant@vagrant-ubuntu-trusty-32:/vagrant/tor$ uname -a
Linux vagrant-ubuntu-trusty-32 3.13.0-48-generic #80-Ubuntu SMP Thu Mar 12 11:16:18 UTC 2015 i686 i686 i686 GNU/Linux
vagrant@vagrant-ubuntu-trusty-32:/vagrant/tor$ src/test/test util/cast_double_to_int64
util/cast_double_to_int64: OK
1 tests ok.  (0 skipped)
vagrant@vagrant-ubuntu-trusty-32:/vagrant/tor$ 

comment:60 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

The merge with the latest git origin failed because the test_util_strclear function doesn't exist, so I had to remove this line:

UTIL_TEST(strclear, 0), in src/test/test_util.c

The build fails on OS X 10.10.2 (Xcode 6.3 Beta) with:

src/test/test_util.c:22:10: fatal error: 'values.h' file not found
#include <values.h>

This can be resolved using the UNIX standard float.h, or perhaps limits.h on legacy systems. I'm not sure what the Windows equivalent is.

comment:61 Changed 4 years ago by teor

Once these changes are made, the test fails the clang x86_64 (3.7.0 (trunk 230165) / x86_64-apple-darwin14.1.0) integer undefined behaviour sanitiser with:

util/cast_double_to_int64: src/test/test_util.c:4250:3: runtime error: left shift of negative value -1

This can be resolved by casting the constant -1 to uint64_t not int64_t:

  tt_i64_op(((uint64_t) -1) << 53, ==,
            cast_double_to_int64(-1.0 * pow(2.0, 53.0)));
  tt_i64_op((((uint64_t) -1) << 53) + 1, ==,
            cast_double_to_int64(-1.0 * pow(2.0, 53.0) + 1.0));

See http://stackoverflow.com/a/22734721

"The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1×2E2, reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and non-negative value, and E1×2E2 is representable in the result type, then that is the resulting value; otherwise, the behaviour is undefined.
...
If E1 is a signed type with negative value then the behaviour of left shifting is undefined. This is an easy route to undefined behaviour which may easily get overlooked."

The tests then succeed on OS X 10.10.2 / Xcode 6.3 beta / clang 3.7.0 with integer undefined behaviour checks / x86_64.

Last edited 4 years ago by teor (previous) (diff)

comment:62 Changed 4 years ago by teor

The tests also succeed on OS X 10.10.2 / Xcode 6.3 beta / clang 3.7.0 with integer undefined behaviour checks / -arch i386, with the changes above.

As an aside:
There is currently another instances of integer undefined behaviour in tor under i386 in the donna code. This issue was addressed in #13538, but it's awaiting upstream feedback, I think. My branch bug13538-donna-signed-shift resolves this, if anyone wants to re-run the clang sanitizer on the tor tests under 32-bit and have them all pass.

Last edited 4 years ago by teor (previous) (diff)

comment:63 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:64 Changed 4 years ago by isabela

Points: small
Priority: normalmajor
Version: Tor: 0.2.7

comment:65 Changed 4 years ago by karsten

teor, would you want to submit a patch for the issues you're pointing out above?

comment:66 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Sure. I had a branch, just forgot to submit it.
I've rebased it onto the current git head and got the merge working properly.
It's a little old, so let me know if there's anything missing.

Branch: karsten_bug13192_026_03_teor
Repository: https://github.com/teor2345/tor.git

Just to be clear, what the patch does is:

  • replace values.h with float.h (do all our platforms have float.h, or will some need limits.h?)
  • change the two lines with ((int64_t) -1) << 53 to ((uint64_t) -1) << 53 to avoid undefined signed left shift behaviour

There was no need to remove UTIL_TEST(strclear, 0) from src/test/test_util.c because the function is present in the file.

comment:67 Changed 4 years ago by teor

Apologies, the strclear function is no longer in the tor codebase, so I deleted the unit tests for it. I have no idea how it got mixed up in this patch.

I've updated the branch in the previous comment with these changes.

comment:68 Changed 4 years ago by dgoulet

Patch lgtm, pretty straight forward! With this I think we can close this one finally?

comment:69 Changed 4 years ago by arma

Where are we on this? teor has a patch to clean up some code that we already merged into Tor, and it's sitting here waiting for somebody to look at it?

comment:70 Changed 4 years ago by nickm

I'm concerned about the handling of edge cases in round_to_next_multiple_of and the name of cast_double_to_int64.

(I think that we should provide the invariant that round_to_next_multiple_of(a,b), if it returns, returns x st x >= a and x % b == 0. Otherwise we might someday risk Buffer Problems if somebody relies on that invariant. Does that make sense?)

(I think "cast_..." should be called "clamp_....".)

comment:71 Changed 4 years ago by teor

I'll look into this after I finish the fallback directories work.

comment:72 Changed 4 years ago by teor

Keywords: TorCoreTeam201508 added
Owner: set to teor
Status: needs_reviewassigned

comment:73 Changed 4 years ago by teor

Status: assignedneeds_revision

comment:74 Changed 4 years ago by nickm

Keywords: TorCoreTeam201509 added; TorCoreTeam201508 removed

comment:75 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:76 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:77 Changed 4 years ago by nickm

Keywords: SponsorR removed
Sponsor: SponsorR

Bulk-replace SponsorR keyword with SponsorR sponsor field in Tor component.

comment:78 Changed 4 years ago by dgoulet

Keywords: 027-triaged-1-in removed

comment:79 Changed 4 years ago by dgoulet

Keywords: TorCoreTeam201509 removed

comment:80 Changed 4 years ago by nickm

Keywords: pre028-patch added

comment:81 Changed 4 years ago by teor

Severity: Normal

I'm happy for someone else to do this if they get time. I have ended up with a lot of other things to do. Otherwise, I'll get to it eventually.

comment:82 Changed 4 years ago by teor

Keywords: TorCoreTeam201512 added

comment:83 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

I've made my suggested changes as karsten_bug13192_026_03_teor in my public repository.

comment:84 Changed 4 years ago by yawning

lgtm

comment:85 Changed 4 years ago by nickm

Keywords: TorCoreTeam201511 added; TorCoreTeam201512 removed
Resolution: fixed
Status: needs_reviewclosed

Merged!

comment:86 Changed 14 months ago by teor

Cc: teor removed

Remove useless CC

Note: See TracTickets for help on using tickets.