[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #19877 [Core Tor/Tor]: Implement new guard selection algorithm of prop 271
#19877: Implement new guard selection algorithm of prop 271
-------------------------------------------------+-------------------------
Reporter: andrea | Owner: nickm
Type: task | Status:
| assigned
Priority: Medium | Milestone: Tor:
| 0.3.0.x-final
Component: Core Tor/Tor | Version: Tor:
| unspecified
Severity: Normal | Resolution:
Keywords: isaremoved, nickwants029, tor- | Actual Points:
guards-revamp, TorCoreTeam201611 |
Parent ID: | Points:
Reviewer: | Sponsor:
| SponsorU-must
-------------------------------------------------+-------------------------
Comment (by nickm):
Replying to [comment:9 asn]:
> Hello, I did a very rough initial review based on the WIP branch
`prop271-wip`. Haven't run it yet. Great work so far, very excited about
splitting bridge code to another file as well.
>
> Some of these comments might be too low level for this stage of
implementation, so feel free to ignore them. Also, I have not re-loaded
the whole proposal in my brain yet, so I didn't go too deep in the nitty
gritty details.
>
> - There are a few big scary functions that could be splitted into
smaller functions to make them more manageable. e.g.
`sampled_guards_update_from_consensus()` and
`entry_guards_update_primary`.
>
> Also the fundamental function `select_entry_guard_for_circuit()` could
be split into three smaller functions, each for one spec case.
Ack. I'll add a "XXXX can this split up" comment to those.
> - Agreed that the `entry_guard_succeeded()` tristate is ugly and makes
the `circuit_send_next_onion_skin()` logic harder to read. Perhaps the
retval could be turned into an enum?
Ack, will fix.
> - If `add_bridge_as_entry_guard()` is a pub function of the entrynodes
API, should it have a prefix? Or we are not at that level of tidyness yet?
The entire bridge->entry interface is in flux; I hope we clean it up by
the end.
> - Why do we need the spaceless ISO time functions? Can't we use the
spaceful ones for state? Or is it to maintain backwards compat?
For the state format I chose, everything is spaceless. That makes it much
much easier.
> - Parsing guards from state seems a bit of a pain now. For example,
`entry_guard_parse_from_state()` does ad-hoc string parsing. Would it be
possible to use the config parsing API for that?
Oooh. Interesting. We'd have to make it recursive, so it can handle the
values within a line rather than just handling lines. Could be neat
though. Maybe as another ticket?
> - Could `smartlist_remove_keeporder()` be implemented with
`smartlist_pos()` and `smartlist_del_keeporder()` to avoid writing more
smartlist code?
Only in quadratic-time: smartlist_remove_keeporder() needs to remove every
occurrence of the target element. So to emulate it with pos and
del_keeporder, you'd need to say:
{{{
int idx;
while ( (idx = smartlist_pos(sl, element)) >= 0) {
smartlist_del_keeporder(sl, idx);
}
}}}
> Finally, how should one robustly test this branch? Do you use iptable
scripts or something?
I've been doing ad-hoc iptable stuff, doing some of my hacking from nasty
coffeeshop ISPs, etc. I haven't figured out how to do a really thorough
hostile-network integration test though.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19877#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs