2016年4月22日 上午4:54，"George Kadianakis" desnacked@riseup.net写道：
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.
Ah, Sorry it's STATE_TRY_REMAINING In proposal spec 2.2.2 the second paragraph. Return each entry from REMAINING_GUARDS using NEXT_BY_BANDWIDTH.
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.
This was mainly for shuffling the guards again before return. And actually I agree with you that sampled_guards has already been shuffled while filling. FIFO would make things simpler. About Security I don't think it's much different to do random pick on 40 or fewer guards, Will discuss with the team for more inputs or concerns.
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.
cool, will just comment out this.
I think it's safe to ignore decide_num_guards for now.
Thanks!
It seems like we come to a point that most of prop259 can be stable for a while, we are going to do some cleanup in this implementation and spec. I think next week we can ask more people to review, does that sounds OK?
BTW, About your segmentfault I couldn't reproduce, maybe related with your torrc/state file?