[tor-bugs] #4862 [Tor]: Consider disabling dynamic intro point formula (numerology)

[Tor Bug Tracker & Wiki]

#4862: Consider disabling dynamic intro point formula (numerology)
-------------------------+-------------------------------------------------
     Reporter:  hellais  |      Owner:
         Type:           |     Status:  needs_revision
  enhancement            |  Milestone:  Tor: 0.2.7.x-final
     Priority:  major    |    Version:  Tor: 0.2.7
    Component:  Tor      |   Keywords:  needs-proposal, tor-hs,
   Resolution:           |  027-triaged-1-in, SponsorR
Actual Points:           |  Parent ID:
       Points:           |
  medium/large           |
-------------------------+-------------------------------------------------

Comment (by asn):

 Replying to [comment:35 dgoulet]:
 > Replying to [comment:34 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.)?

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