Crate cleanups #110

str4d · 2019-08-19T00:07:41Z

No description provided.

Eirik0

Everything looks great! The only thing I have not reviewed is the addition of log = "0.4".

Eirik0 · 2019-08-21T04:58:56Z

bellman/src/gadgets/boolean.rs

-            &Boolean::Constant(c) => Some(c),
-            &Boolean::Is(ref v) => v.get_value(),
-            &Boolean::Not(ref v) => v.get_value().map(|b| !b),
+        match *self {


Is there a benefit to dereferencing here? I am wondering if this is not necessary, as of Rust 1.26: https://rust-lang.github.io/rfcs/2005-match-ergonomics.html

The clippy warning that this change addressed is:

warning: you don't need to add `&` to all patterns --> bellman/src/gadgets/boolean.rs:415:9 | 415 | / match self { 416 | | &Boolean::Constant(c) => Some(c), 417 | | &Boolean::Is(ref v) => v.get_value(), 418 | | &Boolean::Not(ref v) => v.get_value().map(|b| !b), 419 | | } | |_________^ | = note: #[warn(clippy::match_ref_pats)] on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats help: instead of prefixing all patterns with `&`, you can dereference the expression | 415 | match *self { 416 | Boolean::Constant(c) => Some(c), 417 | Boolean::Is(ref v) => v.get_value(), 418 | Boolean::Not(ref v) => v.get_value().map(|b| !b), |

Dereferencing is still necessary here:

$ git diff diff --git a/bellman/src/gadgets/boolean.rs b/bellman/src/gadgets/boolean.rs index b26bb19..dafa82f 100644 --- a/bellman/src/gadgets/boolean.rs +++ b/bellman/src/gadgets/boolean.rs @@ -412,7 +412,7 @@ impl Boolean { } pub fn get_value(&self) -> Option<bool> { - match *self { + match self { Boolean::Constant(c) => Some(c), Boolean::Is(ref v) => v.get_value(), Boolean::Not(ref v) => v.get_value().map(|b| !b), $ cargo build Compiling bellman v0.1.0 (/.../librustzcash/bellman) error[E0308]: mismatched types --> bellman/src/gadgets/boolean.rs:416:42 | 416 | Boolean::Constant(c) => Some(c), | ^ | | | expected bool, found &bool | help: consider dereferencing the borrow: `*c` | = note: expected type `bool` found type `&bool` error: aborting due to previous error For more information about this error, try `rustc --explain E0308`. error: Could not compile `bellman`. To learn more, run the command again with --verbose. $ rustc --version rustc 1.36.0 (a53f9df32 2019-07-03)

Eirik0 · 2019-08-21T05:03:03Z

bellman/src/gadgets/uint32.rs

    }

-    pub fn into_bits_be(&self) -> Vec<Boolean> {
-        self.bits.iter().rev().cloned().collect()
+    pub fn into_bits_be(self) -> Vec<Boolean> {


Is there a distinction between to_bits_ and into_bits_? I am comparing this to the name change in bellman/src/gadgets/num.rs.

The clippy warning that this change addresses is:

warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name --> bellman/src/gadgets/uint32.rs:78:25 | 78 | pub fn into_bits_be(&self) -> Vec<Boolean> { | ^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention

This follows the corresponding entry in the Rust style guide.

I altered the various methods to be consistent with how they used self in context. I first tried having the into_* methods consume self, and where that was not possible due to the caller relying on passing an immutable reference, I altered the method name to to_*.

Eirik0 · 2019-08-21T05:08:07Z

bellman/src/multiexp.rs

@@ -153,7 +153,7 @@ fn multiexp_inner<Q, D, G, S>(
    mut skip: u32,
    c: u32,
    handle_trivial: bool,
-) -> Box<Future<Item = <G as CurveAffine>::Projective, Error = SynthesisError>>
+) -> Box<dyn Future<Item = <G as CurveAffine>::Projective, Error = SynthesisError>>


This change is also made when running cargo fix --edition-idioms.

Eirik0 · 2019-08-21T05:35:53Z

zcash_primitives/src/jubjub/fs.rs

@@ -321,8 +321,8 @@ impl Field for Fs {
        loop {
            let mut tmp = {


This could be replaced with

// Mask away the unused most-significant bits. let tmp = Fs(FsRepr([ rng.next_u64(), rng.next_u64(), rng.next_u64(), rng.next_u64() & (0xffffffffffffffff >> REPR_SHAVE_BITS), ]));

And then we wouldn't need the loop below, but that may be beyond the scope of this PR.

str4d · 2019-08-21T09:13:23Z

I added log to the dependecy tree in order to provide distinct branches inside equihash::validate_subtrees, otherwise clippy complained that the three branches of the Equihash conditional were identical. We could alternatively (or additionally) set up an Error type instead of using bool. I think log is an independently beneficial crate to depend on, particularly if we want to move more zcashd logic into Rust - we should be able to log from the Rust side into zcashd's debug.log. (Implementing the connecting logic in librustzcash would require a separate PR.)

ebfull · 2019-08-23T23:26:40Z

Looking good, will re-review when #113 is merged since it will conflict with this I think.

Memo::from_str was previously shadowing a built-in trait method.

str4d · 2019-08-23T23:42:56Z

Rebased on master to fix merge conflicts with #113.

Eirik0

utACK. Rebase looks good.

defuse

utACK. I left some non-blocking comments.

defuse · 2019-09-04T22:53:58Z

zcash_primitives/src/jubjub/montgomery.rs

@@ -214,7 +213,7 @@ impl<E: JubjubEngine, Subgroup> Point<E, Subgroup> {

        let mut delta = E::Fr::one();
        {
-            let mut tmp = params.montgomery_a().clone();
+            let mut tmp = *params.montgomery_a();


Rust noob question: Is it correct that I can be sure this isn't modifying any part of params because it isn't &mut in the function signature? (I've been making this assumption in a couple cases above where an explicit .clone() was removed, too.).

JubjubParams::montgomery_a takes &self, so you are guarateed that params is not modified.

The reason for removing the clone() is that JubjubParams::montgomery_a returns JubjubEngine::Fr (which resolves to ff::ScalarEngine::Fr), which via ff::Field is required to implement Copy, and thus makes the explicit clone unnecessary.

defuse · 2019-09-04T23:18:00Z

bellman/src/domain.rs

@@ -337,12 +341,12 @@ fn parallel_fft<E: ScalarEngine, T: Group<E>>(
                let omega_step = omega.pow(&[(j as u64) << log_new_n]);

                let mut elt = E::Fr::one();
-                for i in 0..(1 << log_new_n) {
+                for (i, tmp) in tmp.iter_mut().enumerate() {


Previously this loop's body would execute 1 << log_new_n times. With this change it should execute num_cpus times according to the length of tmp when it's created above, right?. Won't that change the behavior?

Oh, I see tmp, is shadowed by the outermost loop so here it will have length 1 << log_new_n and the behavior is unchanged. It might be worth renaming some variables to avoid similar confusion.

str4d requested review from dconnolly, ebfull and Eirik0 August 19, 2019 00:07

str4d force-pushed the crate-cleanups branch from 99557e3 to a0a9b74 Compare August 19, 2019 00:14

str4d removed the request for review from Eirik0 August 20, 2019 17:27

Eirik0 reviewed Aug 21, 2019

View reviewed changes

Eirik0 approved these changes Aug 21, 2019

View reviewed changes

str4d added this to the v0.1.0 milestone Aug 22, 2019

str4d added 8 commits August 24, 2019 00:37

Move Equihash validator into zcash_primitives

d78c94b

Address various clippy warnings/errors in bellman

d65fe2c

Address various clippy warnings/errors in zcash_primitives

3a8efd9

Address various clippy warnings/errors in zcash_proofs

9154167

Rename into_ -> to_ where &self is used.

fe93f2f

Take self directly in into_* functions

9016548

Log distinct error cases in Equihash verification

7c1d4d9

impl FromStr for Memo

0c7eb84

Memo::from_str was previously shadowing a built-in trait method.

str4d force-pushed the crate-cleanups branch from a0a9b74 to 0c7eb84 Compare August 23, 2019 23:42

Eirik0 approved these changes Aug 23, 2019

View reviewed changes

defuse approved these changes Sep 4, 2019

View reviewed changes

str4d merged commit b19b40c into zcash:master Sep 4, 2019

str4d deleted the crate-cleanups branch September 4, 2019 23:44

str4d mentioned this pull request Sep 4, 2019

PaymentAddress encapsulation #109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crate cleanups #110

Crate cleanups #110

str4d commented Aug 19, 2019

Eirik0 left a comment

Eirik0 Aug 21, 2019

str4d Aug 21, 2019

Eirik0 Aug 21, 2019

str4d Aug 21, 2019

Eirik0 Aug 21, 2019

Eirik0 Aug 21, 2019

str4d commented Aug 21, 2019

ebfull commented Aug 23, 2019

str4d commented Aug 23, 2019

Eirik0 left a comment

defuse left a comment

defuse Sep 4, 2019

str4d Sep 4, 2019

defuse Sep 4, 2019

		@@ -321,8 +321,8 @@ impl Field for Fs {
		loop {
		let mut tmp = {

Crate cleanups #110

Crate cleanups #110

Conversation

str4d commented Aug 19, 2019

Eirik0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Aug 21, 2019

ebfull commented Aug 23, 2019

str4d commented Aug 23, 2019

Eirik0 left a comment

Choose a reason for hiding this comment

defuse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment