feat: add extended ristretto commitment factory and pedersen generators #99

hansieodendaal · 2022-05-26T13:28:21Z

Added extended ristretto commitment factory for Pedersen commitments making use of the extended Pedersen generators in the Tari Bulletproofs+ library.

Notes:

RISTRETTO_NUMS_POINTS have been updated by sequentially hashing the domain separated Ristretto255 generator point instead of sequentially hashing the generator point only. Domain separated hashing is less likely to result in unintended collisions or reuse elsewhere.
An ExtendedHomomorphicCommitmentFactory trait has been added
ristretto/pedersen has been changed into a module that defines pedersen/commitment_factory and pedersen/extended_commitment_factory
PedersenCommitmentFactory and ExtendedPedersenCommitmentFactory use the same base G and H generator points so that each factory produces the same commitments for their default implementation.
For ExtendedPedersenCommitmentFactory:
- Homomorphism with public key only holds for extended commitments with ExtensionDegree::Zero
- ~~'Ord' and 'PartialOrd' are undefined for extended commitments other than ExtensionDegree::Zero~~
Update: The common interface trait HomomorphicCommitmentFactory has also been implemented for ExtendedPedersenCommitmentFactory so that its default invocation is compatible with all methods of PedersenCommitmentFactory.

The next PR will include the ExtendedRangeProofService trait and BulletProofsPlusService.

Add extended commitment factory Added extended ristretto commitment factory for Pedersen commitments making use of the extended Pedersen generators in the Tari Bulletproofs+ library.

Cargo.toml

AaronFeickert · 2022-05-26T15:30:12Z

src/ristretto/constants.rs

+            let mut data = b"TARI CRYPTO - ".to_vec();
+            data.append(&mut i.to_le_bytes().to_vec());
+            data.append(&mut val.to_vec());


Is there a particular reason to bind the base point to these generators? They're still NUMS if you simply use an indexed hash. I also recommend using a more descriptive domain separator that makes the intent of the generators more clear.

Depending on efficiency, you could also use Blake2b, which has specific safe functionality for domain separation (the persona parameter) and index-type functionality (the salt parameter, or simply passing the index as fixed-encoding input).

Updated as suggested, although I did not change the hash function as hashing performance is not an issue for the test case that generates the test vector for the NUMS constants.

AaronFeickert · 2022-05-26T15:33:57Z

src/ristretto/pedersen/extended_commitment_factory.rs

+    fn zero(&self) -> PedersenCommitment {
+        let extension_degree = self.0.extension_degree as usize;
+        let zero = vec![Scalar::zero(); extension_degree + 1];
+        let mut points = Vec::with_capacity(extension_degree + 1);
+        points.push(self.0.h_base);
+        points.append(&mut self.0.g_base_vec.clone());
+        let c = RistrettoPoint::multiscalar_mul(&zero, &self.0.g_base_vec);
+        HomomorphicCommitment(RistrettoPublicKey::new_from_pk(c))
+    }


This must always return the group identity, so there's no need to compute this multiscalar multiplication.

Updated and added a specific test case for it

AaronFeickert

It may be useful to add a test that asserts the NUMS points are unique and all non-zero, to catch the case where the hashing implementation has failed catastrophically. This isn't sufficient to guarantee the generators are sufficiently independent, but the failure of such a test would immediately imply commitments are trivially non-binding.

src/ristretto/pedersen/extended_commitment_factory.rs

Cargo.toml

CjS77

This is a partial overview review.

Everything is reviewed except extended_commitment_factory.rs which requires a deeper dive, so I'll focus on that in a 2nd pass.

Everything else is fine, except some semantic and nit-picky stuff. See comments

src/ristretto/constants.rs

src/commitment.rs

src/ristretto/pedersen/commitment_factory.rs

src/ristretto/pedersen/mod.rs

src/ristretto/pedersen/extended_commitment_factory.rs

CjS77

Reviewed extended_commitment_factory.rs.

No major issues; the copies in commit are likely detrimental to performance, but I didn't run any benches to determine the extent of it.

src/ristretto/pedersen/extended_commitment_factory.rs

…ent_factory-patch fix: use a slice directly in the extended_commitment_factory

stringhandler

I like most of it, but the abstraction is not 100% correct.

stringhandler · 2022-05-31T13:03:18Z

src/ristretto/pedersen/extended_commitment_factory.rs

+impl ExtendedPedersenCommitmentFactory {
+    /// Create a new Extended Pedersen Ristretto Commitment factory for the required extension degree using
+    /// pre-calculated compressed constants - we only hold references to the static generator points.
+    pub fn new_with_extension_degree(extension_degree: ExtensionDegree) -> Result<Self, CommitmentError> {


I'm not too fond of the use of ExtensionDegree here. The type is in the bulletproofs_plus crate, meaning that you can't use this crate without Bulletproofs, for example, if it were hidden behind a feature flag. PedersonGens is perhaps in the wrong crate, or perhaps we should use a field in the ExtendedPedersonCommitmentFactory gens: Vec<RistrettoPoint>

As we are dependent on

use bulletproofs_plus::{generators::pedersen_gens::ExtensionDegree, PedersenGens};

here and if we need to remove the dependency I think the most elegant solution would be to completely move generators out of bulletproofs_plus into its own crate.

If that solution is acceptable I propose we deal with that as a separate series of PRs.

Good point. Would it be better to rewrite ExtensionDegree as a newtype around u64 rather than an enum, since the code in bp+ always converts it to an int anyway.
You could then add an adapter type in tari_crypto to resolve the dependency issue.

CjS77

LGTM

Add extended range proofs

85ba7b3

Add extended commitment factory Added extended ristretto commitment factory for Pedersen commitments making use of the extended Pedersen generators in the Tari Bulletproofs+ library.

hansieodendaal changed the title ~~feat: add extended range proofs~~ feat: add extended ristretto commitment factory and pedersen generators May 26, 2022

delta1 reviewed May 26, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

fix bulletproofs-plus git reference

45ea1d1

AaronFeickert requested changes May 26, 2022

View reviewed changes

AaronFeickert reviewed May 26, 2022

View reviewed changes

src/ristretto/pedersen/extended_commitment_factory.rs Outdated Show resolved Hide resolved

AaronFeickert reviewed May 26, 2022

View reviewed changes

src/ristretto/pedersen/extended_commitment_factory.rs Outdated Show resolved Hide resolved

review comments

10855e3

stringhandler reviewed May 27, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

review comments

70fa2c3

AaronFeickert previously approved these changes May 27, 2022

View reviewed changes

CjS77 reviewed May 27, 2022

View reviewed changes

src/ristretto/pedersen/extended_commitment_factory.rs Outdated Show resolved Hide resolved

Revert changes to NUMS constants

280f3d4

hansieodendaal dismissed AaronFeickert’s stale review via 280f3d4 May 30, 2022 05:42

This was referenced May 30, 2022

Update commit to accept references tari-project/bulletproofs-plus#9

Closed

Create TariHash API / trait #100

Closed

Generalize Not enough Ristretto NUMS points #101

Open

hansieodendaal added 2 commits May 30, 2022 10:27

review comments

9a6f94e

lints clippy

0c2f632

hansieodendaal changed the title ~~feat: add extended ristretto commitment factory and pedersen generators~~ [wip] feat: add extended ristretto commitment factory and pedersen generators May 30, 2022

therustmonk and others added 3 commits May 31, 2022 11:32

fix: use a slice directly in the extended_commitment_factory

1642053

Merge pull request #1 from DenisKolodin/ho_extended_ristretto_commitm…

c6550ec

…ent_factory-patch fix: use a slice directly in the extended_commitment_factory

Common trait interface proposal

7a0efff

hansieodendaal changed the title ~~[wip] feat: add extended ristretto commitment factory and pedersen generators~~ feat: add extended ristretto commitment factory and pedersen generators May 31, 2022

stringhandler requested changes May 31, 2022

View reviewed changes

CjS77 previously approved these changes May 31, 2022

View reviewed changes

hansieodendaal changed the title ~~feat: add extended ristretto commitment factory and pedersen generators~~ [wip] feat: add extended ristretto commitment factory and pedersen generators Jun 2, 2022

remove bulletproofs_plus dependency

0c7461e

hansieodendaal dismissed CjS77’s stale review via 0c7461e June 3, 2022 03:06

hansieodendaal changed the title ~~[wip] feat: add extended ristretto commitment factory and pedersen generators~~ feat: add extended ristretto commitment factory and pedersen generators Jun 3, 2022

hansieodendaal mentioned this pull request Jun 3, 2022

remove dependency on extension degree hansieodendaal/tari-crypto#2

Closed

hansieodendaal requested review from stringhandler and CjS77 June 3, 2022 03:15

stringhandler approved these changes Jun 3, 2022

View reviewed changes

stringhandler merged commit 9a579f6 into tari-project:main Jun 3, 2022

hansieodendaal deleted the ho_extended_ristretto_commitment_factory branch June 5, 2022 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add extended ristretto commitment factory and pedersen generators #99

feat: add extended ristretto commitment factory and pedersen generators #99

hansieodendaal commented May 26, 2022 •

edited

Loading

AaronFeickert May 26, 2022

hansieodendaal May 27, 2022

AaronFeickert May 26, 2022

hansieodendaal May 27, 2022

AaronFeickert left a comment

CjS77 left a comment

CjS77 left a comment

stringhandler left a comment

stringhandler May 31, 2022

hansieodendaal May 31, 2022

CjS77 Jun 1, 2022

CjS77 left a comment

feat: add extended ristretto commitment factory and pedersen generators #99

feat: add extended ristretto commitment factory and pedersen generators #99

Conversation

hansieodendaal commented May 26, 2022 • edited Loading

AaronFeickert May 26, 2022

Choose a reason for hiding this comment

hansieodendaal May 27, 2022

Choose a reason for hiding this comment

AaronFeickert May 26, 2022

Choose a reason for hiding this comment

hansieodendaal May 27, 2022

Choose a reason for hiding this comment

AaronFeickert left a comment

Choose a reason for hiding this comment

CjS77 left a comment

Choose a reason for hiding this comment

CjS77 left a comment

Choose a reason for hiding this comment

stringhandler left a comment

Choose a reason for hiding this comment

stringhandler May 31, 2022

Choose a reason for hiding this comment

hansieodendaal May 31, 2022

Choose a reason for hiding this comment

CjS77 Jun 1, 2022

Choose a reason for hiding this comment

CjS77 left a comment

Choose a reason for hiding this comment

hansieodendaal commented May 26, 2022 •

edited

Loading