Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  defect   | Status:  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-

Comment (by nickm):

 Looks good to me too.  The code is tricky, but the coverage is complete.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  defect   | Status:  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => merge_ready


Comment:

 LGTM!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * status:  accepted => needs_review


Comment:

 Branch: `ticket31652_042_01`
 PR: https://github.com/torproject/tor/pull/1401

 The patch has changed quite a bit but the logic is the same.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  defect   | Status:  accepted
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * keywords:  tor-hs, tor-circuit, 042-should, dgoulet-merge => tor-hs, tor-
 circuit, 042-should


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  defect   | Status:
 |  accepted
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should, |  Actual Points:
  dgoulet-merge  |
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * status:  needs_revision => accepted
 * owner:  neel => dgoulet


Comment:

 Now that we now what to do, I can take it up.

 Thanks neel!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should, |  Actual Points:
  dgoulet-merge  |
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-

Comment (by dgoulet):

 Yes apology here... I clearly was very confused or made a typo but this
 was misleading:

 https://github.com/torproject/tor/pull/1315#pullrequestreview-287606815

 comment:23 is the right way imo.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should, |  Actual Points:
  dgoulet-merge  |
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-

Comment (by asn):

 Yes, dgoulet I think you are right.

 Given your feedback I think the behavior before
 
https://github.com/torproject/tor/pull/1315/commits/5c26c656a2037f3b264d8c7609621450e6e46293
 is the right one.

 But we need a unittest that **actually** tests this feature.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should, |  Actual Points:
  dgoulet-merge  |
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * status:  merge_ready => needs_revision


Comment:

 I think we can _not_ do this:

 {{{
   if (hs_circ_service_get_intro_circ(ip) && over_max_retries) {
 return true;
   }
 }}}

 If we have a pending intro circuit for that `ip` and we are over the max
 retries, this means that the pending intro circuit is the one that will be
 used or will fail.

 So returning `true` means that it will be removed from the service list
 and will lead to a lost lingering intro circuit that once established,
 we'll hit an error because we can't find the "ip" object.

 If we have one established or in the process of establishing one, NEVER
 remove the IP is what I think we should do.

 If we have no circuits at all and we are over the limit, remove it.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should, |  Actual Points:
  dgoulet-merge  |
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => merge_ready
 * keywords:  tor-hs, tor-circuit, 042-should => tor-hs, tor-circuit,
 042-should, dgoulet-merge


Comment:

 I think that looks reasonable. Thanks!

 That said, if you have a unittest that does not fail after changing the
 behavior of the function, your unittest is not testing the function well
 enough. That's what I tried to say at comment:18.

 Anyhow, I think that's OK for now tho.

 I will mark this as merge_ready, and put David as the merger so that he
 takes a second look as well.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 I fixed the issue while allowing tests to pass.

 However, I believe my code is tested with the test routine
 `test_service_event()`:

 {{{
 /* Now, we'll create an IP with a registered circuit. The IP object
  * shouldn't go away. */
 ip = helper_create_service_ip();
 service_intro_point_add(service->desc_current->intro_points.map, ip);
 ed25519_pubkey_copy(>hs_ident->intro_auth_pk,
 >auth_key_kp.pubkey);
 hs_circuitmap_register_intro_circ_v3_service_side(
  circ, >auth_key_kp.pubkey);
 run_housekeeping_event(now);
 tt_int_op(digest256map_size(service->desc_current->intro_points.map),
   OP_EQ, 1);
 /* We'll mangle the IP object to expire. */
 ip->time_to_expire = now;
 run_housekeeping_event(now);
 tt_int_op(digest256map_size(service->desc_current->intro_points.map),
   OP_EQ, 0);
 }}}

 Setting as needs review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-

Comment (by asn):

 Replying to [comment:19 neel]:
 > To clarify, is the bug you are talking about related to removing the
 intro point if `hs_circ_service_get_intro_circ()` is true? If not, what is
 it?

 Yep! Before `861ff757712d008424746e9d1e9bd85b3f472dee` we used to return
 `True` if `hs_circ_service_get_intro_circ(ip)` is true. After that commit,
 we return `False`.
 This seems like a behavior change and not just refactoring.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-

Comment (by neel):

 To clarify, is the bug you are talking about related to removing the intro
 point if `hs_circ_service_get_intro_circ()` is true? If not, what is it?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 OK this is much better. I really understand the logic now. Thanks!

 That said, you changed the behavior of the code in the latest iteration
 which I was not expecting (I was just expecting documentation). In
 particular, the code now avoids removing the intro circ if
 `hs_circ_service_get_intro_circ()` is true (which makes sense!) whereas
 before it was doing the exact opposite.

 How could you fix that bug without the unittest noticing at all? Can you
 please enrich the unittest so that it catches such bugs in the future?

 We are almost there!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 I have clarified the code, calling the new function
 `should_remove_intro_point()` and simplified the logic.

 The code works this way:

 If this statement is true:

 {{{
 if (ip->circuit_established || hs_circ_service_get_intro_circ(ip))
 }}}

 We return `false` to not destroy the circuit. Otherwise, we return this
 test case:

 {{{
   return (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES);
 }}}

 Setting as needs review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Neel, I'm still confused here...

 Why does a function called `should_not_retry_intro_point()` allow the
 possibility of a retry when we have gone over the number of maximum
 retries? In my view, it should always return `false` in that case.

 In particular the following block:
 {{{
   /* If we have gone over the number of retried circuits, make sure we
 don't
* already have an established circuit. */
   if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) {
 return !ip->circuit_established || hs_circ_service_get_intro_circ(ip);
   }
 }}}
 is confusing me even tho I've read it a few times. When I'm reading that
 function my logic would be "If we have gone over the number of retried
 circuits, we only allow retries if ...", but then why would we allow
 retries if we are past the max?  Also the comment mentions the established
 circuit clause, but not the `hs_circ_service_get_intro_circ(ip);` one. Can
 you please clarify that logic further? :/

 We seem to have left some comments in `cleanup_intro_points()` that are
 only relevant to `should_not_retry_intro_point()`.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 I made the corrections.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Unittest looks reasonable, but I have a question on the naming of the
 function and the overall logic. Maybe some documentation to clarify is
 missing?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Thanks for the fixups. I think that `if` block in `cleanup_intro_points()`
 has grown to an unruly complexity which is hard to understand what it's
 checking and why anymore. Can you please split into a separate function,
 split all the clauses into different `if` statements, and document them
 individually?

 Thanks! :)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * reviewer:  dgoulet => asn


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  dgoulet  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  dgoulet  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Hmm, seems some test failed and CI is sad. (Didn't do any source code
 review)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  dgoulet  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 Fixed it!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
-+-
 Reporter:  dgoulet  |  Owner:  neel
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200   | Points:  0.1
 Reviewer:  dgoulet  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Close but slight problem!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  neel
 Type:  defect  | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs tor-circuit  |  Actual Points:
Parent ID:  #30200  | Points:  0.1
 Reviewer:  dgoulet |Sponsor:  Sponsor27-must
+
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 New PR: https://github.com/torproject/tor/pull/1315

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  neel
 Type:  defect  | Status:  needs_revision
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs tor-circuit  |  Actual Points:
Parent ID:  #30200  | Points:  0.1
 Reviewer:  dgoulet |Sponsor:  Sponsor27-must
+
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Comment on PR

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  neel
 Type:  defect  | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs tor-circuit  |  Actual Points:
Parent ID:  #30200  | Points:  0.1
 Reviewer:  dgoulet |Sponsor:  Sponsor27-must
+
Changes (by dgoulet):

 * reviewer:  asn => dgoulet


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  neel
 Type:  defect  | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs tor-circuit  |  Actual Points:
Parent ID:  #30200  | Points:  0.1
 Reviewer:  asn |Sponsor:  Sponsor27-must
+
Changes (by neel):

 * status:  assigned => needs_review


Comment:

 PR: https://github.com/torproject/tor/pull/1297

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  neel
 Type:  defect  | Status:  assigned
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs tor-circuit  |  Actual Points:
Parent ID:  #30200  | Points:  0.1
 Reviewer:  asn |Sponsor:  Sponsor27-must
+
Changes (by neel):

 * owner:  (none) => neel
 * cc: neel (added)
 * status:  new => assigned


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
+
 Reporter:  dgoulet |  Owner:  (none)
 Type:  defect  | Status:  new
 Priority:  Medium  |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  |   Keywords:  tor-hs tor-circuit
Actual Points:  |  Parent ID:  #30200
   Points:  0.1 |   Reviewer:  asn
  Sponsor:  Sponsor27-must  |
+
 Context: Lets say a service has 3 intro circuits opened and stable.

 Now, imagine one circuit collapses, like for instance the intro point
 restarted "tor" after an update. Our code is designed to retry 3 times
 that is once every second and then give up on the intro point.

 What it looks like:

 Every second, `run_build_circuit_event()` is run and launches intro
 circuit(s) if we are missing any. For each IP, it will increment the
 `circuit_retries` counter but does not actually look at it to decide to
 launch or not.

 Before that event, also every 1 second, `cleanup_intro_points()` checks
 that every intro point has not expired, fell off the consensus or that
 `circuit_retries` is greater than (>) `MAX_INTRO_POINT_CIRCUIT_RETRIES =
 3`.

 Putting this together, imagine now that the first 3 attempts failed for
 whatever reasons and then we launch a 4th one because `circuit_retries =
 3`, it does pass validation of `> MAX_INTRO_POINT_CIRCUIT_RETRIES` so then
 a circuit is launched and that very one succeeds.

 Because `circuit_retries` is now 4 then the next second,
 `cleanup_intro_points()` removes the IP and closes the valid open
 established circuit...

 I've observed the above a surprising amount of time because booting a tor
 relay takes some seconds mostly due to the diff-cache parsing.

 In a nutshell, we should NOT launch a circuit if we reached the max
 retries for an intro point. This back and forth of opening a circuit and
 then deciding that we went over the limit makes no sense in the first
 place.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs