hs-v3: Consider flagging an intro point as bad if rendezvous fails multiple times

This comes from #25882 (moved).

Should a client claim that an IP is failing if the RP never connected? In other words, handling the CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED purpose when cleaning up a circuit.

I would think that having a maximum amount of retries let say (arbitrarily) after 2 times that you introduce, the IP sends back the ACK but then you end up timing out the RP circuit, we would flag the IP in the failure cache and move on to a new IP.

This would not be a "catch all" issues with connectivity but would at least zone out buggy or malicious IP that do not relay the introduction.

changed milestone to %Tor: 0.4.4.x-final

added component::core tor/tor milestone::Tor: 0.4.4.x-final owner::neel points::0.2 priority::medium reviewer::dgoulet severity::normal sponsor::27-can status::needs-review tor-circuit tor-hs type::defect labels

Trac:
Status: new to assigned
Owner: N/A to neel
Cc: N/A to neel

Fix keywords.

Trac:
Keywords: -tor-hs deleted, tor-hs added

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

I just handle CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED on cleanup in here. Is that okay, or is there more I need to do?

Trac:
Status: assigned to needs_review

I think this PR needs a changes file?

Trac:
Milestone: Tor: unspecified to Tor: 0.4.3.x-final

I don't believe so, as this feature was introduced in the commits 7f83c43594dcf13fb04352f5faa8db2cd86354c1 and cbc495453cb522db1584525a06d26386f5819e05, and aren't introduced in a released version yet (from the information I got on GitHub).

Sources:

• https://github.com/torproject/tor/commit/cbc495453cb522db1584525a06d26386f5819e05

• https://github.com/torproject/tor/commit/7f83c43594dcf13fb04352f5faa8db2cd86354c1

The changes file for #32020 (moved) does not mention failing rendezvous points, so we need another changelog entry for this ticket: https://github.com/torproject/tor/blob/master/changes/ticket32020

Trac:
Status: needs_review to needs_revision

Made the change. Setting as needs review.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to dgoulet
Status: needs_review to needs_revision

Made the changes. Setting as needs review.

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

Old PR has merge conflicts. New PR: https://github.com/torproject/tor/pull/1653

tag all currently needs_review tickets with 043-can. (Since there's code before the feature freeze, maybe we can take it.)

Trac:
Keywords: N/A deleted, 043-can added

Trac:
Status: needs_review to needs_revision
Milestone: Tor: 0.4.3.x-final to Tor: unspecified
Keywords: 043-can deleted, N/A added

New simplified PR: https://github.com/torproject/tor/pull/1682

No tests, however.

Trac:
Milestone: Tor: unspecified to Tor: 0.4.4.x-final
Status: needs_revision to needs_review

Hmm chceked the PR and made a small comment.

I'm generally concerned here about the lack of unittests.

Also I'd appreciate a torspec branch (or a thorough post on this ticket) that analyzes the effects of this new behavior. For example, what is the old behavior vs the new behavior in the following two scenarios:

• Intro node is broken and does not forward rend requests.
• Service is overloaded and cannot handle rendezvous.

Trac:
Status: needs_review to needs_revision

Sorry for the delay.

I have extended the ticket changes writeup in a fixup commit.

I do not have tests, I don't know of any test that calls hs_client_circuit_cleanup_on_free()

Also, I did not see your comment

Trac:
Status: needs_revision to needs_review

changed time estimate to 1h 36m

moved to tpo/core/tor#32614 (closed)