Fan Jiang fanjiang@thoughtworks.com writes:
[ text/plain ] On Thu, Apr 21, 2016 at 4:32 AM, George Kadianakis desnacked@riseup.net wrote:
Fan Jiang fanjiang@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!