[tor-bugs] #7157 [Tor]: "Low circuit success rate %u/%u for guard %s=%s."

Tue Dec 18 21:10:20 UTC 2012

#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 nickm):

 Replying to [comment:19 mikeperry]:
 > > 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?

 The AUTOBOOL thing is special.  For numeric values, you can do what we're
 doing and have a special "out of bounds" value that indicates .  But
 regular booleans only take one value.

 > > 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?

 I think that's okay so long as we definitely prevent scale_factor >
 mult_factor.

 > > 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?

 Well, it's not a bug.  A different log domain seems in order. I'm not sure
 that other fixes are needed; will the code still work in 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?

 A short little email to tor-dev to follow-up on the proposal, or whatever
 would be fastest for you, would be cool.  Just so there's some record of
 what you're doing and why.

 > > 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.

 Okay.
 > > "and it shat itself" ? [in connection_ap_process_end_not_open()]
 >
 > Past tense of "to shit [oneself]". ;)

 Yeah, but the exit didn't shit itself.  It just gave an error.

 I'm not against cussing in software, but this would be the first of George
 Carlin's Dirty Seven in Tor, and I'd like to save that for a more special
 occasion.

 > 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.

 -Wall, not warningless.  It's been that way for a while, I thought.

 Maybe --enable-gcc-warnings-advisory (or whatever it's called) should be
 default.

 > 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:20>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online