<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Hi, </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">
</div></div>Hello Fan and team,<br>
<br>
I think I'm not a big fan of the pending_guard and pending_dir_guard<br>
concept. To me it seems like a quick hack that tries to address fundamental<br>
issues with our algorithm that appeared when we tried to adapt the proposal to<br>
the tor codebase.<br>
<br></blockquote><div> </div><div>Yeah agree, this pending_guard hack was trying to avoid some implementation problem, we need to redesign.</div><div>I haven't got any good idea about this, that will be nice if you already got some thoughts.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I think one of the main issues with the current algorithm structure is that<br>
_one run of the algorithm_ can be asked to _setup multiple circuits_, and each<br>
of those circuits has different requirements for guards. That is, since we do<br>
filtering on START based on the requirements of circuit #1, this means that any<br>
other circuits that appear before END is called, also have to adapt to the<br>
requirements of circuit #1. This is obvious in the code since we use<br>
guard_selection->for_directory throughout the whole algorithm run, even though<br>
for_directory was just the restriction of circuit #1.<br>
<br>
Specifically about the pending_guard trick, I feel that it interacts in<br>
unpredictable ways with other features of the algorithm. For example, consider<br>
how it interacts with the primary guards heuristic. It could be time for the<br>
algorithm to reenter the primary guards state and retry the top guards in the<br>
list, but because of the pending_guard thing we actually return the 15th guard<br>
to the circuit.<br>
<br>
IMO we should revisit the algorithm so that one run of the algorithm can<br>
accomodate multiple circuits by design and without the need for hacks. Here is<br>
an idea towards that direction:<br>
<br>
I think one very important change that we can do to simplify things is to<br>
remove the need to filter guards based on whether they are dirguards, fast,<br>
or stable. My suggestion here would be to *only* consider guards that are<br>
dirguards _and_ fast _and_ stable. This way, any circuit that appears will be<br>
happy with all the guards in our list and there is no need to do the<br>
pending_dir_guard trick. See [0] on why I think that's safe to do.<br>
<br>
This is easy to do in the current codebase. You just need to call<br>
entry_is_live() with need_uptime, need_capacity and for_directory all<br>
enabled (instead of flags being 0).<br>
<br>
If you do the above, your sampled guard set will be able to accomodate any<br>
circuit that comes its way and that should simplify logic considerably.<br>
<br></blockquote><div> </div><div>Sounds great, that can simplify the logic a lot, I've done the change, no more pending_dir_guard.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Let me know if the above does not make sense.<br>
<br>
Here are some more comments:<br>
<br>
- So the above idea addresses a large part of the filtering logic that happens<br>
on START. The rest of the filtering logic has to do with ClientUsesIPv6,<br>
ReachableAddreses, etc. . I think it's fine to conduct that filtering on<br>
START as well.<br>
<br>
- I tried to run the branch as of bb3237d, but it segfaulted. Here is where it crashed:<br>
<br>
#1 0x000055555567eb25 in guards_update_state (next=0x5555559c3f40, next@entry=0x5555559c35e8, guards=guards@entry=0x5555559c4620,<br>
config_name=config_name@entry=0x55555570c47e "UsedGuard") at src/or/prop259.c:1137<br>
1137 !strchr(e->chosen_by_version, ' ')) {<br>
<br>
Let me know if you need more info here.<br>
<br></blockquote><div>Never saw this before, will look into it.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- There is a memleak on 'extended' in filter_set().<br>
<br>
In general, I feel that logic in that function is a bit weird. The function<br>
is called filter_set() but it can actually end up adding guards to the list.<br>
Maybe it can be renamed?<br>
<br></blockquote><div> </div><div>Split it up to filter_set & expand_set probably can make this clear. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- What's up with the each_remaining_by_bandwidth() function name?<br>
<br></blockquote><div> </div><div>I guess it should be iterate_remaining_guards_by_bandwidth.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
---<br>
<br>
<br>
[0]: I think that's OK to do and here is why:<br>
<br>
All Guards are Fast.<br>
About 95% of Guards are Stable (this will become 100% with #18624)<br>
About 80% of Guards are V2Dir/dirguards (this will become 100% with #12538)<br>
<br>
#12538 got merged in 0.2.8, so if prop259 gets merged in 0.2.9, by the<br>
time prop259 becomes active, almost all guards will be dirguards.<br>
<br>
So I think it's fine to only consider guards that are dirguards && fast &&<br>
stable now, since by the time prop259 gets deployed that will be the case<br>
for almost 100% of guards.<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">____<div><br><div><img src="http://www.thoughtworks.com/imgs/favicons/favicon.ico"><br></div><div><span style="color:rgb(136,136,136)">Fan Jiang 蒋帆</span></div><div>Amateur Code Chef<br style="color:rgb(136,136,136)"><span style="color:rgb(136,136,136)">Thoughtworks, Inc.</span><br style="color:rgb(136,136,136)"><span style="color:rgb(136,136,136)">mobile +86-150-9189-3714</span><br></div><div><span style="color:rgb(136,136,136)">skype <a href="mailto:fan@torchz.net" target="_blank">fan@torchz.net</a></span></div><div><span style="color:rgb(136,136,136)"><br></span></div><div><br></div></div></div></div></div></div></div></div>
</div></div>