Add interface and computation of Sapling note commitment by bitcartel · Pull Request #11 · zcash/librustzcash

bitcartel · 2018-05-15T23:02:05Z

No description provided.

str4d · 2018-05-16T19:28:59Z

This won't work, because to_uniform requires the input to be 64 bytes (so that we get approximate uniformity taking the value mod the field size). r needs to already be a valid random Jubjub point, which means we need another API to return such values (via Fs::rand(&mut rng)).

@str4d: you mean "r needs to already be a valid uniform-random Fs element".

Let's work backwards. We want a valid rust note so that we can call note.cm().

let note = sapling_crypto::primitives::Note { value, g_d, pk_d, r };

What is r? It's the commitment randomness and I've seen it defined as this: (what type is this in rust?)

let commitment_randomness: fs::Fs = rng.gen();

So from C++ what type should the commitment randomness be? Currently in C++ 'r' (short for 'rcm') is a 'uint256' which matches the spec.

If 'to_uniform' only takes 64 bytes, does that mean in C++ 'r' should be a 64 byte buffer? Yet the spec says rcm must be 256 bits.

@daira @str4d @ebfull

The spec (section 4.6.2) says that rcm is chosen uniformly on NoteCommit^Sapling.Trapdoor := F_{r_J} (Fs in the code).

@bitcartel the problem here is the API, not the types. The to_uniform API is documented as taking 64 bytes of Random Oracle output. In sapling-crypto, this is always the output of BLAKE2b (see hash_to_scalar in src/util.rs), but @ebfull and I decided to split the two components apart so as to not constrain the generic ToUniform trait to always require BLAKE2b.

What is r? It's the commitment randomness and I've seen it defined as this: (what type is this in rust?)

let commitment_randomness: fs::Fs = rng.gen();

let commitment_randomness: fs::Fs specifies that the type is Fs. We can set it to the output of rng.gen() because Rust can figure out that the random number generator can be used to create a uniformly-random Fs element (via Fs::rand(&mut rng)).

So from C++ what type should the commitment randomness be? Currently in C++ 'r' (short for 'rcm') is a 'uint256' which matches the spec.

In C++, r needs to be the serialization of commitment_randomness; in this function we need to simply read it into an Fs element (via the read_fs functions introduced in #9). This means we need another API to generate commitment_randomness and return it in serialized form (via the write_fs function introduced in #9).

Oh, to_uniform is also used by #9, taking the output of PRFexpand (which also uses BLAKE2b internally).

Note that sometimes we're given the opening of the commitment (e.g. when we're processing a decrypted note plaintext), rather than needing to create a new random one. This could technically work with an API that took a 64-byte r (because to_uniform is a no-op if the input is already in range), but that would be confusing and would need to be thoroughly documented. A separate API to return a uniform-random Fs element seems preferable.

str4d · 2018-05-16T19:42:45Z

I was going to suggest using sapling_crypto::primitives::PaymentAddress, but that involves an unnecessary copy, so the current logic is fine.

daira · 2018-05-16T20:34:26Z

@str4d: you mean "r needs to already be a valid uniform-random Fs element".

daira · 2018-05-16T20:35:23Z

This API mixes cryptographic code with an unsafe API. I suggest we have a safe API for the cryptography that is usable and testable from Rust, and then a thin wrapper around it for the C++ side. I also think this would be more thoroughly testable if we had the a Rust function with equivalent (safely typed) parameters that constructs and returns the full sapling_crypto::primitives::Note (so that you can test that it has constructed the correct note), and then a wrapper around that to return the commitment.

Good idea for the whole library.

Agreed, but not blocking.

daira · 2018-05-22T23:28:39Z

The spec (section 4.6.2) says that rcm is chosen uniformly on NoteCommit^Sapling.Trapdoor := F_{r_J} (Fs in the code).

ebfull · 2018-06-02T20:02:15Z

There's no recoverable error case, so let's not return a boolean until there is one.

Good point. Force pushed to address this.

ebfull · 2018-06-02T20:02:42Z

Agreed, but not blocking.

bitcartel · 2018-06-02T21:15:05Z

Pushed update to address review comments.

daira · 2018-06-04T20:11:11Z

The spec doesn't require pk_d to be prime order in keys obtained from another source (pk_d in a note is declared as having type KA^Sapling.Public which is J, not J_(r)). I could make it require this outside the circuit, but note that the circuit doesn't check it.

I've looked closely at the changes that would be needed to the spec, and I'm convinced that the spec should be changed to require pk_d (and g_d, but that's already implied) to be of prime order outside the circuit.

daira · 2018-06-04T20:18:23Z

We might as well delete this now. (Does not block.)

ebfull · 2018-06-04T20:53:28Z

This is going to fail because you're not calling the function.

bitcartel · 2018-06-04T21:16:11Z

This should be const. @ebfull

🤷‍♂️ It's being passed by value so it doesn't really matter.

str4d · 2018-06-05T03:19:37Z

+            &tv.note_r,
+            &mut result
+        ));
+        assert_eq!(&result, &tv.note_cm);


I would have just put this in src/tests/key_components.rs, rather than duplicating the test vectors. But we can clean this up later.

str4d · 2018-06-05T03:21:10Z

+    };
+
+    let result = unsafe { &mut *result };
+    write_le(note.cm(&JUBJUB).into_repr(), &mut result[..]);


The PR uses both FrRepr::write_le(_) and write_le(&FrRepr, _) (which wraps the former). Non-blocking, as we should refactor the entire module to remove the now-unnecessary write_le() wrapper.

Opened #16.

Implements and documents serialization Closes #11.

Remove some mutable variables and use multiplication operator

…14868de..23f0768ea 23f0768ea Release lightwallet-protocol v0.4.0 41156c767 Merge pull request #11 from zcash/feature/get_mempool_tx_pools 7c130e883 Add `lightwalletProtocolVersion` field to `LightdInfo` struct. edbb726d7 Apply suggestion from code review 38fddd73b Apply suggestions from code review 0250f2720 Add pool type filtering to `GetMempoolTx` argument. 54ccaadd5 Change semantics of pool-based pruning of compact transactions from "may prune" to "must prune". b0667ec99 Merge pull request #9 from zcash/2025-11-doc-TransparentAddressBlockFilter f3fea7bd4 doc: TransparentAddressBlockFilter doesn't include mempool a67dd323a Merge pull request #8 from zcash/2025-11-lightdinfo-upgrade-info 11da4b7e3 add next upgrade info to LightdInfo structure (GetLightdInfo) 42cd8f720 Transparent data docs update (#7) c0cf957ac Merge pull request #5 from zcash/2025-11-comments 912fc3609 Minor clarification in GetBlockRange documentation. 6b03f2cce Documentation (comments) only d978256a2 Merge pull request #1 from zcash/compact_tx_transparent 7eeb82e7c Merge pull request #4 from zcash/add_changelog a95359dc9 Apply suggestions from code review 592b637a8 Add transparent data to the `CompactBlock` format. 9d1fb2c41 Add a CHANGELOG.md that documents the evolution of the light client protocol. 180717dfa Merge pull request #3 from zcash/merge_librustzcash_history 450bd4181 Merge the history of the .proto files from `librustzcash` for complete history preservation. a4859d11d Move protobuf files into place for use in `zcash/lightwallet-protocol` 2e66cdd9e Update zcash_client_backend/proto/service.proto eda012519 fix comment f838d10ad Add gRPC LightdInfo Donation Address db12c0415 Merge pull request #1473 from nuttycom/wallet/enrichment_queue 698feba96 Apply suggestions from code review 20ce57ab3 zcash_client_backend: Add `block_height` argument to `decrypt_and_store_transaction` a6dea1da8 Merge pull request #1482 from zancas/doc_tweak 4d2d45fc9 fix incorrect doc-comment e826f4740 update CompactBlock doc-comment, to cover non-Sapling shielded notes, and addresses e9a6c00bf Various documentation improvements 988bc7214 Merge pull request #872 from nuttycom/feature/pre_dag_sync-suggest_scan_ranges 58d07d469 Implement `suggest_scan_ranges` and `update_chain_tip` a9222b338 Address comments from code review. e20310857 Rename proto::compact::{BlockMetadata => ChainMetadata} ac63418c5 Reorganize Sapling and Orchard note commitment tree sizes in CompactBlock. 0fdca14f1 zcash_client_backend: Add note commitment tree sizes to `CompactBlock` serialization. 2a0c2b8b7 zcash_client_backend: Add gRPC bindings behind feature flag 1342f0480 zcash_client_backend: Address compact_formats.proto comments 68aa4e01b zcash_client_backend: Bring in latest `compact_formats.proto` e712eb1bc Add prevHash field to CompactBlock 440384c3e Build protobufs for compact formats git-subtree-dir: zcash_client_backend/lightwallet-protocol git-subtree-split: 23f0768ea4471b63285f3c0e9b6fbb361674aa2b

Improve security of z_getencryptionaddress function in verus_zfunc crate

Merge upstream to get `core2` fix

bitcartel requested review from ebfull and str4d May 15, 2018 23:02

bitcartel self-assigned this May 16, 2018

str4d requested changes May 16, 2018

View reviewed changes

bitcartel force-pushed the 3061_add_sapling_note_commitment_calculation branch from cb3371b to 10b10ac Compare May 21, 2018 21:46

daira reviewed May 22, 2018

View reviewed changes

bitcartel force-pushed the 3061_add_sapling_note_commitment_calculation branch from 84b77ab to 7b1d764 Compare June 1, 2018 21:31

bitcartel changed the title ~~Add interface and computation for Sapling note commitment~~ Add interface and computation of Sapling note commitment Jun 2, 2018

ebfull requested changes Jun 2, 2018

View reviewed changes

bitcartel force-pushed the 3061_add_sapling_note_commitment_calculation branch from 73dd38f to a579039 Compare June 2, 2018 21:13

ebfull approved these changes Jun 4, 2018

View reviewed changes

daira reviewed Jun 4, 2018

View reviewed changes

ebfull requested changes Jun 4, 2018

View reviewed changes

bitcartel commented Jun 4, 2018

View reviewed changes

bitcartel added 4 commits June 4, 2018 14:24

Add ffi and computation for Sapling note commitment.

4ef2e9a

Add test for generating commitment randomness.

eb3d8aa

Add test of librustzcash_sapling_compute_cm against test vectors.

44e2a53

Update authors.

943df43

bitcartel force-pushed the 3061_add_sapling_note_commitment_calculation branch from a579039 to 943df43 Compare June 4, 2018 21:24

ebfull approved these changes Jun 4, 2018

View reviewed changes

str4d approved these changes Jun 5, 2018

View reviewed changes

str4d merged commit 2e8a11a into zcash:master Jun 5, 2018

str4d referenced this pull request in str4d/librustzcash Aug 28, 2018

Implements and documents serialization, closes #11.

c618240

str4d referenced this pull request in str4d/librustzcash Aug 28, 2018

Auto merge of zcash#15 - ebfull:serialization-impl, r=ebfull

6df2c00

Implements and documents serialization Closes #11.

ebfull added a commit to ebfull/librustzcash that referenced this pull request Feb 23, 2019

Merge pull request zcash#11 from Eirik0/fq-cleanup

d61b6bc

Remove some mutable variables and use multiplication operator

michaeltout pushed a commit to VerusCoin/librustzcash that referenced this pull request Mar 17, 2026

Merge pull request zcash#11 from who-biz/ka_agree-enc-addr-derivation

9c8627e

Improve security of z_getencryptionaddress function in verus_zfunc crate

S1nus added a commit to S1nus/librustzcash that referenced this pull request May 1, 2026

Merge pull request zcash#11 from S1nus/merge-upstream-apr-30

c24b6b8

Merge upstream to get `core2` fix

Conversation

bitcartel commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel commented Jun 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daira May 16, 2018 •

edited

Loading

daira May 22, 2018 •

edited

Loading

daira May 16, 2018 •

edited

Loading

daira May 22, 2018 •

edited

Loading

daira Jun 4, 2018 •

edited

Loading