[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 dgoulet):

 Replying to [comment:36 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?
 >

 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.

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