Re: [tor-bugs] #25381 [Core Tor/Tor]: Add crypto_rand_double_sign() in C and Rust

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#25381: Add crypto_rand_double_sign() in C and Rust
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  teor
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-relay, privcount, review-        |  Actual Points:  1
  group-34                                       |
Parent ID:  #23061                               |         Points:  2
 Reviewer:  isis                                 |        Sponsor:
                                                 |  SponsorQ
-------------------------------------------------+-------------------------
Changes (by isis):

 * status:  needs_review => needs_revision


Comment:

 Hi! This looks very good so far!

 Random notes:

 1. It's a little hard for me to follow where stuff is coming from, due to
 the use of `use something::something::*` in several places. IIRC, Rust has
 considered issuing a warning on using `*` imports, and stylistically it's
 preferred to be explicit about where code is coming from (it also makes it
 easier to switch to using something with a colliding name from a different
 namespace later, if we should ever need to). The one case where it's
 generally considered acceptable is in testing to import the parent module,
 e.g. in `foo.rs`:
     {{{#!rust
     pub fn bar() -> bool {
         true
     }

     #[cfg(test)]
     mod test {
         use super::*;

         // [...]
     }
     }}}

 2. I fixed the `pub use` issues with your doctests and the dead code
 warnings in `` (feel free to use that commit or not… it was just easier
 for me to show how than explain).

 3. You're passing things around in a way that takes ownership in a lot of
 cases where you don't seem to need to, e.g.
     {{{#!rust
     pub fn max_f64(a: f64, b: f64) -> f64
     }}}
     I had this habit too, at first, especially for integer types where the
 pointer to it would be larger than the type. (C has ingrained some bad
 habits, imho.) However, it'll very likely make your code ''faster'' if you
 instead pass by borrows whenever you can (especially in the glorious
 future when we have more multithreaded stuff!), e.g.
     {{{#!rust
     pub fn max_f64<'a>(a: &'a f64, b: &'a f64) -> &'a f64
     }}}
     (You might be able to leave the explicit lifetime off in that case
 with the current Rust, I'm not quite sure.) Also, even if it doesn't make
 the code faster, it does make it automatic for me to determine when
 considering calling one of your functions if the function is going to
 mutate my value. If it's an `&T`, I know right away that it won't.

 4. (Trivial style nitpick) Is your rustfmt telling you to put function
 arguments on separate lines like this?
     {{{#!rust
     pub fn get_binomial_standard_deviation(
         probability: f64,
         range: f64,
         trials: u64,
     ) -> f64 {
     }}}
     Mine tries to correct it to be all the same line for some reason,
 possibly we've got different versions or something? Anyway, this drives me
 nuts (also our C function impl style drives me nuts too, so idk, maybe
 I'll just be driven nuts by the whitespace whenever I look at any of our
 code, lol)

 5. I'm slightly confused about the tests in `crypto_rand_f64/tests/`? They
 don't appear to be integration tests (e.g. testing behaviour combined with
 our other crates, or otherwise external code), so they could possibly just
 be unittests in a `mod test` in one of the modules.

 6. I'm extremely confused about the tests in `limits_f64.rs`? Like why
 they are just hanging out, not in their own `mod test` or anything? I'm
 surprised this compiles, and worried that rustc is treating those as
 integration tests, which should mean that they are run whenever anything
 from that module is called? I have no idea how this is even working.

 7. Possibly some functions which are `pub` could be `pub(crate)`? Not
 entirely sure, so you'd probably know better which things you expect to be
 used externally.

 8. Possibly a TODO for the future: In some of my code for #24659, I
 started by making a `src/rust/crypto/` directory, with further (eventual)
 subdirectories for digests, randomnesses, ciphers, signatures, etc. We
 might want to find a nice namespace for your code to live in, so that
 we're not doing the very C-like thing of writing stuff like
     {{{#!rust
     use crypto_rand_f64::crypto_rand_distribution::*;
     }}}
     and instead we could do, e.g.
     {{{#!rust
     use tor::crypto::random::f64::distribution::*;
     }}}
     or something. (Also I think we should be claiming `tor::` as a top-
 level namespace, imho.)

 Other than those nitpicky things, your Rust code is extremely good. I
 really appreciate the efforts you've put into documentation and testing,
 especially also the inline comments explaining your assumptions and why
 things shouldn't/can't fail, etc., this made it ''way'' easier to quickly
 understand what the code was doing and what it was expected to do. :)
 Great work so far!

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