Hi,
I want to use the crate Jubjub in your workspace in my Rust project.
Do you know what I should specify in my Cargo.toml?

TestConstraintSystem is a useful module for testing circuit code, that currently lives inside the sapling-crypto crate alongside the Sprout and Sapling circuits. It should be moved somewhere that is obviously part of the toolkit for circuit implementers.

This will probably be a new crate that depends on bellman (given that @ebfull wants bellman to be flexible enough that such things don't need to be declared or maintained by bellman itself).

Part of #41.

e.g. #92 (comment)

There are strategies we can use for this, such as creating a "last raised error" FFI function.

Most Windows systems when using ANSI APIs, and some Unix systems, do not use UTF-8 encoding for filesystem paths. It's also common for usernames to have non-ASCII characters. In that case parameter loading will fail here:

The comment is correct; we should not be using CStr, since CStr::to_str assumes UTF-8 (see doc just above here).

Strictly speaking the Right Way to do it on Windows is to use "wide" (UTF-16) filesystem APIs. I don't know how well supported that is in Rust, and for now I'd recommend sticking to ANSI APIs and just passing the bytes through as-is.

With #12 merged, these wrappers are now trivial and thus redundant.

If we want to retain the slight cleanliness improvement (two lines into one) from read_le(), we should move it to Fs::from_le().

From #84.

• #92 (comment)
• #92 (comment)
• #92 (comment)
• #92 (comment)
• #92 (comment)

We should do this once the current crop of PRs is through, before the next phase of the refactor. We should then enforce consistent formatting in Travis CI.

Hi,

When I was testing librustzcash.a with a c++ testcase, I encounter the following error:

./librustzcash.a(compiler_builtins-d0572f7a936161bf.compiler_builtins.2h648oya-cgu.0.rcgu.o): In function `compiler_builtins::int::sdiv::rust_i128_div':
/rustc/9fda7c2237db910e41d6a712e9a2139b352e558b//src/rustc/compiler_builtins_shim/../../libcompiler_builtins/src/macros.rs:293: multiple definition of `__divti3'
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/libgcc.a(_divdi3.o):(.text+0x0): first defined here

It seems a redefinition of some function with libgcc ?
Or the way I link the static lib is incorrect?

Looking forward to your reply.
Thanks!

According to #41 there is a stated goal to

2. Separate components so that hardware wallets, light wallets etc. can use lightweight, portable implementations of the pieces that they need.

and this issue aligns directly with that goal. Currently all 3 of these parameter files are needed to init librustzcash:

3.8M	sapling-output.params
 46M	sapling-spend.params
692M	sprout-groth16.params

There now exist multiple Zcash source code forks (at least Hush and Pirate) that no longer use Sprout param files. Recently I was able to make it so Hush does not download 900MB of sprout files, since the new Hush mainnet has never had a Sprout transaction. It was a completely wasteful download: wasteful of user time+bandwidth and also costly in terms of Amazon CloudFront costs.

I looked into the Rust code to see if it was possible to initialize librustzcash without sprout-groth16.params, but it seems baked in deep. Is the librustzcash willing to support this use case or does it fall outside your support realm?

What I am looking for are versions oflibrustzcash_init_zksnark_params that do not take sprout-groth16 file as parameter, or perhaps will not fail if null values are given for sprout-groth16:

https://github.com/zcash/librustzcash/blob/master/librustzcash/src/rustzcash.rs#L123

If you can make your library more flexible, I imagine many thousands of dollars per year can be saved in Amazon Cloudfront costs, by not having every full node of various other coins downloading files that they don't actually need.

Hello,

I'm trying to write a test similar to the one written in mimc.rs which does the following steps for the mimc hash function: generate parameters, create proof and then verify it. I want to do this for the blake2s hash function using the blake2s circuit already implemented in the sapling-crypto crate. The problem I am having is that verification is failing and I don't know what I'm doing wrong. Here are the major components of the code

/// This is our demo circuit for proving knowledge of the
/// preimage of a Blake2s hash invocation.
struct Blake2sDemo<E: Engine>  {
    preimage: Vec<Boolean>,
    phantom: PhantomData<E>
}

impl<E: Engine> Circuit<E> for Blake2sDemo<E>  {
    fn synthesize<CS: ConstraintSystem<E>>(
        self,
        cs: &mut CS
    ) -> Result<(), SynthesisError>
    {
        // Compute blake2s circuit
        let image = blake2s::blake2s(
            cs.namespace(|| "computation of image"),
            &self.preimage,
            CRH_IVK_PERSONALIZATION
        )?;

        // makes the image a public input to the circuit
        multipack::pack_into_inputs(cs.namespace(|| "pack nullifier"), &image);
        Ok(())
    }
}

fn test_blake2s() {
    let rng = &mut thread_rng();

    // Create parameters for our circuit
    let mut rng2 = XorShiftRng::from_seed([0x5dbe6259, 0x8d313d76, 0x3237db17, 0xe5bc0654]);
    let input_len = 8;
    let data: Vec<u8> = (0..input_len).map(|_| rng2.gen()).collect();
  
    let params = {
        let c = Blake2sDemo {
            preimage: convert_to_boolean_vec(&data),
            phantom: PhantomData
        };

        generate_random_parameters::<Bls12, _, _>(c, rng).unwrap()
    };

    // Prepare the verification key (for proof verification)
    let pvk = prepare_verifying_key(&params.vk);

    const SAMPLES: u32 = 1;
    let mut proof_vec = vec![];

    for _ in 0..SAMPLES {
        // Generate a random preimage and compute the image
        // TODO: understand if msg has to have a multiple of 8 bytes
        let mut rng2 = XorShiftRng::from_seed([0x5dbe6259, 0x8d313d76, 0x3237db17, 0xe5bc0654]);

        let input_len = 8;
        let mut h = Blake2s::with_params(32, &[], &[], CRH_IVK_PERSONALIZATION);
        let data: Vec<u8> = (0..input_len).map(|_| rng2.gen()).collect();
        h.update(&data);
        let hash_result = h.finalize();

        let preimage = data;
        let image = hash_result.as_ref().to_vec();

        proof_vec.truncate(0);

        // Create an instance of our circuit (with the witness)
        {
            let c = Blake2sDemo {
                preimage: convert_to_boolean_vec(&preimage),
                phantom: PhantomData
            };
            // Create a groth16 proof with our parameters.
            let proof = create_random_proof(c, &params, rng).unwrap();

            proof.write(&mut proof_vec).unwrap();
        }

        total_proving += start.elapsed();

        let start = Instant::now();
        let proof = Proof::read(&proof_vec[..]).unwrap();
         
        let mut public_input = [Fr::zero(); 2];
        // Add the blake2s hash output as public input to circuit through multiscalar packing
        {
            let image = multipack::bytes_to_bits_le(&image);
            println!("num bits of black256: {}", image.len());
            let image = multipack::compute_multipacking::<Bls12>(&image);

            assert_eq!(image.len(), 2);

            public_input[0] = image[0];
            public_input[1] = image[1];
            println!("pub input 0: {:?}", public_input[0]);
            println!("pub input 1: {:?}", public_input[1]);
        }
        
        // Check the proof: THIS FAILS
        assert!(verify_proof(
            &pvk,
            &proof,
            &public_input
        ).unwrap());
    }
}

I've spent many days debugging ensuring things like I'm passing the correct preimage to the circuit and hash function. What am I missing? Is parameter generation supposed to be done differently?

My overall goal is to strip out components of the full sapling circuit to build a smaller circuit and this test was my first step towards that. Any help would be appreciated.

In the protocol specification, when verifying proof, nullifier is split into 2 parts as follows.

In check_spend function,

E::Fr::CAPACITY is 254 rather than 251. It means nf_{0..253}^{old} and nf_{254,255}^{old} are used in public_input[5] and public_input[6], respectively. In fact, it is reasonable that CAPACITY is 254, since the rs is 255-bit.

The bvk field in the SaplingProvingContext and SaplingVerifyingContext structs does not actually contain bvk. It actually tracks the sum of the spend cv minus the sum of the output cv, which is then used at the end along with valueBalance to calculate bvk. We should rename it (e.g. to cv_sum) and document it to make this clearer.

• #66 (comment)
• #66 (comment)

We should address this once we reach the point in #41 of cleaning up crate APIs.

Right now zcash_client_sqlite (and lightwalletd) doesn't gracefully handle reorgs. We need to make changes to handle those reorgs more gracefully. Relates to Electric-Coin-Company/zcash-android-wallet-sdk#23.

#29 (comment)

This will take a point P and a secret sk and compute [sk] [8] P or fail if P is not valid.

There are official Zcash binaries which seem to implies there are official librustzcash binaries but they are statically linked.

Compilers versions and flags play a great role in security for rust. Please let testing the library itself !

I'm getting a test failure when running cargo test uint32_shr. Is this normal/expected, or do I have something odd in my configuration?

Here's the tail end of the traceback:

   7: core::panicking::panic_fmt
             at libcore/panicking.rs:77
   8: core::panicking::panic
             at libcore/panicking.rs:52
   9: sapling_crypto::circuit::uint32::test::test_uint32_shr
             at sapling-crypto/src/circuit/uint32.rs:631
  10: sapling_crypto::__test::TESTS::{{closure}}
             at sapling-crypto/src/circuit/uint32.rs:624
  11: core::ops::function::FnOnce::call_once
             at /checkout/src/libcore/ops/function.rs:223
  12: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1451
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:642
  13: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105

Some specs:

• rustc 1.29.2 (17a9dc751 2018-10-05)
• cargo 1.29.0 (524a578d7 2018-08-05)
• Fedora 28
• Linux Kernel 4.19.2

The Sapling logic for proving and verifying was refactored into the zcash_proofs crate in #30. We should add tests!

The tests themselves should be pretty straightforward. However, we can't assume that the test runner has the mainnet Sapling parameters, so we probably want to generate fake parameters on-the-fly (if this is not too complex or slow to do in a test).

I looked at all of zcash_client_backend, zcash_client_sqlite, and some of zcash_primitives in revision ae63116. I was only focused on stuff related to the mobile wallet. I'm posting here instead of in @str4d's fork for greater visibility.

tl;dr: No major issues, mostly just stuff that hasn't been implemented yet, side-channel attack ideas, and a suggestion to adjust the threat model so that light wallets can recover from briefly connecting to a compromised lightwalletd.

Design Comments

• The light client is written assuming lightwalletd is sending correct data to the light client. In practice this won't be perfectly true. I think the threat model should be adjusted so that a light wallet must be able to recover from having once connected to a malicious lightwalletd. This seems likely to happen, e.g. when a popular lightwalletd server gets compromised temporarily. Right now it's possible for a bad lightwalletd to permanently DoS a light wallet, probably in a couple ways. One is that the highest-numbered block in the cache is taken as authoritative in validate_combined_chain, so a bad lightwalletd can send a very-high-numbered fake block which won't be fixed by syncing with a good lightwalletd. A malicious lightwalletd could also corrupt past transaction data by sending fake transaction whose txid already exists in the DB and then stmt_update_tx updates its block number to the wrong value. Another way to permanently mess up a light wallet is to omit any spending transactions it sent (i.e. don't send it back to the wallet when it gets mined); the wallet will think the transaction expired and mark that balance as spendable again, and any subsequent spends of the same notes will fail.
• It doesn't look like the PoW checking specified in ZIP 307 is implemented yet. Having those checks in place will help defend against some kinds of malicious-lightwalletd attacks.
• How reorgs are implemented isn't really clear to me (i.e. the contract between the Rust stuff and the user of the Rust stuff). Splitting the responsibility for handling reorgs across this interface feels like an area where bugs will turn up. Good discussion topic for Zcon1.
• (Not a problem. Just mentioning so that I remember.) If transactions were malleable, then it would be possible to spend the same notes as a light wallet spent, but in a different transaction, which would make it look like the wallet's spend expired and it would erroneously mark those notes as unspent. (edit: I later found out transactions are malleable... so maybe this is a problem?)

zcash_client_backend

• Comparison of nullifiers is not constant time in scan_block, so an attacker observing this through local side-channels could potentially learn which nullifiers belong to the user.
• scan_block_from_bytes looks unused.
• My paranoia wants unit tests for the functions that encode/decode the keys in encoding.rs (there is already a test for encoding/decoding addresses).

zcash_client_sqlite

• rewind_to_height doesn't seem to "rewind" any notes that were discovered (and put into the received_notes table) in blocks that got rewound, so the user might be left thinking they have balance that they don't.
• Where is validate_combined_chain used? I can't find its use in the Rust crates or the SDK. (I was looking for it because I was concerned about threading/TOCTTOU bugs.)
• In send_to_address, there's a non-constant-time comparison of the viewing keys inside an SQL query. This potentially makes it easier to steal the keys through side-channels.

zcash_primitives

• parse_note_plaintext_minus_memo extracts the values from the unauthenticated plaintext. There are a couple ways this function can return None, and if an adversary can distinguish between them through side-channels they can use it as a decryption oracle. I think you could get both rcm and v by flipping bits in the ciphertext in a binary-search-like manner until you find a value that's one off from the maximums they are compared against (MAX_MONEY and MODULUS respectively). The comparison of the note commitment is also not constant time so it may be possible to learn the computed commitment value based on how long it takes the comparison to fail.
• spending_key and the ZIP32 functions it calls don't enforce ZIP32's requirement that the seed "MUST be at least 32 bytes."

Stuff I didn't look at

• Dependencies.
• ZIP32 implementation (a bug here could lead to use of weak keys, but it looks like it has good tests).
• Most of zcash_primitives.
• Protobuf, I assume the spec and generated code work correctly.

In current librustzcash, FFI wrapper code is mixed with cryptographic code. An example short enough to show in full is:

I recommend instead that the FFI wrapper functions be strictly separated from cryptographic code, something like this (uncompiled):

#[no_mangle] 
pub extern "system" fn librustzcash_ask_to_ak(
    ask: *const [c_uchar; 32],
    result: *mut [c_uchar; 32],
) {
    let ask = unsafe { &*ask };
    let ak = ask_to_ak(ask);
    let result = unsafe { &mut *result };
    ak.write(&mut result[..]).expect("length is 32 bytes");
}

fn ask_to_ask(ask: &[u8; 32]) -> edwards::Point<Bls12, PrimeOrder> {
    fixed_scalar_mult(ask, FixedGenerators::SpendingKeyGenerator); 
}

In this example the cryptographic code is trivial, but there are many functions in librustzcash, such as librustzcash_sapling_check_spend, that currently have extensive, tricky, security-critical cryptographic code that is peppered with unsafe blocks for argument handling.

The advantages of separating out the FFI wrappers as above are:

• There is less distraction from "unsafe-but-not-really" blocks when reading the cryptographic code.
• It is likely to be easier to review the FFI wrapper code separately from the cryptographic code.
• The cryptographic functions can have much more precise and idiomatic types (as shown by the example).
• librustzcash could be used directly from Rust without any unsafe code being called, and without having to use C-style types.
• Safe wrappers could be provided for languages other than Rust with a direct Rust FFI. Existing cross-safe-language FFI projects include Helix for Ruby-Rust, and Neon for NodeJS-Rust. Interoperability between memory-safe languages without going via C is important, and direct interfaces to Rust are likely to be supported for a wider range of languages as Rust becomes more popular.

Goals:

1. Improve code quality and testing coverage to decrease the chance of critical bugs. This includes modifying code to match the specification in areas where the specification diverged. Also, clearly specifying code that is variable time.
2. Separate components so that hardware wallets, light wallets etc. can use lightweight, portable implementations of the pieces that they need.
3. Expose more engineers to the cryptographic underpinnings and algorithms, at the implementation level, to improve code maintenance.

Strategy:

Different components will be built and cleaned up independently and then brought together as git subtrees in this librustzcash repository as code matures.

Primitive crates (not Zcash specific, generally useful):

1. zkcrypto/jubjub implements the Jubjub elliptic curve, the scalar field Fr, and the base field Fq, as defined in the Zcash protocol specification.
2. zkcrypto/ff implements traits for finite fields. We will not be bringing in ff_derive or any other macros, to reduce dependency trees. jubjub will bring this in as a dependency.
3. zkcrypto/group implements traits and generic group tools such as multi-exponentiation and FFTs. (TODO: multi-threaded variants of these algorithms; enabled by crate feature, or built outside this crate by exposing primitives?)
4. zkcrypto/bellman implements circuit traits and primitive structures, as well as basic gadget implementations such as booleans, number abstractions, etc. Unlike now, bellman will be generic using ff and won't be pairing-specific.
5. zkcrypto/pairing implements basic traits for pairing-friendly elliptic curve constructions.
6. zkcrypto/bls12_381 implements BLS12-381, which brings in jubjub as a dependency (to use its Fq implementation).
7. zkcrypto/groth16 implements the Groth16 proving system as used in Zcash. This brings in pairing and bellman as dependencies.

Zcash-specific crates:

1. zcash_primitives contains implementations of crypto components in Zcash, such as keys and their derivations (ZIP32), addresses, notes, and any other structures or algorithms specific to Zcash, such as transactions.
2. zcash_proofs contains an implementation of the prover and verifier for Sapling proofs. This brings in bls12-381, groth16 as dependencies and contains the implementation of the Spend/Output circuits.
3. zcash_client_backend contains tools for maintaining a Zcash wallet (something which can send and receive payments, maintain keys, etc.)
4. librustzcash is a C FFI for consensus-rule verification of Sapling transactions, interacting with the prover from C++, etc.

Task list (as of 2019-08-14):

• Merge zip32 crate into zcash_primitives.
  • #88
• Split up sapling-crypto across bellman, zcash_primitives, and zcash_proofs.
  • #104
• Implement new jubjub crate.
• Pull jubjub into workspace.
  • #62
• Refactor ff traits to match jubjub API.
  • #223
• impl ff::* for jubjub
  • #227
• ⚠️ Refactor group traits to match jubjub API.
  • #230, #241
• ⚠️ Move multicore logic from bellman into group.
• impl group::* for jubjub
  • #245
• ⚠️ Implement new bls12_381 crate.
• Pull bls12_381 into workspace.
• impl ff::* for bls12_381
  • #227
• impl group::* for bls12_381
  • #245
• Refactor pairing traits to match bls12_381 API.
  • #238
• impl pairing::* for bls12_381
  • #245
• Replace zcash_primitives::jubjub with the new crate.
  • #272
• Replace pairing::bls12_381 with the new crate.
  • #272
• Extract groth16 from bellman.
  • Moved to zkcrypto/bellman#51

I'm trying to do a JNR wrapper of the library, and i can't find unit tests for methods like: librustzcash_sapling_check_spend, librustzcash_sapling_binding_sig, etc. It is in your future plans to add them?

I'm a rust n00b so...

Any known examples for creating proofs and nullifiers from C/C++?

The deployed circuits for Sapling (and hybrid Sprout) are not going to change now that Sapling has activated, so there is no need for callers to specify the expected hash of the files. (Doing so was useful during the testing phase, before the final parameters were generated via the MPC.)

#4 (comment)

This is mainly a technicality; 64 bytes should be sufficient for security and the resulting alpha will be adequately uniform. However, it was specified as 640 bits, i.e. 80 bytes in the protocol spec (RedDSA.GenRandom in section 5.4.6). Either the spec or the implementation should be changed; I would prefer the implementation.

Instead of requiring all APIs to contain parameter fields, we can leverage the Rust features system to have this handled at build time. The will be a feature on the zcash_client_backend crate and possibly on other crates depending on what makes sense.

It is currently possible to construct an invalid PaymentAddress, and then pass it to functions that assume a valid address (e.g. #92 (comment)). For example, the specification requires that pk_d is of prime order (which excludes the identity), but the type of pk_d only enforces that it is in the prime-order subgroup (which permits the identity).

Is it possible for pedersen_hash to return the point at infinity? If so how do we ensure that such results don't lead to hash collisions?

Let's assume we have a scalar s that spans two segments (let's assume s includes the 6 personalisation bits). Further lets1 = s[0..63] and s2 = [63..-1] be the two segments and G1, G2 two prime order base points of the jubjub curve.

If I understand the code correctly, we compute pedersen_hash(s) = s1 * G1 + s2 * G2

It seems possible that this result could be the point at infinity.

In that case, if we e.g. shift s1 and s2 by one bit each, that is s1' = s1 << 1 and s2' = s2 << 1 and calculate pedersen_hash(s'), where s' = concat(bin(s1), bin(s2)) we get:

pedersen_hash(s') = (s1 << 1) * G1 + (s2 << 1) * G2 
                  = 2*s1*G1 + 2*s2*G2 
                  = 2*(s1*G1 + s2*G2) 
                  = 2 * (INF) = INF = pedersen_hash(s)

which would be a collision, wouldn't it?

I'm likely missing some basic understanding of why this is not possible or what we are doing to avoid this case, and would appreciate some insight.

Thanks.

I might not be understanding the code correctly, but where are we enforcing that the x-coordinate is correctly selected from the lookup table, when doing a 3-bit lookup with conditional negation (e.g. used in pedersen_hash)?

I can only see the y-lookup being enforced. Does enforcement happen in add_bool_with_coeff (I couldn't find any cs.enforce in that method)?
Or do we not need to verify the x-lookup for some reason?

We can do this in parallel with reading, just like I do in powersoftau/phase2. See HashReader.

As I argue here https://chat.zcashcommunity.com/channel/zcashd-team?msg=KrsprfCZBfa83ksJz
it might be better to check that rk has prime order.

We do this for other stuff, and I think this should be no exception. It avoids me having to check hashes in PRs like this.

rand crate version is now 0.6.5 while the projects in this workspace are using rand = "0.4".
Specifically Rand trait is deprecated

Once #41 is completed, the librustzcash crate should just be an FFI wrapper around a set of pure-Rust APIs. Currently, we test (some of) this FFI indirectly via testing of zcashd. We should instead have FFI tests in the librustzcash crate.

The simple way would be to duplicate tests from the pure-Rust crates, and modify them to use the FFI methods. This would probably be rather hard to maintain. It would be great to figure out a strategy to reduce test duplication, while keeping the individual tests readable. Or we might decide that doing so isn't feasible, and just decide to duplicate test logic, in which case this is an easy issue to close 😄

#66 (comment)

We currently hand-craft the librustzcash.h file for the librustzcash crate. We could instead use cbindgen to generate it automatically. We should look into what additional dependencies this would require zcashd to depend on.

e.g.

• #92 (comment)
• #92 (comment)

This function will take diversifier d, scalar esk, compute g_d = GH(d) or fail if d is invalid, and produce [esk] g_d as its result.

I'm a rust n00b so...
How do I compile and install the lib？

Hi,

I am trying to use edwards::Point from my own library.
I am using extern crate sapling_crypto; and extern crate pairing;
I run the following:

use sapling_crypto::jubjub::{edwards, fs::Fs, PrimeOrder, Unknown, JubjubBls12, JubjubEngine };
use pairing::bls12_381::Bls12;
pub type PK = edwards::Point<Bls12,Unknown>;
#[derive(Clone)]
pub struct JubjubPoint {
    purpose: &'static str,
    ge: PK,
}

and get the error that the trait 'elliptic::curves::sapling_crypto::jubjub::JubjubEngine' is not implemented for 'elliptic::curves::pairing::bls12_381::Bls12' I also tried to import all relevant traits but I get the same error.

When I try this code as part of the tests in redjubjub.rs I get no error.
Apparently I need to import the code from jubjub.rs that implements the jubjubEngine trait for Bls12 but I am not sure how to do it.

Thanks!

See https://trac.torproject.org/projects/tor/ticket/27199 , and the upstream Rust bug rust-lang/rust#52652 (in particular my comment here).

We currently do set panic = 'abort' in the release profile of librustzcash, which is why this is not a security bug (I believe) for current Zcash as built by default. I think (but I could be wrong) that to avoid the undefined behaviour, it only needs to be set for the crate that is directly being called by the FFI. We may want to set it for other crates under the librustzcash project anyway.

There are several projects that use pairing and would like to contribute to it. They will probably start collaborating on a fork, but I think everyone would also be happy to contribute upstream instead.

Will the librustzcash changes be merged back into the pairing repo eventually, or is that deprecated? Will the new version be published on crates.io?

Would you be open to merging the functionality mentioned in that issue? (Most of the pairing users' feature requests seem to overlap a lot.)
Or is it better for POA, Filecoin, MaidSafe and others to work on a separate fork for now?

The zeroize crate provides:

safe†, portable access to cross-platform intrinsics for securely zeroing memory which are specifically documented as guaranteeing they won't be "optimized away".

We should use it as appropriate to put fixed limits on the persistence of secret material in-memory.