Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#4862 closed enhancement (fixed)

Consider disabling dynamic intro point formula (numerology)

Reported by: hellais Owned by:
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: needs-proposal, tor-hs, 027-triaged-1-in
Cc: rransom Actual Points:
Parent ID: Points: medium/large
Reviewer: Sponsor: SponsorR

Description (last modified by asn)

This ticket was repurposed and it's now about discussing the dynamic formula for determining the number of introduction points of a hidden service.

The formula leaks the popularity of the hidden service on an hourly basis. Furthermore, it's memoryless which causes the hidden service to use a much bigger number of introduction points than normal (comment:15:ticket:15513).


In #3825 rransom proposed to tune the number of IP to rebuilt once one expires based on the history of the Tor HS usage. I believe these numbers are not optimal and there should be a better way to do this.

These are the first numbers that I will try to estimate:
I = number of intro points
C = Connections made to an IP in it's lifetime
T = Total number of connections made to the HS in 24 hours

What we are interested in estimating is the value of NUM_INTRO_POINTS_MAX and this is based on the estimation of C. To determine this we will consider this equation:

I = T/C

I believe it is reasonable to suppose that a very busy HS will at most have 1M connections in 24 hours. This means that:

I = 1'000'000/C

Currently C is set to 16384. I am not sure why this number was chosen, but if this number is good we would need to set the value of I to 61.

Lets take for granted that 61 is a good value for NUM_INTRO_POINTS_MAX, this means that the number of active IP at a given time should be in the range of 3-61.

For this reason I believe it would be good to have the number of IP to recreate to be in the range of 1-20 dependent on the history of a Tor HS.

The basic thing we can do is use a linear function to determine this number x. We want a linear function that has these properties:

f(0) = 1f(4/3) = NUM_INTRO_POINTS_MAX (supposing that for lifetime of IP tends to end the fraction (time_since_publishing/IP_MIN_LT)*(accepted_ip_connection)/(IP_CON_LT) -> 4/3)
This leads us to this:

x = (1 - NUM_INTRO_POINTS_MAX)*((time_since_publishing/IP_MIN_LT)*(accepted_ip_connection)/(IP_CON_LT)) + 1

in the case of NUM_INTRO_POINTS_MAX = 20 this means:

x = 25.3333333 * (time_since_publishing/IP_MIN_LT)*(accepted_ip_connection)/(IP_CON_LT) + 1

A better way to do this is have an exponential function that converges asymptotically to 20.

Does this seem sane?

Child Tickets

Change History (46)

comment:1 Changed 7 years ago by hellais

Description: modified (diff)

comment:2 Changed 7 years ago by hellais

Description: modified (diff)

comment:3 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final

0.2.4.x, though it's backportable if it turns out in testing to be much better than the status quo.

comment:4 Changed 6 years ago by arma

It would be great to have a hidden service's intro point count grow to accommodate load.

rransom, did you have an opinion on hellais's numbers?

hellais, did you have a patch?

comment:5 in reply to:  4 ; Changed 6 years ago by rransom

Replying to arma:

It would be great to have a hidden service's intro point count grow to accommodate load.

They already do that.

rransom, did you have an opinion on hellais's numbers?

Do not exceed 10 introduction points in any hidden service descriptor.

comment:6 Changed 6 years ago by nickm

Keywords: needs-proposal added

comment:7 Changed 6 years ago by nickm

Keywords: tor-hs added

comment:8 Changed 6 years ago by nickm

Component: Tor Hidden ServicesTor

comment:9 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

comment:10 Changed 6 years ago by arma

Summary: Tor Hidden Service IP replacing numerologyTor Hidden Service Intro Point replacing numerology

comment:11 Changed 6 years ago by asn

Depending on the threat model we choose, we should be careful so that the number of Introduction Points of a Hidden Service does not reveal too much information about its popularity.
For example, maybe an attacker should not be able to do "Oh, this HS has 19 IPs, which means that it's getting somewhere between 15k to 16k introduction requests a day.".

comment:12 in reply to:  5 ; Changed 6 years ago by asn

Replying to rransom:

Replying to arma:

It would be great to have a hidden service's intro point count grow to accommodate load.

They already do that.

rransom, did you have an opinion on hellais's numbers?

Do not exceed 10 introduction points in any hidden service descriptor.

Robert, why is 10 your hard limit on the number of Introduction Points? Do you think that more than 10 Introduction Points per HS would put too much load on the network?

comment:13 in reply to:  12 Changed 6 years ago by rransom

Replying to asn:

Replying to rransom:

Replying to arma:

rransom, did you have an opinion on hellais's numbers?

According to a recent paper, hidden services' descriptors are only requested tens of thousands of times per day. Arturo's claim that a popular hidden service could be connected to 1000000 times per day is either (a) bullshit, or (b) based on the number of HTTP requests or the number of streams carrying HTTP requests, not the number of introduction attempts.

The limit on the number of introductions to be handled by an introduction circuit during its lifetime is only partly a measure of service popularity; it is also the limit on the amount of memory consumed by each introduction circuit's replay-detection cache. Based on the numbers in the recent paper, 16ki introductions per circuit was far too high. (Unfortunately, no one who operated a popular hidden service provided useful measurements to me while I was designing the introduction-point expiration code.) It would be reasonable to reduce INTRO_POINT_LIFETIME_INTRODUCTIONS to a few thousand.

Do not exceed 10 introduction points in any hidden service descriptor.

Robert, why is 10 your hard limit on the number of Introduction Points? Do you think that more than 10 Introduction Points per HS would put too much load on the network?

There are several reasons that anyone who understands Tor well enough to be trusted to maintain or update or redesign or even gripe about the hidden service protocol would refuse to consider putting more than 10 introduction points in any hidden service descriptor. Here are some of them:

  • Each hidden service publishes its descriptor to at most 6 directory servers. This constant 6 is part of the Tor protocol and enforced at all hidden services, all HS clients, and all HS directory servers, and cannot be changed within version 2 of the HS directory protocol. Any hidden service whose clients would overload 10 introduction points with CREATE cells would overload its directory servers with CREATE cells first.
  • There is an upper limit on the length of a v2 HS descriptor, enforced at all HS directory servers. 10 introduction points is clearly more than any hidden service will ever need to put in a single descriptor, and keeps HS descriptor sizes well below that upper bound, even if a future version of Tor includes more information about each introduction point in every HS descriptor.

comment:14 Changed 4 years ago by asn

More work on this has been happening on #8950.

comment:15 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.7.x-final

Worth looking at during 0.2.7 triage IMO

comment:16 Changed 4 years ago by nickm

Status: newassigned

comment:17 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:18 Changed 4 years ago by dgoulet

Keywords: SponsorR added

comment:19 Changed 4 years ago by isabela

Points: medium/large
Priority: normalmajor
Version: Tor: 0.2.7

comment:20 Changed 4 years ago by asn

Description: modified (diff)
Summary: Tor Hidden Service Intro Point replacing numerologyConsider disabling dynamic intro point formula (numerology)

comment:21 Changed 4 years ago by asn

So, our current intuition here is to disable this formula by default.

After this we have various options:

a) Keep the formula and make it easy to enable using torrc.
b) Remove the formula completely, and allow the hidden service operator to specify the number of introduction points herself.
c) Remove the formula completely, and allow the hidden service operator to suggest a range of values for the number of introduction points, and then pick randomly from that range.
d) Keep the formula but also do the above. And maybe also randomize default number of IPs.
e) Remove formula. All HSes have 3 IPs. Period.
f) ???

comment:22 Changed 4 years ago by arma

I originally had suggested 'b' as the right answer.

But from looking at 'c', I wonder how bad it is for different hidden services to choose different numbers. Maybe somebody who operates six services will choose the same number for each, thus somehow giving something away? Now I wonder if maybe we want a simple boolean, "use the default three" vs "use the more scalable ten" and have done with it.

I guess that question leads to a deeper question: is ten actually more scalable in any meaningful way than three?

comment:23 in reply to:  22 ; Changed 4 years ago by dgoulet

Replying to arma:

I guess that question leads to a deeper question: is ten actually more scalable in any meaningful way than three?

The assumption here would be yes. Lots of traffic on 3 IPs, changing to 6 IPs means traffic is divided by two on those original 3. It also spread the load over the network instead of chocking it on 3 specific places. However, more IPs means more pipes to load the HS. If an IP chockes from a flood of requests, chances are that the HS will also chocke thus the IP is "kind" of a first line of defense. But if an extra IP is added which then offloads the others and make them work again, well the HS at that point could get chocked.

HS scalability is an other question here but let's keep in mind that in the present situation (until we have crazy performance improvement or HS load balancing), adding IPs could mean "easier" DDoS.

So I would go with *b* here.

  • Kill with fire the algorithm, in it's form right now it:
    • Leak popularity badly
    • Automatically increase your pipe to DDoS an HS.
  • Add a torrc option that enables an operator to specify a fix amount of IPs
    • That could be quite useful for let say Facebook that have its HS on a *HUGE* machine/network and wants to maximize reachability.
  • 3 IPs by default period.

I'm ready to work on the patch asap once we come up with a consensus.

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

comment:24 in reply to:  23 Changed 4 years ago by special

I agree with *b* for the same reasons as dgoulet.

*c* (random from a range) doesn't seem justified - the operator still stands out for having changed it, and if they copy this configuration, they likely have other distinguishers in their stack.

Replying to arma:

I guess that question leads to a deeper question: is ten actually more scalable in any meaningful way than three?

I think more than 6 (= number of HSDirs) doesn't make much sense.

As far as I know, we have no evidence of IPs ever being the limiting factor for scalability. Every indication we do have is that the service will fall apart first, which also makes sense intuitively. It's a single point and does much more work than an IP.

I could see this becoming a problem if we have many busy services, and they start overloading relays when many of them select the same IP. In that case, increasing IPs per service could help distribute the load. Still, this is entirely speculative, and I don't think we could build the right solution without actual data.

comment:25 Changed 4 years ago by arma

dgoulet: when we build this, assuming we allow the user to configure a number of intro points, please put a cap on the number they can pick. I don't want anybody picking 15000 intro points. :)

(I could still totally get behind either forcing everybody to use 3, because we don't know that more than 3 is actually helpful in any way, and it makes the onion service stand out; or forcing everybody to choose either 3 and 6, and not letting them pick their own number. I don't seem to have much support for those options though I guess?)

comment:26 in reply to:  25 Changed 4 years ago by dgoulet

Replying to arma:

dgoulet: when we build this, assuming we allow the user to configure a number of intro points, please put a cap on the number they can pick. I don't want anybody picking 15000 intro points. :)

Yup, 100% agree. I was planning to have the default to 3 and high bound to 10 (current state). We can discuss those values (if sane or not) later on with more research (#15513 will help answer that.)

(I could still totally get behind either forcing everybody to use 3, because we don't know that more than 3 is actually helpful in any way, and it makes the onion service stand out; or forcing everybody to choose either 3 and 6, and not letting them pick their own number. I don't seem to have much support for those options though I guess?)

Why having 7 IPs for instance makes an HS stand out? I give you address adkadsh778akajaa.onion, you check it out and you see 7 IPs. What's next? This might be msft.com or some other 2 users service but that won't change anything in terms of standing out apart from having 7 IPs? Do you have a scenario in mind on why that would be not good?

One thing we can assume here is that if an operator wants to use its own number like 7, it's "probably" an informed decision to do that meaning "Oh OK I was under heavy load lately, I'll bump it to 7 to improve the situation". Which brings me back to offering only 6 as an alternative is roughly the same for an operator to make a decision about something else than 3.

comment:27 Changed 4 years ago by dgoulet

Status: assignedneeds_review

Food for thought time! Here is a patch for both removing the algorithm and adding a torrc option for the amount of introduction points.

Two commits for both changes and one changes file for the whole ticket.

Branch bug4862_027_01

Also, do we really need a proposal for this? (keyword: need-proposal)

comment:28 in reply to:  27 Changed 4 years ago by asn

Replying to dgoulet:

Food for thought time! Here is a patch for both removing the algorithm and adding a torrc option for the amount of introduction points.

Two commits for both changes and one changes file for the whole ticket.

Branch bug4862_027_01

Looks good to me. I also tested it briefly and it seems to work.

comment:29 Changed 4 years ago by asn

BTW, what is this change supposed to be doing?

     if (!intro_point_set_changed &&
-        (n_intro_points_unexpired >= service->n_intro_points_wanted)) {
+        (n_intro_points_unexpired == service->n_intro_points_wanted)) {
       continue;
     }

If you know what that block of code does, can you please document it? Thanks!

comment:30 in reply to:  29 ; Changed 4 years ago by arma

Replying to asn:

BTW, what is this change supposed to be doing?

     if (!intro_point_set_changed &&
-        (n_intro_points_unexpired >= service->n_intro_points_wanted)) {
+        (n_intro_points_unexpired == service->n_intro_points_wanted)) {
       continue;
     }

If you know what that block of code does, can you please document it? Thanks!

I haven't looked much at the overall patch, but I think George is right to ask about this part. The original code said "if you are going to have enough intro points still, you're done." The new code with this change seems to go through a weird "if you have more than enough, then proceed with the function, and then the for() loop is a no-op, and then we hit

    /* If there's no need to launch new circuits, stop here. */
    if (!intro_point_set_changed)
      continue;

"

So I think the change above just adds a tiny bit of inefficiency and confusion, rather than actually changing behavior. But it still looks like simplifying and unconfusing would be wise.

comment:31 Changed 4 years ago by arma

Unless David meant for you to actually drop some intro points when the new number of desired intro points has decreased? Does the new code do that? It's not obvious to me that it does.

comment:32 in reply to:  30 Changed 4 years ago by dgoulet

Replying to arma:

I haven't looked much at the overall patch, but I think George is right to ask about this part. The original code said "if you are going to have enough intro points still, you're done." The new code with this change seems to go through a weird "if you have more than enough, then proceed with the function, and then the for() loop is a no-op, and then we hit

    /* If there's no need to launch new circuits, stop here. */
    if (!intro_point_set_changed)
      continue;

"

So I think the change above just adds a tiny bit of inefficiency and confusion, rather than actually changing behavior. But it still looks like simplifying and unconfusing would be wise.

If I recall correctly (thus adding a comment is very wise! :P) with this patch we always have a fixed number of intro points so if we have the same amount of *non* expired IPs (n_intro_points_unexpired) and IPs the service wants (service->n_intro_points_wanted), we have nothing more to do so we stop right there and move on to the next service with that continue. If we do not, it means we have to create new IP(s) thus getting in the for loop just after the check.

I unfortunately don't see how this adds inefficiency?

Looking at this code more closely now, I think that the check is not useful since we have now a fix number of IPs so only testing intro_point_set_changed == 1 should be enough.

comment:33 Changed 4 years ago by dgoulet

Here is a new branch. I've gone a bit insane but I think it needed to be done at some point anyway.

The two first commits are untouched but 3 more were added that are related to this ticket because removing the adaptative algorithm changed a bit how the function works thus the refactoring that hopefully helps the situation. In the process, those commits fix open issues that are detailed and listed in the commit message. This branch has been tested in a chutney network.

Need careful review: bug4862_027_02

(No changes file for now, if we get some ack on this, I'll add them)

comment:34 Changed 4 years ago by asn

Oh my, complex branch!

One small question for now:

  • I see you are decrementing n_intro_points_established in remove_invalid_intro_points(). I'm wondering if this decrement always matches with a previous increment.

I see we are only incrementing that counter in rend_service_intro_established() when we receive an INTRO_ESTABLISHED cell. What happens if the circuit dies before we receive an INTRO_ESTABLISHED cell, but after we add it to the intro_nodes smartlist? In that case, the counter will be extra decremented, right?

comment:35 in reply to:  34 ; Changed 4 years ago by dgoulet

Status: needs_reviewneeds_revision

Replying to asn:

  • I see you are decrementing n_intro_points_established in remove_invalid_intro_points(). I'm wondering if this decrement always matches with a previous increment.

I see we are only incrementing that counter in rend_service_intro_established() when we receive an INTRO_ESTABLISHED cell. What happens if the circuit dies before we receive an INTRO_ESTABLISHED cell, but after we add it to the intro_nodes smartlist? In that case, the counter will be extra decremented, right?

Hrm, yes indeed, if the circuit dies before we can establish, we end up with an extra decrement. What we could do here is add a flag in rend_intro_point_t called circuit_established that we set once we establish and when decrementing the service's counter, we make sure that this flag is set?

comment:36 in reply to:  35 ; Changed 4 years ago by asn

Replying to dgoulet:

Replying to asn:

  • I see you are decrementing n_intro_points_established in remove_invalid_intro_points(). I'm wondering if this decrement always matches with a previous increment.

I see we are only incrementing that counter in rend_service_intro_established() when we receive an INTRO_ESTABLISHED cell. What happens if the circuit dies before we receive an INTRO_ESTABLISHED cell, but after we add it to the intro_nodes smartlist? In that case, the counter will be extra decremented, right?

Hrm, yes indeed, if the circuit dies before we can establish, we end up with an extra decrement. What we could do here is add a flag in rend_intro_point_t called circuit_established that we set once we establish and when decrementing the service's counter, we make sure that this flag is set?

That might be a reasonable way to do it. However why do we need this extra counter? Does it give the same result as count_established_intro_points(serviceid)? If yes, why do we need both things?

My main worries about this branch is the synchronization between the various expiring_nodes, intro_nodes, retry_nodes lists, as well as the new counters that got introduced like n_intro_points_established. My plan is to test the branch soon, to see how these things work in real use.

Also, I noticed that this code was added:

+    /* Remove the intro point associated with this circuit, it's being
+     * repurposed or closed thus cleanup memory. */
+    rend_intro_point_t *intro = find_intro_point(circuit);
+    if (intro != NULL) {
+      smartlist_remove(service->intro_nodes, intro);
+      rend_intro_point_free(intro);
+    }
+

I'm wondering why this was not needed before. If for some reason is needed now, maybe you can move it a bit below when other hidden service data are also freed (crypto_pk_free(intro_key); etc.)?

comment:37 Changed 3 years ago by asn

I started testing this!

It seems like tor will crash when the HS tries to upload the second HS descriptor.

It will crash like this:

#0  0x00005555555b3014 in rend_data_dup (data=0x7fffffffe030) at src/or/rendcommon.c:1407
#1  0x000055555564c3ff in directory_initiate_command_rend (_addr=0x555556184480, or_port=20, dir_port=57712, digest=0x555555aa65fc "\351:~;\371\237>\374.\246\356|D\270\230\340\201\212\322\305\026W2\356\252\033I\237KE\031\332h\251\367f8\356\203\353\236B/\243\253p܌\215\022\210\026\224\062\365\306)#F#", 
    dir_purpose=176 '\260', dir_purpose@entry=17 '\021', router_purpose=64 '@', router_purpose@entry=0 '\000', indirection=4294958752, resource=0x0, 
    payload=0x555556185d20 "rendezvous-service-descriptor 5ex2pe24d4y3nus3umqen4rgbqlk34v6\nversion 2\npermanent-key\n-----BEGIN RSA PUBLIC KEY-----\nMIGJAoGBAM1ZaWMtX7rigjmTALwcr4bteltZVF4YCP9F6NLx0lB3SACu/XNrQVpt\nX8H7CMf3t3HYRlciX"..., payload_len=3253, if_modified_since=0, rend_query=0x7fffffffe030)
    at src/or/directory.c:981
#2  0x000055555564c940 in directory_initiate_command_routerstatus_rend (status=status@entry=0x555555aa65e0, dir_purpose=dir_purpose@entry=17 '\021', router_purpose=router_purpose@entry=0 '\000', indirection=indirection@entry=DIRIND_ANONYMOUS, resource=resource@entry=0x0, 
    payload=payload@entry=0x555556185d20 "rendezvous-service-descriptor 5ex2pe24d4y3nus3umqen4rgbqlk34v6\nversion 2\npermanent-key\n-----BEGIN RSA PUBLIC KEY-----\nMIGJAoGBAM1ZaWMtX7rigjmTALwcr4bteltZVF4YCP9F6NLx0lB3SACu/XNrQVpt\nX8H7CMf3t3HYRlciX"..., payload_len=3253, if_modified_since=0, 
    rend_query=0x7fffffffe030) at src/or/directory.c:646
#3  0x00005555555b9b9d in directory_post_to_hs_dir (renddesc=0x55555616ec40, descs=0x55555617bbc0, hs_dirs=0x0, service_id=0x7fffffffe170 "brhc7vtx6cmchjda", seconds_valid=33661) at src/or/rendservice.c:3158
#4  0x00005555555b9f27 in upload_service_descriptor (service=0x555555975e40) at src/or/rendservice.c:3273
#5  0x00005555555babc0 in rend_consider_services_upload (now=now@entry=1434288689) at src/or/rendservice.c:3696

I think the problem was introduced with the refactoring commit 6d127695ea: In directory_post_to_hs_dir() it introduced the rend_data structure allocated on the stack, that is not properly initialized before passed to directory_initiate_command_routerstatus_rend(). So even though the onion_address element was initialized, other required elements were not and that caused crashes. This crash happened in rend_data_dup() when the code was trying to access the hsdirs_fp pointer that was never initialized (and properly contains stack garbage).

Not yet sure why the invocation to directory_initiate_command_routerstatus() was changed in the refactoring commit.

BTW, you can check this issue by reducing INTRO_POINT_LIFETIME_MIN_SECONDS and INTRO_POINT_LIFETIME_MAX_SECONDS to very small numbers, so that the HS keeps on making new intros.

Last edited 3 years ago by asn (previous) (diff)

comment:38 in reply to:  36 Changed 3 years ago by dgoulet

Replying to asn:

Replying to dgoulet:

Replying to asn:

  • I see you are decrementing n_intro_points_established in remove_invalid_intro_points(). I'm wondering if this decrement always matches with a previous increment.

I see we are only incrementing that counter in rend_service_intro_established() when we receive an INTRO_ESTABLISHED cell. What happens if the circuit dies before we receive an INTRO_ESTABLISHED cell, but after we add it to the intro_nodes smartlist? In that case, the counter will be extra decremented, right?

Hrm, yes indeed, if the circuit dies before we can establish, we end up with an extra decrement. What we could do here is add a flag in rend_intro_point_t called circuit_established that we set once we establish and when decrementing the service's counter, we make sure that this flag is set?

That might be a reasonable way to do it. However why do we need this extra counter? Does it give the same result as count_established_intro_points(serviceid)? If yes, why do we need both things?

This counter was added to partially fix #13483, basically uploading the desriptor once all introduction points are established, not before. In rend_consider_services_upload(), we make sure that this counter is equal or above the amount of intro points the service wanted and if so, it means we have enough information to upload a valid descriptor.

Ok you caught me, I secretly want count_established_intro_point() to go away. It iterates over _all_ circuits and string compare the onion address. I think it's extra work that is not needed and could be replaced by the n_intro_points_established counter in the service object.

My main worries about this branch is the synchronization between the various expiring_nodes, intro_nodes, retry_nodes lists, as well as the new counters that got introduced like n_intro_points_established. My plan is to test the branch soon, to see how these things work in real use.

Also, I noticed that this code was added:

+    /* Remove the intro point associated with this circuit, it's being
+     * repurposed or closed thus cleanup memory. */
+    rend_intro_point_t *intro = find_intro_point(circuit);
+    if (intro != NULL) {
+      smartlist_remove(service->intro_nodes, intro);
+      rend_intro_point_free(intro);
+    }
+

I'm wondering why this was not needed before. If for some reason is needed now, maybe you can move it a bit below when other hidden service data are also freed (crypto_pk_free(intro_key); etc.)?

It's added because else we leak the intro point object when we clean this extra circuit. Before that, we would clean it in rend_services_introduce() when detecting that there were no more circuit linked to it.

We can't do that anymore because 1) we retry on intro points and this intro point here we are talking about wasn't used in the first place (extra intro point for performance reason), 2) it must not be considered has a valid intro points else the circuit established tracking is borked and 3) it must not be considered as an expired node also since we are allowed to re-use it.

comment:39 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

Ok I did much of the fixes here and also one extra commit on top to fix another issue I found while testing.

See branch: bug4862_027_03

comment:40 in reply to:  39 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Replying to dgoulet:

Ok I did much of the fixes here and also one extra commit on top to fix another issue I found while testing.

See branch: bug4862_027_03

Things seem to work more smoothly here.

One problem: If the circuit to an IP fails, and we try to retry our connection to it, this retry attempt is subject to cannibalization, which means that we might end up having a random IP instead of the one we wanted. This is related to #16260.

comment:41 Changed 3 years ago by asn

Two very minor things:

  • Maybe we should change the following log message:
        log_info(LD_REND, "Giving up on %s as intro point for %s"
                 " (circuit disappeared).",
                 safe_str_client(extend_info_describe(intro->extend_info)),
                 safe_str_client(service->service_id));
    
    since now we attempt a retry instead of giving up on the circuit.
  • There is this comment that is also not quite true anymore:
          /* This funcion will be called again by the main loop so this intro
           * point without a intro circuit will be removed. */
    

comment:42 in reply to:  41 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

Replying to asn:

Two very minor things:

  • Maybe we should change the following log message:
        log_info(LD_REND, "Giving up on %s as intro point for %s"
                 " (circuit disappeared).",
                 safe_str_client(extend_info_describe(intro->extend_info)),
                 safe_str_client(service->service_id));
    
    since now we attempt a retry instead of giving up on the circuit.
  • There is this comment that is also not quite true anymore:
          /* This funcion will be called again by the main loop so this intro
           * point without a intro circuit will be removed. */
    

Good catch. I fixed both hopefully correctly. See top commit of the new branch. For comment:40, #16260 has been merged so we won't end up changing the IP.

See branch: bug4862_027_04

comment:43 Changed 3 years ago by asn

Nick, I have also reviewed the branch and it seems ready to merge.

comment:44 Changed 3 years ago by arma

Roger's status: I'm excited to see this intro point variance bug fixed, and also I hear we get a bonus fix for onion services on mobile where they get to keep their intro points when the network hiccups. I worry that we're changing our design in a bunch of ways that contradict the old design -- I should in my spare time look through the comments for the new stuff and try to identify design changes we just made without realizing we were making them. But none of those steps should block merging this code if everybody likes it.

comment:45 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Agreed with patch; agreed with arma. Closing this and squashing and merging. Also closing #13483 and #8239 and #8864 .

comment:46 Changed 3 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.