[tor-bugs] #25705 [Core Tor/Tor]: Refactor circuit_build_failed to separate build vs path failures

[Tor Bug Tracker & Wiki]

#25705: Refactor circuit_build_failed to separate build vs path failures
--------------------------+--------------------------------
 Reporter:  mikeperry     |          Owner:  (none)
     Type:  defect        |         Status:  needs_revision
 Priority:  Medium        |      Milestone:
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:  #25546        |         Points:
 Reviewer:                |        Sponsor:  SponsorV-can
--------------------------+--------------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:1 mikeperry]:
 > Ok, I made the smallest possible change here. I just moved the path
 length check up from the guard failure block to the beginning of the
 function. The only thing that wasn't build failure accounting or
 node/network blaming was the HS RP retry code, which I also moved up.
 >
 > I also removed the destroy check, as per #25347, as a separate commit.
 We can decide which one we like better. On the one hand, asn's branch has
 more checks and might be safer. On the other hand, this is a smaller
 change, and keeps everything related to counting circuit failures in the
 same place.
 >
 > mikeperry/bug25705

 Thanks for the patch here Mike! A github RP would be helpful so that I can
 comment inline, but I'll just do it here for now:

 - `4c64811d0b`: Looks reasonable to me. I find it kinda naughty that we
 special handle S_CONNECT_REND twice in that function, but I didnt find a
 way to improve this in a straightforward way.

 - `a2d44546c2`: A few things here:

 - Shouldn't we do the `already_marked` computation even when
 `circ->base_.received_destroy`, so that we don't double-mark? Since, IIUC
 the only reason we distinguish between receiving `DESTROY` or not now, is
 so that we can do better logging. I think that also has the potential to
 simplify the code a bit. Ideally I think that whole block should go into
 its own function.

 - Do we want to mark the guard as bad for any `DESTROY` reason? Is there a
 reason we wouldn't do that? Do we remember our old rationale here. Seems
 like the commit that introduced the ''don't do anything if DESTROY'' logic
 is  Roger in `5de88dda0acda6914bacd1e14c2e037798c5d9d9`.

 - Do we want to merge this patch without taking the precautions of Roger's
 points `G` and `E` from #25347?

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