[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #7157 [Tor]: "Low circuit success rate %u/%u for guard %s=%s."
#7157: "Low circuit success rate %u/%u for guard %s=%s."
-----------------------------------------+----------------------------------
Reporter: arma | Owner:
Type: enhancement | Status: needs_revision
Priority: normal | Milestone: Tor: 0.2.4.x-final
Component: Tor | Version:
Keywords: tor-client, MikePerry201212 | Parent: #5456
Points: | Actualpoints: 19
-----------------------------------------+----------------------------------
Comment(by mikeperry):
Replying to [comment:17 nickm]:
> Since it's one branch, I'll review the branch in one place.
>
> This is part one of the review: I need to take a break now. It covers
up to but
> not including 721f7e375114abfc, with a little skipping around in in the
diff
> that I've been reading side-by-side to see what stays and what gets
fixed.
Ok. If I don't reply to a comment, assume it is simply fixed.
> pathbias_get_dropguards -- I think it's supposed to give a boolean, bxut
it's
> taking its input in range 0..100 and dividing it by 100.0. Assuming I'm
> right about it being a boolean, you need to chance its type in config.c
to
> "autobool", so that setting it to "auto" can mean "use the value from
the
> consensus".
Ok. I fixed this, but do we want the AUTO prefix for any of the other path
bias configs? We all want them to default to consensus values, I think?
> Do we verify that scale_factor strictly less than mult_factor? It looks
like
> in ef1b830ef8d751172ebe5 we removed the requirement, making it so
> scale_factor == mult_factor is allowed.
I think this is fine. It doesn't cause damage if this happens, it merely
allows us to disable scaling entirely if we want, right?
> pathbias_count_successful_close (and others that rely on
> entry_guard_get_by_id_digest) is going to hit its LD_BUG whenever we
have a
> node stop being a guard after we open a circuit through it.
Hrmm. Suggestions here? The log is at info, but if we know it can happen,
I guess you mean to say it should be a different log domain?
Is there anything better we can do to handle that case?
> I'm concerned about what happens with circuits that are around when Tor
dies.
> Do they count as successes, or failures, or not at all? I think right
now
> they're counted as never having closed sucessfully; is that so? It
seems
> kind of odd to compare "successfully closed circuits" to "circuits for
which
> we've been the first hop."
No, we give currently open circuits the benefit of the doubt. See
pathbias_get_closed_count(). We count currently opened circuits as
successfully closed for that guard whenever we evaluate our counts or
write the state file.
> This business with counting streams successfully sent on a circuit,
using end
> stream reasons to say which are okay and which aren't, and so on... is
it
> specified anywhere ? I'm not seeing it in proposal 209.
No, it is based on discussions with Rob Jansen after he reviewed Prop209.
I haven't documented it yet. It's closely related to #7691.. Should I
document both of those in the proposal, or try to condense the whole thing
into a spec update?
> The comment on any_streams_succeeded seems wrong; it's not SOCKS
streams,
> it's any client streams, right?
This got cleaned up and refactored into a path_state_t flag in later
commits.
> "and it shat itself" ? [in connection_ap_process_end_not_open()]
Past tense of "to shit [oneself]". ;)
While writing #7691, I actually discovered that exits will send back
END_STREAM_REASON_TORPROTOCOL if you send them utter garbage (I used the
wrong hop's cpath to send them the cell at first). However, after I looked
closer at the code, it seems quite possible for the other two reason codes
to come back for more subtle precision tagging.
> Gcc gives me these warnings:
>
> {{{
> src/or/circuitbuild.c:1020: warning: no previous prototype for
âpathbias_get_warn_rateâ
> src/or/circuitbuild.c: In function âpathbias_get_dropguardsâ:
> src/or/circuitbuild.c:1058: warning: implicit conversion shortens 64-bit
value into a 32-bit value
> src/or/circuitbuild.c: In function âentry_guard_inc_circ_attempt_countâ:
> src/or/circuitbuild.c:1669: warning: cast from function call of type
âdoubleâ to non-matching type âintâ
> src/or/circuitbuild.c:1688: warning: cast from function call of type
âdoubleâ to non-matching type âintâ
> src/or/circuitbuild.c:1706: warning: cast from function call of type
âdoubleâ to non-matching type âintâ
> src/or/circuitbuild.c:1725: warning: cast from function call of type
âdoubleâ to non-matching type âintâ
> make[1]: *** [src/or/circuitbuild.o] Error 1
> cc1: warnings being treated as errors
> src/or/connection_edge.c: In function
âconnection_ap_handshake_socks_replyâ:
> src/or/connection_edge.c:2191: warning: format â%ldâ expects type âlong
intâ, but argument 5 has type âuint64_tâ
> make[1]: *** [src/or/connection_edge.o] Error 1
> cc1: warnings being treated as errors
> src/or/entrynodes.c: In function âentry_guards_update_stateâ:
> src/or/entrynodes.c:1197: warning: comparing floating point with == or
!= is unsafe
>
> }}}
Yikes. Didn't realize we made the default build warningless.
All of the above fixes should be pushed to their own 'part 1' review
commit shortly.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7157#comment:19>
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