[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25705 [Core Tor/Tor]: Refactor circuit_build_failed to separate build vs path failures
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs