<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 21, 2016 at 4:32 AM, George Kadianakis <span dir="ltr"><<a href="mailto:desnacked@riseup.net" target="_blank">desnacked@riseup.net</a>></span> wrote:<br><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"><span class="">Fan Jiang <<a href="mailto:fanjiang@thoughtworks.com">fanjiang@thoughtworks.com</a>> writes:<br>
<br>
> [ text/plain ]<br>
</span><div><div class="h5">> Hi,<br>
><br>
>> 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<br>
>> proposal to<br>
>> the tor codebase.<br>
>><br>
>><br>
> Yeah agree, this pending_guard hack was trying to avoid some implementation<br>
> problem, we need to redesign.<br>
> I haven't got any good idea about this, that will be nice if you already<br>
> got some thoughts.<br>
><br>
><br>
>> 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<br>
>> each<br>
>> of those circuits has different requirements for guards. That is, since we<br>
>> do<br>
>> filtering on START based on the requirements of circuit #1, this means<br>
>> 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<br>
>> 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,<br>
>> consider<br>
>> how it interacts with the primary guards heuristic. It could be time for<br>
>> the<br>
>> algorithm to reenter the primary guards state and retry the top guards in<br>
>> the<br>
>> list, but because of the pending_guard thing we actually return the 15th<br>
>> 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.<br>
>> 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,<br>
>> 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<br>
>> 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<br>
>> any<br>
>>   circuit that comes its way and that should simplify logic considerably.<br>
>><br>
>><br>
> Sounds great, that can simplify the logic a lot, I've done the change, no<br>
> more pending_dir_guard.<br>
><br>
<br>
</div></div>Hm. Can't you also remove pending_guard? What's the point of it now?<br>
<br></blockquote><div> </div><div>Pending guard was actually for sampled_guard, say when we are in SAMPLED_GUARDS_STATE</div><div>we don't wan't to make the algo return a new random guard picked_by_banwith, we want to keep the same one</div><div>until the callback of first hop comes. We can make it specific for sampled_guards after checking the state. </div><div>Does this make sense?</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">
BTW, looking at your e54551adbfd5be4bee795df10f925b53fc9e730d I suggest you<br>
also use entry_is_live() with the ENTRY_NEED_UPTIME and ENTRY_NEED_CAPACITY<br>
flags always enabled. So that it always returns Stable && Fast guards.<br>
<br></blockquote><div> </div><div>Yes, I have done this change.</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">
We should also look at how ENTRY_ASSUME_REACHABLE and ENTRY_NEED_DESCRIPTOR are<br>
used in the rest of the code, to see if we should enable them or not ourselves.<br>
<span class=""><br>
>> Never saw this before, will look into it.<br>
><br>
> - 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<br>
>> function<br>
>>   is called filter_set() but it can actually end up adding guards to the<br>
>> list.<br>
>>   Maybe it can be renamed?<br>
>><br>
>><br>
> Split it up to filter_set & expand_set probably can make this clear.<br>
><br>
> - What's up with the each_remaining_by_bandwidth() function name?<br>
>><br>
>><br>
> I guess it should be iterate_remaining_guards_by_bandwidth.<br>
><br>
<br>
</span>Better. Or sample_guard_by_bandwidth() ? Or get_guard_by_bandwidth() ?<br>
<br>
IIUC that function is probalistically sampling from the 'guards' set, so it's<br>
not really iterating it.<br>
<br></blockquote><div> </div><div>We are actually pick and removing guards from remaining_set in this function, </div><div>And I saw the filter_set used this function wrong which has been fixed,<br></div><div>so maybe `pop` is better than `get`.</div><div><br></div><div>Another thing:</div><div>Do we still need decide_num_guards to define the n_primary_guards? and it's the only remaining one is using for_directory.<br></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>
Cheers!<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>