Opened 17 months ago

Last modified 5 months ago

#25381 needs_revision enhancement

Add crypto_rand_double_sign() in C and Rust

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, privcount, review-group-34, 034-triage-20180328, 034-removed-20180328, rust
Cc: catalyst, chelseakomlo Actual Points: 1
Parent ID: #26637 Points: 2
Reviewer: isis Sponsor: SponsorV-can

Description

We need a function that returns 1.0 or -1.0 with equal probability, so we can avoid weird tricks that waste floating point precision.

Since we want to use this in the laplace and guassian distributions, it needs to be implemented in both C and Rust.

Child Tickets

Change History (19)

comment:1 Changed 17 months ago by teor

See my branch rust-rand-f64-v2 on https://github.com/teor2345/tor.git

I need help getting the module doc tests to run.
It seems like trivial changes can activate or deactivate doc tests.

I wanted to put crypto_rand_f64/crypto_rand_distribution.rs in test/distribution/mod.rs, but I couldn't get its doc tests to run.

Once I've sorted out these issues, I'll be ready to put the branch on oniongit for a real review.

comment:2 Changed 17 months ago by teor

Status: assignedneeds_revision

I was using an old version of rustc, the new version handles the doctests correctly, and they fail.
So I have some bugs to fix,

comment:3 Changed 17 months ago by catalyst

Cc: catalyst added

comment:4 Changed 17 months ago by teor

Actual Points: 1
Status: needs_revisionneeds_review

Please see my draft branch rust-rand-f64-v4 on https://github.com/teor2345/tor.git

It's not ready to be merged yet, but it has basic structure, tests, and some functionality.

Here are the remaining tasks:

  • write some floating-point error documentation
  • clean up the module imports: the imports I'm doing seem rather clunky
  • tidy up some of the comments and missing references

comment:5 Changed 17 months ago by isis

Keywords: review-group-34 added
Reviewer: isis

comment:6 Changed 17 months ago by teor

Isis also tells me to make rand a hard dependency.

I need to:

  • commit Cargo.lock and Cargo.toml
  • cargo vendor the dependency to tor-rust-dependencies (we should have a script for this, maybe it's in the instructions)

comment:7 Changed 17 months ago by dgoulet

Status: needs_reviewneeds_revision

comment:8 Changed 17 months ago by isis

Status: needs_revisionneeds_review

Reviewing the prototype without those fixes seems fine, they won't impact my understanding of the code any.

comment:9 Changed 17 months ago by isis

Status: needs_reviewneeds_revision

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:
    pub fn bar() -> bool {
        true
    }
    
    #[cfg(test)]
    mod test {
        use super::*;
    
        // [...]
    }
    
  1. I fixed the pub use issues with your doctests and the dead code warnings in 4ee8a4e8626 (feel free to use that commit or not… it was just easier for me to show how than explain). (In my bug25381_pub_use branch.)
  1. 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.
    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.

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.

  1. (Trivial style nitpick) Is your rustfmt telling you to put function arguments on separate lines like this?
    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)

  1. 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.
  1. 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.
  1. 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.
  1. 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
    use crypto_rand_f64::crypto_rand_distribution::*;
    

and instead we could do, e.g.

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!

Last edited 17 months ago by isis (previous) (diff)

comment:10 Changed 16 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 16 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:12 Changed 16 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

comment:13 Changed 13 months ago by teor

Parent ID: #23061#26637

comment:14 Changed 12 months ago by chelseakomlo

Cc: chelseakomlo added
Keywords: rust added

comment:15 Changed 12 months ago by nickm

Sponsor: SponsorQSponsorV-can

comment:16 Changed 12 months ago by cypherpunks3

It looks like branch rust-rand-f64-v4 was deleted, and there's no other branches starting with rust-* in the repo https://github.com/teor2345/tor.git

Is it still needs_revision if there's nothing left to revise or pick up where it left off?

comment:17 in reply to:  16 Changed 12 months ago by teor

Replying to cypherpunks3:

It looks like branch rust-rand-f64-v4 was deleted, and there's no other branches starting with rust-* in the repo https://github.com/teor2345/tor.git

Is it still needs_revision if there's nothing left to revise or pick up where it left off?

I archived my old tor repository, so I could fork https://github.com/torproject/tor.git
(GitHub doesn't support pull requests unless the repositories share a common fork ancestor.)

The branch is still at:
https://github.com/teor2345/tor-old/tree/rust-rand-f64-v4

comment:18 Changed 5 months ago by teor

Owner: teor deleted
Status: needs_revisionassigned

comment:19 Changed 5 months ago by teor

Status: assignedneeds_revision
Note: See TracTickets for help on using tickets.