<p dir="ltr"><br>
2016年4月22日 上午4:54，"George Kadianakis" <<a href="mailto:desnacked@riseup.net">desnacked@riseup.net</a>>写道：<br>
><br>
> Fan Jiang <<a href="mailto:fanjiang@thoughtworks.com">fanjiang@thoughtworks.com</a>> writes:<br>
><br>
> > [ text/plain ]<br>
> > On Thu, Apr 21, 2016 at 4:32 AM, George Kadianakis <<a href="mailto:desnacked@riseup.net">desnacked@riseup.net</a>><br>
> > wrote:<br>
> ><br>
> >> Fan Jiang <<a href="mailto:fanjiang@thoughtworks.com">fanjiang@thoughtworks.com</a>> writes:<br>
> >><br>
> >> > [ text/plain ]<br>
> >> > Hi,<br>
> >> ><br>
> >> >> Hello Fan and team,<br>
> >> >><br>
> >> >> <snip><br>
> >> >><br>
> >> > Sounds great, that can simplify the logic a lot, I've done the change, no<br>
> >> > more pending_dir_guard.<br>
> >> ><br>
> >><br>
> >> Hm. Can't you also remove pending_guard? What's the point of it now?<br>
> >><br>
> >><br>
> > Pending guard was actually for sampled_guard, say when we are in<br>
> > SAMPLED_GUARDS_STATE<br>
> > we don't wan't to make the algo return a new random guard<br>
> > picked_by_banwith, we want to keep the same one<br>
> > until the callback of first hop comes. We can make it specific for<br>
> > sampled_guards after checking the state.<br>
> > Does this make sense?<br>
> ><br>
><br>
> Hello,<br>
><br>
> I'm a bit confused. I can't find this SAMPLED_GUARDS_STATE in the spec or code.</p>
<p dir="ltr">Ah, Sorry it's STATE_TRY_REMAINING<br>
In proposal spec 2.2.2 the second paragraph.<br>
Return each entry from REMAINING_GUARDS using NEXT_BY_BANDWIDTH.</p>
<p dir="ltr">> IIUC, we are picking guards from SAMPLED_GUARDS to be used based on their<br>
> bandwidth? If yes, I'm not sure if that's needed.</p>
<p dir="ltr">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.<br>
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.</p>
<p dir="ltr">> Since we already sampled by bandwidth when we originally put the guards in<br>
> SAMPLED_GUARDS, I don't think we also need to sample by bandwidth when we pick<br>
> guards from SAMPLED_GUARDS to be used.<br>
><br>
> Instead you could treat SAMPLED_GUARDS as an ordered FIFO list and always<br>
> return the top element when you are asked to sample for it. Then you wouldn't<br>
> need to keep track of 'pending_guard' anymore. That seems simpler.<br>
><br>
> Did I understand the problem correctly? Do you find any security issues with my<br>
> suggestion?<br>
><br>
> > BTW, looking at your e54551adbfd5be4bee795df10f925b53fc9e730d I suggest you<br>
> >> also use entry_is_live() with the ENTRY_NEED_UPTIME and ENTRY_NEED_CAPACITY<br>
> >> flags always enabled. So that it always returns Stable && Fast guards.<br>
> >><br>
> >><br>
> > Yes, I have done this change.<br>
> ><br>
><br>
> Saw the commit. Cheers.<br>
><br>
> > We should also look at how ENTRY_ASSUME_REACHABLE and ENTRY_NEED_DESCRIPTOR<br>
> >> are<br>
> >> used in the rest of the code, to see if we should enable them or not<br>
> >> ourselves.<br>
> >><br>
> >> >> Never saw this before, will look into it.<br>
> >> ><br>
> >> > - There is a memleak on 'extended' in filter_set().<br>
> >> >><br>
> >> >>   In general, I feel that logic in that function is a bit weird. The<br>
> >> >> function<br>
> >> >>   is called filter_set() but it can actually end up adding guards to the<br>
> >> >> list.<br>
> >> >>   Maybe it can be renamed?<br>
> >> >><br>
> >> >><br>
> >> > Split it up to filter_set & expand_set probably can make this clear.<br>
> >> ><br>
> >> > - What's up with the each_remaining_by_bandwidth() function name?<br>
> >> >><br>
> >> >><br>
> >> > I guess it should be iterate_remaining_guards_by_bandwidth.<br>
> >> ><br>
> >><br>
> >> Better. Or sample_guard_by_bandwidth() ? Or get_guard_by_bandwidth() ?<br>
> >><br>
> >> IIUC that function is probalistically sampling from the 'guards' set, so<br>
> >> it's<br>
> >> not really iterating it.<br>
> >><br>
> >><br>
> > We are actually pick and removing guards from remaining_set in this<br>
> > function,<br>
> > And I saw the filter_set used this function wrong which has been fixed,<br>
> > so maybe `pop` is better than `get`.<br>
> ><br>
> > Another thing:<br>
> > Do we still need decide_num_guards to define the n_primary_guards? and it's<br>
> > the only remaining one is using for_directory.<br>
> ><br>
><br>
> No, let's not use decide_num_guards to define the number of primary guards. The<br>
> original purpose of decide_num_guards was completely different, and it does not<br>
> apply to prop259 very well.</p>
<p dir="ltr">cool, will just comment out this.</p>
<p dir="ltr">> I think it's safe to ignore decide_num_guards for now.<br>
><br>
> Thanks!</p>
<p dir="ltr">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?</p>
<p dir="ltr">BTW, About your segmentfault I couldn't reproduce, maybe related with your torrc/state file?</p>