Re: Patches for proposal 155: Four Improvements of Hidden Service Performance

[Roger Dingledine <arma@xxxxxxx>]

On Wed, Oct 08, 2008 at 01:57:49PM +0200, Karsten Loesing wrote:
> Index: /home/karsten/tor/tor-trunk-155-patch1/ChangeLog
> ===================================================================
> --- /home/karsten/tor/tor-trunk-155-patch1/ChangeLog	(revision 17050)
> +++ /home/karsten/tor/tor-trunk-155-patch1/ChangeLog	(working copy)
> @@ -3,6 +3,8 @@
>      - Now NodeFamily and MyFamily config options allow spaces in
>        identity fingerprints, so it's easier to paste them in.
>        Suggested by Lucky Green.
> +    - Reduce extension timeout for introduction circuits from 60 to 30
> +      seconds.

Applied and in. I changed it a bit, but not in any substantial way
I think.

> Index: /home/karsten/tor/tor-trunk-155-patch2/ChangeLog
> ===================================================================
> --- /home/karsten/tor/tor-trunk-155-patch2/ChangeLog	(revision 17050)
> +++ /home/karsten/tor/tor-trunk-155-patch2/ChangeLog	(working copy)
> @@ -3,6 +3,8 @@
>      - Now NodeFamily and MyFamily config options allow spaces in
>        identity fingerprints, so it's easier to paste them in.
>        Suggested by Lucky Green.
> +    - Start extending introduction circuits in parallel after a delay of
> +      15 seconds (based on work by Christian Wilms).

Isn't this patch a smaller version of patch4? That is, if we started
enough circuits to begin with, can we just skip this step? It seems
odd to apply both patch2 and patch4, and it seems like patch4 is the
more general.

> Index: /home/karsten/tor/tor-trunk-155-patch3/ChangeLog
> ===================================================================
> --- /home/karsten/tor/tor-trunk-155-patch3/ChangeLog	(revision 17050)
> +++ /home/karsten/tor/tor-trunk-155-patch3/ChangeLog	(working copy)
> @@ -3,6 +3,8 @@
>      - Now NodeFamily and MyFamily config options allow spaces in
>        identity fingerprints, so it's easier to paste them in.
>        Suggested by Lucky Green.
> +    - Increase number of server-side introduction points based on number
> +      of recent hidden service requests (based on work by Christian Wilms).

I think this is a very useful change, since we really do want our hidden
services to adapt the number of preemptive circuits they hold open to
the amount of attention they get. But the patch here looks much more
complex than it needs to be.

Rather than keeping a whole history with exact timestamps and everything,
why not keep a counter and a timestamp in rephist (or rend*), and let
the counter decay over time.

That is, when a circuit is requested we call a function to increment the
counter (but never past some cap), and when we're considering making
a new circuit we call a function to get the current counter value,
and every time either of these functions is called we check a static
time_t and move the counter value down by 1 (but not less than 0) for
every interval that's passed since we last called one of the functions.

It's not quite as precise, but it's way less code. :)

> Index: /home/karsten/tor/tor-trunk-155-patch4/ChangeLog
> ===================================================================
> --- /home/karsten/tor/tor-trunk-155-patch4/ChangeLog	(revision 17050)
> +++ /home/karsten/tor/tor-trunk-155-patch4/ChangeLog	(working copy)
> @@ -3,6 +3,9 @@
>      - Now NodeFamily and MyFamily config options allow spaces in
>        identity fingerprints, so it's easier to paste them in.
>        Suggested by Lucky Green.
> +    - Start building more server-side introduction circuits than needed
> +      (five), pick the first three that succeed, and use the others as
> +      general-purpose circuits.

Looks plausible. I'm going to review the patch more tomorrow and then
check it (or something like it) in.

>      /* Remember how many introduction circuits we started with. */
>      prev_intro_nodes = smartlist_len(service->intro_nodes);
> -
> -    /* The directory is now here. Pick three ORs as intro points. */
> -    for (j=prev_intro_nodes; j < NUM_INTRO_POINTS; ++j) {
> +    /* The directory is now here. Pick three ORs as intro points (plus, if
> +     * we currently have none at all, two more so that we can pick the first
> +     * three afterwards). */
> +#define NUM_INTRO_POINTS_INIT (NUM_INTRO_POINTS + 2)
> +    for (j=prev_intro_nodes; j < (prev_intro_nodes == 0 ?
> +             NUM_INTRO_POINTS_INIT : NUM_INTRO_POINTS); ++j) {
>        router_crn_flags_t flags = CRN_NEED_UPTIME;
>        if (get_options()->_AllowInvalid & ALLOW_INVALID_INTRODUCTION)
>          flags |= CRN_ALLOW_INVALID;

This is the part that makes me nervous still. We make 5 intro points,
and as soon as three are ready, we discard (change to general) any others
that finish. What if the three that are ready are at the end of the intro
points list? Don't we need to remove the intro point from the list too,
if we decide not to use its intro circuit?

--Roger