On Thu, Apr 21, 2016 at 4:32 AM, George Kadianakis <desnacked@xxxxxxxxxx> wrote:
Fan Jiang <fanjiang@xxxxxxxxxxxxxxxx> writes:
> [ text/plain ]
Hm. Can't you also remove pending_guard? What's the point of it now?> Hi,
>
>> Hello Fan and team,
>>
>> I think I'm not a big fan of the pending_guard and pending_dir_guard
>> concept. To me it seems like a quick hack that tries to address fundamental
>> issues with our algorithm that appeared when we tried to adapt the
>> proposal to
>> the tor codebase.
>>
>>
> Yeah agree, this pending_guard hack was trying to avoid some implementation
> problem, we need to redesign.
> I haven't got any good idea about this, that will be nice if you already
> got some thoughts.
>
>
>> I think one of the main issues with the current algorithm structure is that
>> _one run of the algorithm_ can be asked to _setup multiple circuits_, and
>> each
>> of those circuits has different requirements for guards. That is, since we
>> do
>> filtering on START based on the requirements of circuit #1, this means
>> that any
>> other circuits that appear before END is called, also have to adapt to the
>> requirements of circuit #1. This is obvious in the code since we use
>> guard_selection->for_directory throughout the whole algorithm run, even
>> though
>> for_directory was just the restriction of circuit #1.
>>
>> Specifically about the pending_guard trick, I feel that it interacts in
>> unpredictable ways with other features of the algorithm. For example,
>> consider
>> how it interacts with the primary guards heuristic. It could be time for
>> the
>> algorithm to reenter the primary guards state and retry the top guards in
>> the
>> list, but because of the pending_guard thing we actually return the 15th
>> guard
>> to the circuit.
>>
>> IMO we should revisit the algorithm so that one run of the algorithm can
>> accomodate multiple circuits by design and without the need for hacks.
>> Here is
>> an idea towards that direction:
>>
>>Â ÂI think one very important change that we can do to simplify things is to
>>Â Âremove the need to filter guards based on whether they are dirguards,
>> fast,
>>Â Âor stable. My suggestion here would be to *only* consider guards that are
>>Â Âdirguards _and_ fast _and_ stable. This way, any circuit that appears
>> will be
>>Â Âhappy with all the guards in our list and there is no need to do the
>>Â Âpending_dir_guard trick. See [0] on why I think that's safe to do.
>>
>>Â ÂThis is easy to do in the current codebase. You just need to call
>>Â Âentry_is_live() with need_uptime, need_capacity and for_directory all
>>Â Âenabled (instead of flags being 0).
>>
>>Â ÂIf you do the above, your sampled guard set will be able to accomodate
>> any
>>Â Âcircuit that comes its way and that should simplify logic considerably.
>>
>>
> Sounds great, that can simplify the logic a lot, I've done the change, no
> more pending_dir_guard.
>
Â
Pending guard was actually for sampled_guard, say when we are in SAMPLED_GUARDS_STATE
we don't wan't to make the algo return a new random guard picked_by_banwith, we want to keep the same one
until the callback of first hop comes. We can make it specific for sampled_guards after checking the state.Â
Does this make sense?
BTW, looking at your e54551adbfd5be4bee795df10f925b53fc9e730d I suggest you
also use entry_is_live() with the ENTRY_NEED_UPTIME and ENTRY_NEED_CAPACITY
flags always enabled. So that it always returns Stable && Fast guards.
Â
Yes, I have done this change.
We should also look at how ENTRY_ASSUME_REACHABLE and ENTRY_NEED_DESCRIPTOR are
used in the rest of the code, to see if we should enable them or not ourselves.
>> Never saw this before, will look into it.
>
> - There is a memleak on 'extended' in filter_set().
>>
>>Â ÂIn general, I feel that logic in that function is a bit weird. The
>> function
>>Â Âis called filter_set() but it can actually end up adding guards to the
>> list.
>>Â ÂMaybe it can be renamed?
>>
>>
> Split it up to filter_set & expand_set probably can make this clear.
>
> - What's up with the each_remaining_by_bandwidth() function name?
>>
>>
> I guess it should be iterate_remaining_guards_by_bandwidth.
>
Better. Or sample_guard_by_bandwidth() ? Or get_guard_by_bandwidth() ?
IIUC that function is probalistically sampling from the 'guards' set, so it's
not really iterating it.
Â
We are actually pick and removing guards from remaining_set in this function,Â
And I saw theÂfilter_set used this function wrong which has been fixed,
so maybe `pop` is better than `get`.
Another thing:
Do we still need decide_num_guards to define the n_primary_guards? and it's the only remaining one is using for_directory.
Cheers!
____