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?