Equihash validator

[str4d]

No description provided.

[ebfull]

You need to leak st (std::mem::forget) or the Vec will deallocate and this function will always return a dangling pointer.

If I were you, I would encapsulate all of this so that we can be sure there are no memory safety problems. Create a structure containing *mut crypto_generichash_blake2b_state. It'll be a transmuted pointer from a Box::into_raw, which you can get with Vec::into_boxed_slice if you want to keep crypto_generichash_statebytes dynamic. You can Box::from_raw to get the box back in your Drop impl.

Also, use std::mem::align_of to test that u8 and crypto_generichash_blake2b_state have the same alignment.

[str4d]

Replaced with blake2b crate.

[ebfull]

Perhaps use byteorder crate to break n and k up instead.

[str4d]

IIRC the reason I did it this way is because n and k are different types to bare chars, and I couldn't get the type checker to be satisfied. But this was my first Rust code, so I should definitely try that 😃

[ebfull]

self.hash.iter().take(len).all(|v| v == 0)

[ebfull]

It looks like these lifetimes can be elided.

[ebfull]

let hash: Vec<_> = a.hash.iter()
                         .zip(b.hash.iter())
                         .skip(trim)
                         .map(|(a, b)| a ^ b)
                         .collect();

or something like that would be more idomatic.

[ebfull]

a.hash.iter().zip(b.hash.iter()).take(len).all(|(a, b)| a == b)

[ebfull]

#![derive(Clone)] for Node instead of manually doing it.

[ebfull]

collapse these uses together like

use libc::{
    uint32_t,
    uint64_t,
    c_uchar,
    size_t
};

[ebfull]

Since you always test against both, make a function that returns the result and panics if the result is different between them.

[ebfull]

Consider rather than using libsodium, use https://crates.io/crates/blake2b since it offers personalization. (Not sure if we can cache states and things like that with it though.)

[str4d]

@ebfull comments addressed. I also tried adding benchmarks locally, but couldn't get cargo bench to recognise the rustzcash library.

[ebfull]

a.indicies.iter().zip(b.indicies.iter()).all(|(a, b)| a != b)

[str4d]

AIUI all() runs the closure on every item in the iterator, which in this case is pairs of indices (a.indices[i], b.indices[i]) due to the zip(). What we actually want is to run the closure on every possible pair of indices (a.indices[i], b.indices[j]).

[ebfull]

Ah. itertools::cartesian_product would be useful here if only I felt good about bringing that into the dependency tree. :)

[Eirik0]

Is this section performance critical or are these lists big? This could be n log(n) if we first sorted the lists. I'm surprised there isn't a trivial way to do this in Rust, but I couldn't find anything immediately.

[ebfull]

Why not do .extend()? Then a and b don't need to be mutable references, and in other parts of your code mutable temporaries don't need to be created.

[str4d]

Switched to .extend(), and also realised that in the recursive case, I can take ownership of the underlying child Nodes, and extend one's indices with the other to eliminate half of the copying cost.

[str4d]

I can't do that in the iterative case because I'm using chunks() which is a representation, which I can't take ownership of (unless there's a clever way to do so).

[str4d]

@ebfull addressed your comments, and also fixed a bug with certain Equihash parameters (including our mainnet ones) by porting over expand_array().

[str4d]

Rebased on master, and switched to using @gtank's blake2-rfc branch (the same commit used by sapling-crypto.

[ebfull]

Use with_capacity here if you can.

[ebfull]

Ah. itertools::cartesian_product would be useful here if only I felt good about bringing that into the dependency tree. :)

[ebfull]

The 'a lifetime isn't used.

[ebfull]

re-ACK

[str4d]

Rebased on master to fix merge conflicts.

[ebfull]

I think that on average this will end up performing a re-allocation. I wouldn't be surprised if it's just as efficient to always use from_children_ref.

[Eirik0]

If not, would it be modifying a's indices? In either event, I am in favor of removing this method and just having from_children_ref which we could then remove the '_ref' from

[ebfull]

I don't know enough about Equihash to determine if this is a sufficient check.

[Eirik0]

The name of this method could be changed to is_first_index_before or something like that. I think saying indices is misleading.

[ebfull]

It would be better to create a fixed-width stack-allocated array here.

let mut lei = [0u8; 4];
(&mut lei[..]).write_u32::<LittleEndian>(i).unwrap();

[str4d]

Rebased on master to fix merge conflicts, and addressed @ebfull's comment.

[Eirik0]

self.indices_per_hash_output() * self.n / 8
=(512 / self.n) * self.n / 8
=(512 * self.n) / (self.n * 8)
=512/8
=64

[str4d]

(512 / self.n) is floor division, so this isn't commutative.

[Eirik0]

I think we should extract this into a method (from the line starting "if !has_collision") and then reuse it in is_valid_solution_iterative. It's a short block of code, but it will have the added benefit of making this look much nicer.

This would lend itself to consolidating the from_children functions.

[Eirik0]

The name of this method could be changed to is_first_index_before or something like that. I think saying indices is misleading.

[Eirik0]

If not, would it be modifying a's indices? In either event, I am in favor of removing this method and just having from_children_ref which we could then remove the '_ref' from

[Eirik0]

Is this section performance critical or are these lists big? This could be n log(n) if we first sorted the lists. I'm surprised there isn't a trivial way to do this in Rust, but I couldn't find anything immediately.

[Eirik0]

It would be nice to extract this else block in to a function. This would make more clear the similarities and differences between this function and the iterative version.

[Eirik0]

Why do we use hash_len here rather than p.collision_byte_length()?

[str4d]

The final round collides two chunks instead of one, and since we've been subtracting from hash_len along the way, it's the correct length, so it's fine to say "just check the entire remaining hash is zero".

[str4d]

@Eirik0 and I paired to refactor some of the code, and also rebased this on master to fix merge conflicts.

[Eirik0]

ACK.
Discussed all the comments/questions in a pairing with @str4d