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

Tue Dec 18 20:19:43 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 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