Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8081 closed defect (fixed)

7802 tweaks per code review

Reported by: andrea Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, MikePerry201301
Cc: Actual Points: 10
Parent ID: Points:
Reviewer: Sponsor:

Description

7802 accidentally got merged with 8024 before being reviewed; joint code review by nickm and myself followed. Nothing so objectionable as to merit reverting the merge, but a few points noted (full IRC transcript will be attached):

18:45 < athena> okay, in circuitbuild.c we've got some pretty straightforward functions to query parameters that look

just fine, then a long block of code which is probably where we should be focusing most attention

18:45 < nickm> Right. The "rate" configuration things are all percentages, but I don't see anything that keeps you from

configuring a value below zero or above 100 in config.c. We should note that. NOTE.

18:45 < nickm> (I'm going to grep for NOTE strings.)
18:45 < athena> okay, yeah, those should be checked and clamped, i think

*

18:48 < nickm> I don't like the name "pathbias_check_use_rate" -- "check" is pretty vague. NOTE.
18:48 < nickm> The return; at the end of that function offends me. NOTE.
18:48 < athena> yeah
18:49 < nickm> I think the comment for the next function (pathbias_mark_use_success) may be wrong; the "stat" in the

first sentence should probably be 'state'. and the word "mark" should appear before "it".

18:49 < nickm> NOTE.

*

18:53 < nickm> ooh. In mark_use_success, we check path_state < PATH_STATE_USE_ATTEMPTED. We should make sure that a

comment on path_state_t says that its values must never be reordered or else stuff might break. NOTE

18:54 < athena> did you mean to say pathbias_should_count? i'm not seeing a pathbias_should_close
18:54 < nickm> sorry, should_count
18:55 < athena> okay
18:55 < athena> looks like it ignores circuits that have some purposes, that are one-hop or checks some config options
18:55 < athena> do circuit purposes ever change during the lifetime of a circuit
18:55 < athena> ?
18:56 < nickm> I believe circuit purposes can change under some circumstances, though the rules are nontrivial
18:56 < athena> hmm
18:56 < nickm> maybe we should cache the result of this function if we've ever called it before?
18:56 < nickm> maybe we should warn if it changes?
18:57 < nickm> this isn't a new problem, if it's a problem
18:57 < athena> yeah, we need a detection for that if we can't be sure it doesn't happen
18:57 < nickm> NOTE. Let's ask mike?

*

19:04 < athena> well, the code moves the pathbias_send_usable_probe() to USE_ATTEMPTED; it seems nontrivial to evaluate

the effects of doing that

19:05 < athena> let's note that and move on, and ask mike about it when he's around
19:05 < nickm> right.
19:05 < athena> probably means we should settle on 'revert' for now if there's something like that in there that we

don't understand

19:06 < nickm> oh wait
19:06 < athena> ?
19:06 < nickm> read the description of 7802 again.
19:06 < nickm> he's splitting PATH_STATE_BUILD_SUCCEEDED into two states, where previously the distinction was not

based on state but based on timestamp_dirty

19:07 < athena> right
19:07 < athena> and the old probe did test timestamp_dirty
19:07 < athena> okay, this one's fine, then
19:07 < nickm> so that block that moved isn't the build_succeeded block; it's the build_succeeded && timestamp_dirty

block

19:07 < nickm> great
19:07 < nickm> and it now sets the state to ALREADY_COUNTED so we don't do this again. Sounds good.
19:08 < nickm> next two changes are function renames.
19:08 < athena> yeah
19:08 < nickm> Then the function now known as pathbias_get_close_success_count.
19:08 < nickm> Hm. path_state >= USE_FAILED and path_state >= BUILD_SUCCEEDED in the same function. I wish those were

macros or inline functions or something.

19:08 < nickm> NOTE above
19:09 < athena> yeah, making assumptions about enums like that always makes me feel icky
19:10 < nickm> (If you add a new value, you need to add it in the right position or there's hell to pay)
19:10 < athena> yeah\
19:11 < nickm> so the change in pathbias_get_closed_count is probably okay though I think.
19:12 < athena> yeah, agree
19:12 < nickm> agree?
19:12 < nickm> great

*

19:18 < nickm> pathbias_act_on_failure_rate?
19:18 < nickm> It doesn't return a boolean; it just messes with stuff and acts accordingly
19:18 < athena> it does, though
19:18 < nickm> oh, it does?
19:18 < athena> it returns -1 if it rejects the guard for a high failure rate
19:18 < nickm> Do we ever look at its return value?
19:18 < athena> or falls to the return 0 at the bottom of the function otherwise
19:18 < athena> dunno
19:19 < nickm> Looks like we call it in 2 places, and never check the return value there.
19:19 < athena> doesn't look like it
19:19 < athena> yeah, your suggestion then
19:19 < nickm> ok. NOTE let;s rename it to pathbias_act_on_failure_rate.
19:20 < athena> should we also get rid of the return value?
19:20 < nickm> Also NOTE that in the case where it sets any persistent value on a guard, it does need to call

entry_guards_changed I think

19:20 < nickm> yes we should. (NOTE)

*

19:21 < nickm> Hm. In the later part of the function that does scaling...
19:22 < nickm> ah, never mind.
19:22 < nickm> that looks okay to me if it does to you, since guard->use_{successes,attempts} are doubles.
19:23 < athena> yeah
19:23 < athena> it's just using a slightly awkward way of passing a rational
19:23 < nickm> We should check whether we enforce that scale_factor is at least 1, and mult_factor is less than or

equal to scale_factor.

19:23 < athena> but i'm not sure why, since if those are doubles we're already using floating point anyway
19:23 < athena> could just be a single function returning another double?
19:23 < nickm> (NOTE)
19:23 < nickm> I think it should be.
19:24 < nickm> I think he's doing it this way because consensus parameters can only be ints
19:24 < nickm> (int32_t, I think)
19:24 < athena> also make sure that we never divide by zero
19:24 < athena> oh, right
19:24 < nickm> Right. (NOTE wrt the refactoring though)
19:24 < athena> so it'd just be a pain to extend that to allow double, then
19:25 < nickm> yeah, but we can have the function return a double, and let the user configure a double themselves if

they want to override it in torrc

19:25 < nickm> (It would be crazy to override the numerator but not the denominator)
19:25 < athena> yeah
19:26 < athena> agreed; should be one function calculating the ratio of two int32_t consensus params *or* pulling a

double out of the config file

19:26 < nickm> Right. Let's do it like that. NOTE

*

19:26 < nickm> on to pathbias_check_close_rate. That should probably get renamed too.
19:27 < athena> pathbias_act_on_close_rate?
19:27 < nickm> yeah. That one _does_ have its return value checked.
19:27 < nickm> (lots of duplicate code here too I think)
19:27 < nickm> Does this look like duplicate code to you? Does a later commit fix that?
19:28 < athena> hold on, lemme look
19:29 < athena> yeah, it's kinda duplicated
19:29 < athena> but it's calling different functions in the log messages (get_use_success_count() vs.

get_close_success_count())

19:30 < athena> getting rid of the duplication would mean passing some function pointers around, probably
19:30 < athena> and i'm not sure where it gets scale_factor from
19:31 < nickm> Or having one block at the top of the function that says if (counting_closed) {

success_count=get_close_success_count() } else {success_count=get_use_success_count();} and avoid the
function pointers

19:31 < athena> yeah, okay
19:31 < athena> then should refactor to have less duplicate code i think
19:32 < nickm> The scaling block is pretty different
19:32 < nickm> isn't scale_factor coming from pathbias_get_scale_factor ?
19:32 < athena> hmm
19:32 < nickm> I hope everything scaled in that block is also a double, or our idea of having one function that returns

a double is going to break.

19:33 < athena> yeah
19:34 < athena> well, we could split the scaling out into separate rescale_<use,close> functions, and handle them like

the success_count thing?

19:35 < nickm> Seems good. Also Given that there are two sets of values that get scaled independently, we should make

sure that their documentation says which block is which, clearly.

19:35 < nickm> NOTE on the refactor and NOTE on the documentation

*

19:41 < nickm> I don't like the check on the return value of pathbias_check_close_rate(). That function only returns

-1 the first time it decides to disable the guard, right?

19:41 < nickm> maybe it should call it, then check whether guard->path_bias_disabled is true? (NOTE)

*

19:45 < nickm> Hm. I wonder what initializes node->use_attempts and node->use_successes and the other doubles if this

part isn't called.

19:46 < nickm> If they all come from a malloc_zero() or a memset, that's a little tricky. Does memset(&dbl, 0,

sizeof(double)) get you a good zero?

19:46 < athena> not on things that aren't IEEE-754 in general
19:47 < nickm> hm. we proably have other places where we do this.
19:47 < athena> yeah
19:47 < athena> it is theoretically a portability issue, but it's gotta be pretty rare to run into a machine like that
19:48 < athena> the only examples of non-754 fp i can think of off-hand are VAX and some older crays
19:48 < nickm> We could treat it like systems where memset(&ptr, 0, sizeof(void*)) doesn't set ptr to NULL.
19:48 < athena> yeah
19:48 < nickm> NOTE. let's do that.

*

19:57 < nickm> athena: this code looks okay to me, but I wonder if it wouldn't be better to change the check to call

one of the pathbias_act_on_foo_rate() functions instead to eliminate duplicated code. What do you think?

19:58 < athena> probably
19:59 < nickm> athena: I think this one isn't super-urgent, but we should still NOTE it. agree?

*

20:06 < athena> 24b9b9f791defcb6156c587a035fde58c35a46d9 next?
20:06 < nickm> yes let's
20:07 < nickm> The two blocks look duplicated, but there are only two
20:08 < athena> yeah
20:08 < athena> it could still be made into two calls to a single duplication-free function, though
20:08 < nickm> agreed.
20:08 < nickm> NOTE
20:08 < athena> a2db17a1aab77c4183f589815800a779a5f24524 ?
20:08 < nickm> hang on. So, if even one stream is detached, we roll back to USE_ATTEMPTED?
20:09 < nickm> What if there are multiple streams and only one is detached?
20:09 < nickm> It doesn't seem too harmful, but we should ask mike IMO.
20:09 < athena> hmm
20:09 < athena> yeah
20:09 < nickm> NOTE let's ask him

*

20:11 < nickm> a2db17a1aab77c4183 ?
20:11 < athena> yeah
20:14 < nickm> doesn't seem crazy. I don't get it though.
20:14 < nickm> I wonder if the comment in circuit_launch_by_extend_info before the line that's being removed is now inaccurate. NOTE

*

20:21 < nickm> so, the next commit does the common code refactoring you mentioned before when it adds

pathbias_count_circs_in_states

20:22 < nickm> also NOTE that the pairs of arguments to pathbias_count_circs_in_states are good things to document in

the documentation for path_state_t

20:22 < athena> yeah, agree
20:22 < nickm> As for the part of this change that changes how scaling happens... how does that look to you?
20:24 < athena> it looks correct to me
20:25 < nickm> I personally would kind of prefer it if we didn't add things to {circ,use}_{attempts,successes} until we

had the corresponding changes in the other scaled variables. then this commit wouldn't be needed.
that's a potentially bigger refactoring though. NOTE it for later?

Child Tickets

Attachments (1)

7802-review.txt (24.3 KB) - added by andrea 7 years ago.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by andrea

Attachment: 7802-review.txt added

comment:1 in reply to:  description ; Changed 7 years ago by mikeperry

I'm working on the above. Replying only to segments where I have questions/comments/disagreements.

Replying to andrea:

18:54 < athena> did you mean to say pathbias_should_count? i'm not seeing a pathbias_should_close
18:54 < nickm> sorry, should_count
18:55 < athena> okay
18:55 < athena> looks like it ignores circuits that have some purposes, that are one-hop or checks some config options
18:55 < athena> do circuit purposes ever change during the lifetime of a circuit
18:55 < athena> ?
18:56 < nickm> I believe circuit purposes can change under some circumstances, though the rules are nontrivial
18:56 < athena> hmm
18:56 < nickm> maybe we should cache the result of this function if we've ever called it before?
18:56 < nickm> maybe we should warn if it changes?
18:57 < nickm> this isn't a new problem, if it's a problem
18:57 < athena> yeah, we need a detection for that if we can't be sure it doesn't happen
18:57 < nickm> NOTE. Let's ask mike?

Hrmm. We could either create a new path_state or re-use PATH_STATE_ALREADY_COUNTED (and maybe rename it?), then log if pathbias_should_count() ever gets called with a path_state of PATH_STATE_ALREADY_COUNTED.

This should future proof us from additional purpose transitions, but I'm pretty sure the hidserv purposes we're really concerned about can't be transitioned out of currently.

19:26 < nickm> on to pathbias_check_close_rate. That should probably get renamed too.
19:27 < athena> pathbias_act_on_close_rate?

I'm not sure I like act_on, but I agree it's better than check. I will refactor out the scaling code and see if I can think of a better name that captures the logging and guard-dropping idea. Perhaps evaluate? Measure?

19:27 < nickm> yeah. That one _does_ have its return value checked.
19:27 < nickm> (lots of duplicate code here too I think)
19:27 < nickm> Does this look like duplicate code to you? Does a later commit fix that?
19:28 < athena> hold on, lemme look
19:29 < athena> yeah, it's kinda duplicated
19:29 < athena> but it's calling different functions in the log messages (get_use_success_count() vs.

get_close_success_count())

19:30 < athena> getting rid of the duplication would mean passing some function pointers around, probably
19:30 < athena> and i'm not sure where it gets scale_factor from
19:31 < nickm> Or having one block at the top of the function that says if (counting_closed) {

success_count=get_close_success_count() } else {success_count=get_use_success_count();} and avoid the
function pointers

19:31 < athena> yeah, okay
19:31 < athena> then should refactor to have less duplicate code i think
19:32 < nickm> The scaling block is pretty different
19:32 < nickm> isn't scale_factor coming from pathbias_get_scale_factor ?
19:32 < athena> hmm
19:32 < nickm> I hope everything scaled in that block is also a double, or our idea of having one function that returns

a double is going to break.

19:33 < athena> yeah
19:34 < athena> well, we could split the scaling out into separate rescale_<use,close> functions, and handle them like

the success_count thing?

19:35 < nickm> Seems good. Also Given that there are two sets of values that get scaled independently, we should make

sure that their documentation says which block is which, clearly.

19:35 < nickm> NOTE on the refactor and NOTE on the documentation

The log messages and thresholds are actually all different between the two rate check functions here. The use bias stuff only has two thresholds (notice and warn+drop), because use failures proved exceedingly rare and nowhere near as sensitive to network conditions as build failures.. I think we want to keep the two logging functions separate for clarity for that reason, but I agree that refactoring out the scaling is a good plan. I also think that should also be two different functions rather than one blob with a conditional check (and probably another enum/define) though.

19:45 < nickm> Hm. I wonder what initializes node->use_attempts and node->use_successes and the other doubles if this

part isn't called.

19:46 < nickm> If they all come from a malloc_zero() or a memset, that's a little tricky. Does memset(&dbl, 0,

sizeof(double)) get you a good zero?

19:46 < athena> not on things that aren't IEEE-754 in general
19:47 < nickm> hm. we proably have other places where we do this.
19:47 < athena> yeah
19:47 < athena> it is theoretically a portability issue, but it's gotta be pretty rare to run into a machine like that
19:48 < athena> the only examples of non-754 fp i can think of off-hand are VAX and some older crays
19:48 < nickm> We could treat it like systems where memset(&ptr, 0, sizeof(void*)) doesn't set ptr to NULL.
19:48 < athena> yeah
19:48 < nickm> NOTE. let's do that.

What does 'that' mean here? Hopefully "pretend they no longer exist"? :)

20:08 < athena> a2db17a1aab77c4183f589815800a779a5f24524 ?
20:08 < nickm> hang on. So, if even one stream is detached, we roll back to USE_ATTEMPTED?
20:09 < nickm> What if there are multiple streams and only one is detached?
20:09 < nickm> It doesn't seem too harmful, but we should ask mike IMO.
20:09 < athena> hmm
20:09 < athena> yeah
20:09 < nickm> NOTE let's ask him

Yes, the idea is that we should probe these instances to ensure that the circuit is still actually viable after the detach. Rolling back the state to USE_ATTEMPTED will induce such a probe upon close. I mention this in the commit message, but I will also add that explanation to the refactored function.

20:21 < nickm> so, the next commit does the common code refactoring you mentioned before when it adds

pathbias_count_circs_in_states

20:22 < nickm> also NOTE that the pairs of arguments to pathbias_count_circs_in_states are good things to document in

the documentation for path_state_t

20:22 < athena> yeah, agree
20:22 < nickm> As for the part of this change that changes how scaling happens... how does that look to you?
20:24 < athena> it looks correct to me
20:25 < nickm> I personally would kind of prefer it if we didn't add things to {circ,use}_{attempts,successes} until we

had the corresponding changes in the other scaled variables. then this commit wouldn't be needed.
that's a potentially bigger refactoring though. NOTE it for later?

Yeah, that might be a better approach. I will put an XXX in the scaling code mentioning that we may want to have separate temporary "pending" counters in the entry_guard_t that don't get added to the attempt counts until circuit close, but I'm not sure we want to do this change at this point as it may have unexpected side effects... Or maybe we should create a separate ticket for this even... Hrmm..

comment:2 in reply to:  1 ; Changed 7 years ago by nickm

Replying to mikeperry:

I'm working on the above. Replying only to segments where I have questions/comments/disagreements.

Thanks!

By the way, as you address these, PLEASE do separate commits for separate issues. It's hard to review the stuff that some people do where there's just one commit called "respond to nick's review."

(I think you've gotten good about this, but easier to remind now than to split up later.)

Replying to andrea:

18:54 < athena> did you mean to say pathbias_should_count? i'm not seeing a pathbias_should_close
18:54 < nickm> sorry, should_count
18:55 < athena> okay
18:55 < athena> looks like it ignores circuits that have some purposes, that are one-hop or checks some config options
18:55 < athena> do circuit purposes ever change during the lifetime of a circuit
18:55 < athena> ?
18:56 < nickm> I believe circuit purposes can change under some circumstances, though the rules are nontrivial
18:56 < athena> hmm
18:56 < nickm> maybe we should cache the result of this function if we've ever called it before?
18:56 < nickm> maybe we should warn if it changes?
18:57 < nickm> this isn't a new problem, if it's a problem
18:57 < athena> yeah, we need a detection for that if we can't be sure it doesn't happen
18:57 < nickm> NOTE. Let's ask mike?

Hrmm. We could either create a new path_state or re-use PATH_STATE_ALREADY_COUNTED (and maybe rename it?), then log if pathbias_should_count() ever gets called with a path_state of PATH_STATE_ALREADY_COUNTED.

This should future proof us from additional purpose transitions, but I'm pretty sure the hidserv purposes we're really concerned about can't be transitioned out of currently.

I'd prefer a simple adjunct variable to a new state. For example, a tristate that says whether pathbias_should_count() has returned true, false, or has neer been called. That way we can check our "it never changes" assumption without risking any more breakage.

19:26 < nickm> on to pathbias_check_close_rate. That should probably get renamed too.
19:27 < athena> pathbias_act_on_close_rate?

I'm not sure I like act_on, but I agree it's better than check. I will refactor out the scaling code and see if I can think of a better name that captures the logging and guard-dropping idea. Perhaps evaluate? Measure?

evaluate and measure seem okay to me, but they don't capture that the guard is going to get dropped potentially. Still, let's not waste our time brainstorming unless we come up with something awesome in the next N minutes of cogitation here.

19:27 < nickm> yeah. That one _does_ have its return value checked.
19:27 < nickm> (lots of duplicate code here too I think)
19:27 < nickm> Does this look like duplicate code to you? Does a later commit fix that?
19:28 < athena> hold on, lemme look
19:29 < athena> yeah, it's kinda duplicated
19:29 < athena> but it's calling different functions in the log messages (get_use_success_count() vs.

get_close_success_count())

19:30 < athena> getting rid of the duplication would mean passing some function pointers around, probably
19:30 < athena> and i'm not sure where it gets scale_factor from
19:31 < nickm> Or having one block at the top of the function that says if (counting_closed) {

success_count=get_close_success_count() } else {success_count=get_use_success_count();} and avoid the
function pointers

19:31 < athena> yeah, okay
19:31 < athena> then should refactor to have less duplicate code i think
19:32 < nickm> The scaling block is pretty different
19:32 < nickm> isn't scale_factor coming from pathbias_get_scale_factor ?
19:32 < athena> hmm
19:32 < nickm> I hope everything scaled in that block is also a double, or our idea of having one function that returns

a double is going to break.

19:33 < athena> yeah
19:34 < athena> well, we could split the scaling out into separate rescale_<use,close> functions, and handle them like

the success_count thing?

19:35 < nickm> Seems good. Also Given that there are two sets of values that get scaled independently, we should make

sure that their documentation says which block is which, clearly.

19:35 < nickm> NOTE on the refactor and NOTE on the documentation

The log messages and thresholds are actually all different between the two rate check functions here. The use bias stuff only has two thresholds (notice and warn+drop), because use failures proved exceedingly rare and nowhere near as sensitive to network conditions as build failures.. I think we want to keep the two logging functions separate for clarity for that reason, but I agree that refactoring out the scaling is a good plan. I also think that should also be two different functions rather than one blob with a conditional check (and probably another enum/define) though.

I think any code-refactoring you can do to eliminate the duplicate parts here would be welcome. If there's more apparent duplicate code that you think won't actually work to refactor, just leave an "XXXX can we eliminate any more duplicate code here?" comment and leave it at that.

19:45 < nickm> Hm. I wonder what initializes node->use_attempts and node->use_successes and the other doubles if this

part isn't called.

19:46 < nickm> If they all come from a malloc_zero() or a memset, that's a little tricky. Does memset(&dbl, 0,

sizeof(double)) get you a good zero?

19:46 < athena> not on things that aren't IEEE-754 in general
19:47 < nickm> hm. we proably have other places where we do this.
19:47 < athena> yeah
19:47 < athena> it is theoretically a portability issue, but it's gotta be pretty rare to run into a machine like that
19:48 < athena> the only examples of non-754 fp i can think of off-hand are VAX and some older crays
19:48 < nickm> We could treat it like systems where memset(&ptr, 0, sizeof(void*)) doesn't set ptr to NULL.
19:48 < athena> yeah
19:48 < nickm> NOTE. let's do that.

What does 'that' mean here? Hopefully "pretend they no longer exist"? :)

Treat it like we do systems where memset doesn't make pointers null: detect it and soil ourselves. I'll write this one as designated autoconf monkey.

20:08 < athena> a2db17a1aab77c4183f589815800a779a5f24524 ?
20:08 < nickm> hang on. So, if even one stream is detached, we roll back to USE_ATTEMPTED?
20:09 < nickm> What if there are multiple streams and only one is detached?
20:09 < nickm> It doesn't seem too harmful, but we should ask mike IMO.
20:09 < athena> hmm
20:09 < athena> yeah
20:09 < nickm> NOTE let's ask him

Yes, the idea is that we should probe these instances to ensure that the circuit is still actually viable after the detach. Rolling back the state to USE_ATTEMPTED will induce such a probe upon close. I mention this in the commit message, but I will also add that explanation to the refactored function.

Thanks!

20:21 < nickm> so, the next commit does the common code refactoring you mentioned before when it adds

pathbias_count_circs_in_states

20:22 < nickm> also NOTE that the pairs of arguments to pathbias_count_circs_in_states are good things to document in

the documentation for path_state_t

20:22 < athena> yeah, agree
20:22 < nickm> As for the part of this change that changes how scaling happens... how does that look to you?
20:24 < athena> it looks correct to me
20:25 < nickm> I personally would kind of prefer it if we didn't add things to {circ,use}_{attempts,successes} until we

had the corresponding changes in the other scaled variables. then this commit wouldn't be needed.
that's a potentially bigger refactoring though. NOTE it for later?

Yeah, that might be a better approach. I will put an XXX in the scaling code mentioning that we may want to have separate temporary "pending" counters in the entry_guard_t that don't get added to the attempt counts until circuit close, but I'm not sure we want to do this change at this point as it may have unexpected side effects... Or maybe we should create a separate ticket for this even... Hrmm..

Right; that's a big-ish change. I say add a ticket and an XXX, but doing it in 0.2.4 is probably not wise at this point.

comment:3 in reply to:  2 Changed 7 years ago by nickm

Replying to nickm:

Treat it like we do systems where memset doesn't make pointers null: detect it and soil ourselves. I'll write this one as designated autoconf monkey.

For this piece, please review "double-0-check" in my public repository.

comment:4 Changed 7 years ago by mikeperry

Status: newneeds_review

Ok, I pushed a series of commits that should address the code review points to mikeperry/ticket8081, head 1e5a1ae9ec1e6607832975e7290b17daf9b0f147. I haven't tested it yet. It also contains a bonus commit to fix a function name that annoyed me during my #5691 review.

The autoconf check in nickm/double-0-check looks OK to me, but it's been about a decade since I've written an autoconf test.

comment:5 Changed 7 years ago by mikeperry

Err, during my #5956 review..

comment:6 Changed 7 years ago by mikeperry

And I just noticed two additional things. Pushed two new commits on that branch, one of which is an autosquash. New head is 0e4cfc2df966ea3be4cc11c786799231ea5d151c.

comment:7 Changed 7 years ago by nickm

Merging double-0-check, the branch with a license to invoke implementation-defined behavior.

2f71cc51fa71ca5040ef2076ac010950584b81e6 -- comments only. Looks okay!

cbb2acebb17582c1d25dfe181556e2c411bf72f9 -- yup, that looks like what I had in mind.

e6141e47e932a9f13aae3a4a546ec37348b8ce35 -- all the new places look correct; assuming none were missed, this is cool.

f2e40c7b625c26be0edab4cf133f28d5cc09febc -- Looks correct.

12e1abf97af9632db9de2f4eb7bd717bcb222e6e -- Plausible. My inner hobgoblin-of-little-minds doesn't like "return;" at the end of functions.

caf6fe591c0b6d0ef9b74a29c73c7468200936e1 -- seems fine.

08396c3e7abadeee569876e4291c4c1bdd3ebe6a -- looks okay

565f6a22346b45638e78c499a6dd0ef5d50caa57 -- sure.

1e5a1ae9ec1e6607832975e7290b17daf9b0f147 -- Can we get better documentation here some time? "governs the fixed-point precision" doesn't say *how* it governs the aforementioned fixed-point precision.

834b09e205606f573401bbe576608e2dfaba1025 -- if you say so.

0e4cfc2df966ea3be4cc11c786799231ea5d151c -- yep.

I haven't tested it yet.

How much testing do you think we want to do here? Think I should merge, or wait for more testing?

comment:8 Changed 7 years ago by mikeperry

I just pushed three more commits: two for your comments (autosquashes), and one to add EntryGuardPathUseBias to the permitted keywords for the state file (oops). New head is fccbc19a6976b5b8e12a4e7f681ebb1b1ef0b1c3.

I've been testing this lightly. So far it seems to function ok, aside from that state file issue.

comment:9 Changed 7 years ago by mikeperry

Hrmm. I've been on a lousy wireless network for the past couple days, and it does seem like my use counts are lower as compared my normal reliable connection. I'm looking at 17/25 (68%) right now for the lowest, which is below the current warn threshold of 70%. Perhaps I should lower the notice threshold to 80% and the warn to 60%? Or 75 and 50?

I've also been thinking that one way we can consolidate the pathbias_measure_ functions is to make just one function that takes the product of the build bias and the use bias and determines the overall fraction of Tor network paths you're effectively able to use. This would solve your duplicate code concerns for these functions, and give us something a little easier to understand in terms of the actual effect of the overall failure rate on users' routing.

I am reluctant to do this for 0.2.4.x because I think we want to see how various users experience these thresholds independently. I also suspect that the circuit build failures will go way down as we deploy ntor and make the onionskin queue size time-based, so we'll want to observe and tune that independently first, too. Separate ticket + XXX?

comment:10 Changed 7 years ago by mikeperry

I just pushed 85bfc8ded7927d4a7dccd5269c26480b75c8b7f1 to update the comment about consolidating pathbias_measure_*, lower the thresholds, and separate the log flags so we get logs for both close and use bias.

comment:11 Changed 7 years ago by nickm

I'd prefer separate-ticket+XXXX; I'm going to squash and merge the rest. The separate log flags really wants its own commit. Generally, whenever you find yourself saying "Also," in the commit message, consider a separate commit. :)

comment:12 Changed 7 years ago by nickm

Keywords: tor-client added
Status: needs_reviewneeds_revision

Okay, I merged everything but 85bfc8ded7927d4a7dccd5269c26480b75c8b7f1. I'll leave this open till that one has a ticket. Thanks!

comment:13 Changed 7 years ago by mikeperry

Actual Points: 10
Resolution: fixed
Status: needs_revisionclosed

I've created #8159, #8160, and #8161 for the various tickets we noted we wanted to make here. #8161 is the ticket for that last commit, which is broken up and ready for review there.

comment:14 Changed 7 years ago by mikeperry

Keywords: MikePerry201301 added
Note: See TracTickets for help on using tickets.