[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #32088 [Core Tor/Tor]: Proposal 310 - choose guards in sampled order



#32088: Proposal 310 - choose guards in sampled order
--------------------------------------+------------------------------------
 Reporter:  Jaym                      |          Owner:  (none)
     Type:  enhancement               |         Status:  needs_review
 Priority:  High                      |      Milestone:  Tor: 0.4.4.x-final
Component:  Core Tor/Tor              |        Version:
 Severity:  Normal                    |     Resolution:
 Keywords:  tor-spec prop271 prop310  |  Actual Points:
Parent ID:                            |         Points:
 Reviewer:  nickm, asn                |        Sponsor:
--------------------------------------+------------------------------------

Comment (by asn):

 Hey thanks for the super fast revisions here! I left a comment on the new
 PR.

 Furthermore, I'd be more confidence if we had some more testing here. The
 test coverage of the branch in terms of LoC is great, but I would also
 like a test that tests the entire logic of this new feature. In
 particular, given that the changes are far from trivial, and change
 various behaviors in the code, I would like a test that tests most of
 that. In particular, I would be looking for something like: Load some
 guards to the sampled set (without using a state file), make sure that the
 sampled set has the sampled idxs correctly set, then create the confirmed
 set and make sure that's well ordered too, then make the primary guards
 list and make sure that's well ordered, finally ask for a guard and make
 sure that's what you would expect. Feel free to reuse code from other
 unittests of course...

 So, before this branch gets merged we would need to do the following two
 things:
 - Get that unittest in.
 - Answer the comment on the new PR.
 - Get the torspec patch for `guard-spec.txt` merged.
 - Squash the PR into more organized commits. Ideally this PR would be
 squashed down to max 3-4 logical commits. Something like the following
 commit structure would work, but feel free to improvise:
   - Commit 1: Use helper functions
 parse_from_state_set_vals/parse_from_state_handle_time
   - Commit 2: Implement prop310"
   - Commit 3: Fix existing unittests
   - Commit 4: Add unittests

 Thanks a lot for your work! :)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32088#comment:26>
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