[tor-dev] Latest state of the guard algorithm proposal (prop259) (April 2016)

[George Kadianakis]

Fan Jiang <fanjiang at thoughtworks.com> writes:

> [ text/plain ]
> On Thu, Apr 21, 2016 at 4:32 AM, George Kadianakis <desnacked at riseup.net>
> wrote:
>
>> Fan Jiang <fanjiang at thoughtworks.com> writes:
>>
>> > [ text/plain ]
>> > Hi,
>> >
>> >> Hello Fan and team,
>> >>
>> >> <snip>
>> >>
>> > Sounds great, that can simplify the logic a lot, I've done the change, no
>> > more pending_dir_guard.
>> >
>>
>> Hm. Can't you also remove pending_guard? What's the point of it now?
>>
>>
> 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?
>

Hello,

I'm a bit confused. I can't find this SAMPLED_GUARDS_STATE in the spec or code.

IIUC, we are picking guards from SAMPLED_GUARDS to be used based on their
bandwidth? If yes, I'm not sure if that's needed.

Since we already sampled by bandwidth when we originally put the guards in
SAMPLED_GUARDS, I don't think we also need to sample by bandwidth when we pick
guards from SAMPLED_GUARDS to be used.

Instead you could treat SAMPLED_GUARDS as an ordered FIFO list and always
return the top element when you are asked to sample for it. Then you wouldn't
need to keep track of 'pending_guard' anymore. That seems simpler.

Did I understand the problem correctly? Do you find any security issues with my
suggestion?

> 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.
>

Saw the commit. Cheers.

> 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.
>

No, let's not use decide_num_guards to define the number of primary guards. The
original purpose of decide_num_guards was completely different, and it does not
apply to prop259 very well.

I think it's safe to ignore decide_num_guards for now.

Thanks!