Amount and signatures use CompressedRistretto #17

mfaulk · 2020-04-16T03:39:42Z

Previously, Amount.commitment was a RistrettoPoint (type aliased as "Commitment'), while the rest of the transaction members are CompressedRistretto. Using RistrettoPoint in Amount causes unnecessary compress and decompress operations. For example, computing the hash, digest, serialization, or deserialization of a Tx involves compressing the Amount commitment for each ring element and transaction output.

This PR adds the newtypes Commitment and CompressedCommitment, and updates Amount to use CompressedCommitment. This should prevent most of the unnecessary compress/decompress operations related to Amount.

This also changes SignatureRctBulletproof's sign and verify methods to take one-time addresses and amount commitments in compressed form (CompressedRistrettoPublic and CompressedCommitment). I think this nicely encapsulates the compress/decompress logic close to where it is used.

mfaulk · 2020-04-16T03:44:15Z

transaction/core/src/amount.rs

@@ -30,7 +33,7 @@ pub enum AmountError {
 pub struct Amount {
    /// A Pedersen commitment `v*G + b*H` to a quantity `v` of MobileCoin, with blinding `b`,
    #[prost(message, required, tag = "1")]
-    pub commitment: Commitment,
+    pub commitment: CompressedCommitment,


Previously, "Commitment" was a type alias for CurvePoint, which we used for any RistrettoPoint that we wanted to serialize.

This now uses the newtype CompressedCommitment which wraps CompressedRistretto. The dedicated newtype gives better type checking, and I think that storing this as a compressed point avoids ambiguity and is more performant (similar to how KeyImages are represented as compressed points).

mfaulk · 2020-04-16T03:44:59Z

transaction/core/src/ring_signature/rct_bulletproofs.rs

@@ -46,6 +48,7 @@ pub struct SignatureRctBulletproofs {
    pub ring_signatures: Vec<RingMLSAG>,

    /// Commitments of value equal to each real input.
+    /// TODO: Make this a CompressedCommitment.


This will be in a follow-on PR about serialization.

mfaulk · 2020-04-16T03:46:39Z

transaction/core/src/ring_signature/rct_bulletproofs.rs

@@ -153,7 +156,7 @@ impl SignatureRctBulletproofs {
    /// * `output_values_and_blindings` - Value and blinding for each output amount commitment.
    pub fn sign<CSPRNG: RngCore + CryptoRng>(
        message: &[u8; 32],
-        rings: &[Vec<(Address, Commitment)>],
+        rings: &[Vec<(CompressedRistrettoPublic, CompressedCommitment)>],


This now takes compressed points, which can be read directly from the TxOuts in a transaction.

mfaulk · 2020-04-16T03:47:03Z

transaction/core/src/ring_signature/rct_bulletproofs.rs

@@ -180,8 +183,8 @@ impl SignatureRctBulletproofs {
    pub fn verify<CSPRNG: RngCore + CryptoRng>(
        &self,
        message: &[u8; 32],
-        rings: &[Vec<(Address, Commitment)>],
-        output_commitments: &[Commitment],
+        rings: &[Vec<(CompressedRistrettoPublic, CompressedCommitment)>],


This now takes compressed points, which can be read directly from the TxOuts in a transaction.

mfaulk · 2020-04-16T19:07:47Z

transaction/core/src/ring_signature/mlsag.rs

@@ -133,7 +135,7 @@ impl RingMLSAG {
    // * `rng` - Randomness.
    pub fn sign<CSPRNG: RngCore + CryptoRng>(
        message: &[u8; 32],
-        ring: &[(Address, Commitment)],


Address and Commitment were both type aliases for RistrettoPoint. The aliases gave some readability, but they didn't actually provide type safety (for example, you could pass an Address to a function that takes a Commitment). This PR removes them in favor of more specific newtypes.

transaction/core/src/ring_signature/mlsag.rs

mfaulk · 2020-04-16T19:26:17Z

transaction/core/src/validation/validate.rs

-    for tx_in in &tx.prefix.inputs {
-        let mut ring: Vec<(Address, Commitment)> = Vec::new();
-        for tx_out in &tx_in.ring {
-            let address = RistrettoPublic::try_from(&tx_out.target_key)?;


This try_from performs a decompression.

mfaulk · 2020-04-16T19:29:21Z

transaction/core/src/ring_signature/mlsag.rs

@@ -347,11 +364,24 @@ impl RingMLSAG {
    }
 }

+fn decompress_ring(


This should now be the only place that input rings are decompressed on the consensus server.

xoloki

LGTM

jcape

A couple stylistic nitpicks, and (more importantly) there are some unnecessary compress/decompress ops which can be removed.

jcape · 2020-04-16T20:43:38Z

consensus/api/src/conversions.rs

+        let target_key: CompressedRistrettoPublic = RistrettoPublic::try_from(target_key_bytes)
            .map_err(|_| ConversionError::KeyCastError)?
            .into();


Suggested change

let target_key: CompressedRistrettoPublic = RistrettoPublic::try_from(target_key_bytes)

.map_err(|_| ConversionError::KeyCastError)?

.into();

let target_key = CompressedRistrettoPublic::try_from(target_key_bytes).map_err(|_| ConversionError::KeyCastError)?;

let _test = RistrettoPublic::try_from(&target_key).map_err(|_| ConversionError::KeyCastError)?;

Right now, this loads the bytes into a compressed ristretto, decompresses that, then compresses it again. If the intent is to check that we've actually been given a ristretto point up front, the second compression can be skipped (or both can be, if it isn't).

jcape · 2020-04-16T20:44:08Z

consensus/api/src/conversions.rs

+        let public_key: CompressedRistrettoPublic = RistrettoPublic::try_from(public_key_bytes)
            .map_err(|_| ConversionError::KeyCastError)?
            .into();


Suggested change

let public_key: CompressedRistrettoPublic = RistrettoPublic::try_from(public_key_bytes)

.map_err(|_| ConversionError::KeyCastError)?

.into();

let public_key = CompressedRistrettoPublic::try_from(public_key_bytes).map_err(|_| ConversionError::KeyCastError)?;

let _test = RistrettoPublic::try_from(&public_key).map_err(|_| ConversionError::KeyCastError)?;

jcape · 2020-04-16T21:23:16Z

transaction/core/src/ring_signature/mlsag.rs

        let r = &self.responses;

+        // Output commitment must decompress.
+        let output_commitment: Commitment = Commitment::try_from(output_commitment)?;


Suggested change

let output_commitment: Commitment = Commitment::try_from(output_commitment)?;

let output_commitment = Commitment::try_from(output_commitment)?;

jcape · 2020-04-16T21:26:34Z

transaction/core/src/ring_signature/rct_bulletproofs.rs

-            .collect();
+        // output_commitments must decompress.
+        // This ensures that each commitment encodes a valid Ristretto point.
+        let mut decompressed_output_commitments: Vec<Commitment> = Vec::new();


Suggested change

let mut decompressed_output_commitments: Vec<Commitment> = Vec::new();

let mut decompressed_output_commitments = Vec::<Commitment>::with_capacity(output_commitments.len());

@jcape

* Roll up 4-14-20 (#1) * only generate enclave pem when needed, use absolute path (#1) * switch local network to not use ssl (#6) * This commit moves mobilecoin to the mobilecoinofficial fork of prost (#8) * This commit moves mobilecoin to the mobilecoinofficial fork of prost Removing cbeck88 permissions controls I have ensured that there are branch protection rules covering the commits * Fix view enclave cargo toml to have the same revision as others * Fix cargo.lock files * Remove selfsigned from README (#9) * Adds Java namespaces to protocol buffers (#12) * fix ecies MC-1216 (#11) * fix ecies MC-1216 was mobilecoinofficial/mobilecoin-internal#321 changes since then: - Removed alloc feature - Added *_in_place_detached api (like aead crate) This reduces the amount of noise in the actual crypto part, the noise being "which bytes go where in the buffer" - Marked the `encrypt_into` and `decrypt_into` apis as not public, because those APIs suck, it should really be as much like aead crate as possible, which is a better thought-out API * add comments about fixing part of API * those APIs have to be public for now, sigh. maybe they aren't so bad * Add comments about API * [MC-1172] rm tranasction::encoders * Reorganize SCP to Cargo standards (#18) * Make /opt/intel/sgxsdk/lib64 part of LD_LIBRARY_PATH in dockerfile (#21) * Make /opt/intel/sgxsdk/lib64 part of LD_LIBRARY_PATH in dockerfile and uprev the dockerfile. This intended to fix ci in PR 14 This fixes issues like `...epid_sim.so` not being found by the test targets. It is not getting installed in `/opt/intel/sgxsdk/sdk_libs`, it is getting installed in the path mentioned ``` Running target/debug/deps/tx_recovery-3449d1ea71010602 /tmp/mobilenode/target/debug/deps/tx_recovery-3449d1ea71010602: error while loading shared libraries: libsgx_epid_sim.so: cannot open shared object file: No such file or directory error: test failed, to rerun pass '-p fog_ingest_server --test tx_recovery' root@cb7f949bccb2:/tmp/mobilenode# ldd /tmp/mobilenode/target/debug/deps/tx_recovery-3449d1ea71010602 linux-vdso.so.1 (0x00007ffc62df8000) libsgx_epid_sim.so => not found libsgx_urts_sim.so => /opt/intel/sgxsdk/sdk_libs/libsgx_urts_sim.so (0x00007fc5c8061000) libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fc5c5fd9000) libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fc5c5dd5000) librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fc5c5bcd000) libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fc5c59ae000) libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fc5c5796000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fc5c53a5000) /lib64/ld-linux-x86-64.so.2 (0x00007fc5c7e6c000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fc5c5007000) libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007fc5c4b3c000) libsgx_uae_service_sim.so => /opt/intel/sgxsdk/sdk_libs/libsgx_uae_service_sim.so (0x00007fc5c8059000) root@cb7f949bccb2:/tmp/mobilenode# ls /opt/intel/sgxsdk/sdk_libs/libsgx_u libsgx_uae_service_sim.so libsgx_urts_sim.so root@cb7f949bccb2:/tmp/mobilenode# ls /opt/intel/sgxsdk/sdk_libs/ libsgx_uae_service_sim.so libsgx_urts_sim.so root@cb7f949bccb2:/tmp/mobilenode# ``` * Make circle ci not source the intel sgx environment As much as possible, the setting up of the build enviornment should be done in the Dockerfile. Not duplicating these lines throughout jenkins, k8s, mob tool, and README is a good thing. (they can be in readme for people who don't want to use the container.) * mobilecoind: b58 endpoints (#22) * mobilecoind b58 rpc endpoints * mobilecoind2: implement generate transfer code tx * test_generate_transfer_code_tx * Basic framework for Java mobilecoind client based on Gradle (#23) * Fixes class name in build (#24) * Replaces "Mobilenode" with "MobileCoin" in several READMEs (#4) * Replaces Mobilenode with MobileCoin in several READMEs * Adds src/README.md * avoid "consensus node" language * Update cloudbuild/README.md Co-Authored-By: Robb Walters <[email protected]> Co-authored-by: Robb Walters <[email protected]> * Move ledger_enclave_server.proto from mobilecoin_api to fog_api (#25) * Move ledger_enclave_server.proto from mobilecoin_api to fog_api * fix build * Actually fix build * Check outbuf_used for null in mobileenclave_call (#17) Merging this. * Tx uses SignatureRctBulletproofs (#2) * Applies patch from mobilecoin-internal * unit tests encodings * Removes unwrap in validate_transaction_signature * Re-enables test_validate_key_images_are_unique_rejects_duplicate * Removes unwraps in TransactionBuilder::build * fmt * Increases proptest cases, reorders imports * Adds CL params to Java code along with monitors and balance check (#33) * Adds CL params to Java code along with monitors and balance check * Fix help message * Build comments * Prettify * Unused import * Added a README, changed the language and flags around entropy * parameter fix * README format * Update proto to reflect current mobilecoind API * First round of suggested fixes * Adds ssl flag * Fix the 'no-vars-given' case. (#38) * Upgrade sentry to 0.18 (#36) * upgrade sentry to 0.18 * lock files * Implement mob client python session (#30) * Implement mob client python session * Reference sign up rather than create account * Start testnet client script * Rename and introduce exchange * start-testnet-mobilecoind * Add public_address * Transfer to public address * Versioned ecies (#28) * Do another pass on ecies API, `encrypt_into` -> `encrypt_in_place` This also allows `encrypt` to return an error, previously I didn't allow that, but I looked in tarcieri's actual aes-gcm crate, and it does return an error if the plaintext is larger than a huge number. I think it might be more sensible to panic there, but anyways, I'd like to make the `ecies` API close to `aead` and make it generic. In order to make that change, I needed to fix the places that were doing `encrypt_into` and `decrypt_into`, which were in the fog hints in transaction crate. So this is a good case study of how the API wokrs out. I also added a special wrapper over `&mut [u8]` called `FixedBuffer` to try to make using it nicer and close to how it worked without the `encrypt_into` functions. If we like `FixedBuffer` then I think we should try to open a PR to `aead` crate and see if Tony wants it. LMK what you think -- if we're happy with this, then in the next PR I'm going to turn this into a trait, then make a version of it that includes two "version tag" bytes so that we can have a nice forwards-and-backwards-compatible wire format for the ECIES ciphertexts. Once they get into the blockchain and into the recovery db we cannot easliy change the algorithm if we don't have that. * fix bug * fix tests * fix clippy * Create versioned Ecies wire format, integrated with Ecies trait * Add and use encrypt_fixed_length APIs for ECIES This is much cleaner than the FixedBuffer thing * Add better docu, references, naming, per code review comments trait ECIES -> RistrettoEcies * fixup previous * Rename `ecies` to `ristretto-box`, and better README / docu * Rename again per @jcape * Move crate `public/crypto/mc-crypto-box` to `public/crypto/box` per discussion * Additional functions in Java client for request codes and transfers (#42) * Additional functions in Java client for request codes and transfers * Document transfer function * Change target to recipient * 'host' = 'server' * Client subaddress (#40) * Use account/subaddress syntax * Flesh out new account * introduce mc-grpc-build and use it mobilecoind-api (#41) * introduce mc-grpc-build and use it mobilecoind-api * delete old autogenerated code * build issue fix and comments * grpc-build -> build-grpc * comment and lock file * readme and lock file * use mcbuild-utils Co-authored-by: Eran Rundstein <[email protected]> Co-authored-by: Chris Beck <[email protected]> Co-authored-by: tsegaran <[email protected]> Co-authored-by: Robb Walters <[email protected]> Co-authored-by: m a t t f a u l k n e r <[email protected]> Co-authored-by: Brian Anderson <[email protected]> Co-authored-by: James Cape <[email protected]> * Roll up 4-15-20 (#2) * introduce mc-grpc-build and use it mobilecoind-api (#41) * introduce mc-grpc-build and use it mobilecoind-api * delete old autogenerated code * build issue fix and comments * grpc-build -> build-grpc * comment and lock file * readme and lock file * use mcbuild-utils * Propose Values in Slot (#16) * Adds a status call to the Java client (#44) * Adds transaction receipt and status checks to Java client * Document parameters for status function * Balance fix (#46) * Client fixes * Better error display * Pep8 * Small fixups in mc-crypto-box README, per joey feedback (#45) I tried to make the commentary about the user-provided nonce more accurate as well * Rewrite the README for digestible crate (#47) * Rewrite the README for digestible crate * Add another sentence * Add another sentence about framing, after reading again * Fix typo * Update public/crypto/digestible/README.md Co-Authored-By: sugargoat <[email protected]> * Update public/crypto/digestible/README.md Co-Authored-By: sugargoat <[email protected]> * Update public/crypto/digestible/README.md Co-Authored-By: sugargoat <[email protected]> * Try to fix the sentences sarah commented on, fixup conclusion Co-authored-by: sugargoat <[email protected]> * Updates README for java client and a couple of minor fixes (#54) * Fix missing check (#51) * Fix missing check * Speed up grpc install * Better wording * Update path for test network (#55) * Update READMEs with sigstruct (#7) * Update READMEs with sigstruct * Add css info * Add signed enclave info * Remove aws * Fix typo * Remove privkey option and provide signed and css to consensus * Add IAS_MODE to mobilecoind Co-authored-by: Eran Rundstein <[email protected]> Co-authored-by: Robb Walters <[email protected]> Co-authored-by: tsegaran <[email protected]> Co-authored-by: garbageslam <[email protected]> * Switch to build-grpc (#7) * switch mobilecoin-api to using the new build-grpc crate * switch attest-api to using the new build-grpc crate * switch grpc-util to using the new build-grpc crate * CircleCI build improvements (#8) * add circleci task for running "cargo build", dedupe pem file generation * fix spacing * call check-dirty-git * lint and save caches in the faster job * rename job * fix typo in check-dirty-git * fix typo * Changes from internal repo (#5) * Removes MAX_TINY_MOB (#9) * Fix crypto directory README (#3) * MC-1283: Export protos directories as cargo depvars (#10) * moves tombstone_block from Tx to TxPrefix (#13) * Nicer mob behavior (check for docker being installed) (#12) This might help the user experience in issues like #6 * Improvements to build instructions in README.md (#16) * Improvements to build instructions in README.md * Fixup SGX_MODE=SIM vs. SGX_MODE=SW, and give explanation about env vars * small tweak * Expand upon the enclave build part * Spelling and grammar * Simplify python example (#14) * Simplify python example * Update README * Reference java example on top level readme * Fixup markdown rendering in various readme's (#19) This is what I get for not rendering them locally before making PR * Update PROD endpoint (#21) * Unit tests that TransactionBuilder returns error if value is not conserved (#22) * Amount and signatures use CompressedRistretto (#17) * Amount and signatures use CompressedRstretto * Comment cleanup * Unit test for Commitment * Unit test for CompressedCommitment * Comment fixes * Extracts ring decompression into a function * Comment fix * RingMLSAG derives Message (#23) * MC-1292 Replace MIN_RING_SIZE and MAX_RING_SIZE with RING_SIZE (#20) * Uses single RING_Size constant * Changes MIN_RING_SIZE to RING_SIZE * Restores test_validate_ring_sizes * Update consensus/api/proto/consensus_common.proto Co-Authored-By: James Cape <[email protected]> * Restores error variant Co-authored-by: James Cape <[email protected]> * Improve readme (#27) * Improve readme Thanks to @joekottke for suggestion to use markdown footnotes in the earlier PR comments * Remove footnote extension, it's not supported in githug-flavored-markdown * SignatureRctBulletproofs derives Message (#25) * SignatureRctBulletproofs derives Message * Removes dead code * Update command with missing params (#26) * Update command * peer-responder-id does not have public key * Typo fix * Example testnet client (#29) * wip * wip * mob and such * begin rpc integration * iterate on the flow * flow works * command line args, comments * check that mobilecoind is running * clippy Co-authored-by: Eran Rundstein <[email protected]> Co-authored-by: Chris Beck <[email protected]> Co-authored-by: tsegaran <[email protected]> Co-authored-by: Robb Walters <[email protected]> Co-authored-by: m a t t f a u l k n e r <[email protected]> Co-authored-by: Brian Anderson <[email protected]> Co-authored-by: James Cape <[email protected]>

Amount and signatures use CompressedRstretto

a6288c5

mfaulk commented Apr 16, 2020

View reviewed changes

mfaulk added 3 commits April 16, 2020 11:46

Comment cleanup

0c4f8b5

Unit test for Commitment

913fffe

Unit test for CompressedCommitment

1d1f269

mfaulk marked this pull request as ready for review April 16, 2020 19:02

mfaulk commented Apr 16, 2020

View reviewed changes

transaction/core/src/ring_signature/mlsag.rs Outdated Show resolved Hide resolved

mfaulk added 2 commits April 16, 2020 12:09

Comment fixes

f0638ce

Extracts ring decompression into a function

e71fb82

mfaulk commented Apr 16, 2020

View reviewed changes

Comment fix

59f4bdd

mfaulk commented Apr 16, 2020

View reviewed changes

xoloki approved these changes Apr 16, 2020

View reviewed changes

cbeck88 approved these changes Apr 17, 2020

View reviewed changes

mfaulk merged commit 01d5bc7 into mobilecoinfoundation:master Apr 17, 2020

jcape suggested changes Apr 17, 2020

View reviewed changes

gitbook-com bot pushed a commit that referenced this pull request Nov 17, 2021

GitBook: [#17] text

297173f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amount and signatures use CompressedRistretto #17

Amount and signatures use CompressedRistretto #17

mfaulk commented Apr 16, 2020 •

edited

Loading

mfaulk Apr 16, 2020

mfaulk Apr 16, 2020 •

edited

Loading

mfaulk Apr 16, 2020

mfaulk Apr 16, 2020

mfaulk Apr 16, 2020

mfaulk Apr 16, 2020

mfaulk Apr 16, 2020

xoloki left a comment

jcape left a comment

jcape Apr 16, 2020

jcape Apr 16, 2020

jcape Apr 16, 2020

jcape Apr 16, 2020

	let output_commitment: Commitment = Commitment::try_from(output_commitment)?;
	let output_commitment = Commitment::try_from(output_commitment)?;

	let mut decompressed_output_commitments: Vec<Commitment> = Vec::new();
	let mut decompressed_output_commitments = Vec::<Commitment>::with_capacity(output_commitments.len());

Amount and signatures use CompressedRistretto #17

Amount and signatures use CompressedRistretto #17

Conversation

mfaulk commented Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

mfaulk Apr 16, 2020 • edited Loading

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

xoloki left a comment

Choose a reason for hiding this comment

jcape 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

mfaulk commented Apr 16, 2020 •

edited

Loading

mfaulk Apr 16, 2020 •

edited

Loading