Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulletproofs (rangeproofs only) #23

Conversation

apoelstra
Copy link
Contributor

This is #16 without any of the arithmetic circuit stuff.

At this point there are probably still obvious code cleanliness things to be dealt with, but I think it's ready for review. I'd like to get this merged so that we can get the rangeproofs at least into Elements soon.

cc @sipa @real-or-random @jonasnick can a couple of you review this? I'm happy to split it into multple PRs if that makes sense.

* If you need to convert to a format suitable for storage or transmission, use
* secp256k1_pedersen_commitment_serialize and secp256k1_pedersen_commitment_parse.
*
* Furthermore, it is guaranteed to identical signatures will have identical

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) s/to/two/

@real-or-random
Copy link
Collaborator

real-or-random commented May 26, 2018 via email

@apoelstra
Copy link
Contributor Author

It was performance motivated. The prover derives 64 * n_gates bytes of randomness, so there was a measurable performance improvement from using chacha. In the rewinding failure case, deriving randomness is a nontrivial portion of the computation (the whole thing is only a couple microseconds and a SHA2 compression is ~300 ns, so that's 10%).

Interesting observation about AES hardware. In general we haven't done any hardware-specific optimizations in -zkp, though we have assembler implementations of some operations upstream. Another motivation was to keep things simple, and chacha is (a) dead simple and (b) we already had code for it laying around in multiple places.

@real-or-random
Copy link
Collaborator

Okay I see, that makes sense. ChaCha20's software performance is certainly excellent and the way to go if hardware optimizations are not a goal, and they're indeed a doubly-edged sword: If you have AES-NI, AES is super fast and well, if you don't have it, then ChaCha20 is much faster. So ChaCha20 is a choice that does not prefer peers with a specific architecture [1]. And I think that's the right way to go in systems where everybody should be able to participate.

(That's by the way interesting for online protocols such as DiceMix. Everybody needs to wait for the slowest peer every round, so if you want to optimize for a lower wall-clock time, you can even make the argument that it makes sense to optimize for slower machines. Though I don't think that matters a lot for protocols where bandwidth is the bottleneck and not computation. But that's another story.)

[1] https://calomel.org/aesni_ssl_performance.html

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More next week. :)

* import hashlib
* C = EllipticCurve ([F (0), F (7)])
* G_bytes = '0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8'.decode('hex')
* H = C.lift_x(int(hashlib.sha256(G_bytes).hexdigest(),16))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need H = C.lift_x(F(int(hashlib.sha256(G_bytes).hexdigest(),16))) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, this must be a new change in sage. Thanks for the fix!

x10 = LE32(seed32[6]);
x11 = LE32(seed32[7]);
x12 = idx;
x13 = idx >> 32;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This right shift has undefined behavior if idx/size_t has 32 bits (or fewer). Same below.

(I guess this is rather a concern in the other file.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idx is now a uint64_t.

#define LE32(p) (p)
#endif

static void secp256k1_scalar_chacha20(secp256k1_scalar *r1, secp256k1_scalar *r2, const unsigned char *seed, size_t idx) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to the comment below: size_t idx is somewhat strange. The code below is intended to handle foridx values that are longer than 32 bits but then size_t is not the right type to do this consistently on every platform, i.e., the 32-bit and 64-bit machines will behave differently (even if idx >> 32 == 0 on a 32-bit machine).

My suggestion is to use uint32_t here and set x13 = 0 (and accept that we won't need more than 256 GiB of randomness. Another way is simply to use uint64_t.


static void secp256k1_scalar_chacha20(secp256k1_scalar *r1, secp256k1_scalar *r2, const unsigned char *seed, size_t idx) {
size_t n;
size_t over_count = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use size_t for the two counter variables either (because the values do not express memory sizes). But in this case this is probably just personal taste. ;)

@real-or-random
Copy link
Collaborator

Does not compile for me.

src/bench_bulletproof.c:43:5: error: unknown type name ‘secp256k1_bulletproof_circuit’
     secp256k1_bulletproof_circuit **circ;
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/bench_bulletproof.c:44:5: error: unknown type name ‘secp256k1_bulletproof_circuit_assignment’
     secp256k1_bulletproof_circuit_assignment *assn;
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/bench_bulletproof.c:12:
src/bench_bulletproof.c: In function ‘bench_bulletproof_circuit_setup’:
src/bench_bulletproof.c:109:11: warning: implicit declaration of function ‘secp256k1_bulletproof_circuit_prove’; did you mean ‘secp256k1_bulletproof_rangeproof_prove’? [-Wimplicit-function-declaration]
     CHECK(secp256k1_bulletproof_circuit_prove(data->common->ctx, data->common->scratch, data->common->generators, data->circ[0], data->common->proof[0], &data->common->plen, data->assn, NULL, 0, data->common->nonce, &data->common->value_gen[0], NULL, 0) == 1);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@apoelstra
Copy link
Contributor Author

Derp, I forgot to commit the circuit-removal from the benchmarks. Fixed.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know all of this is just moved code but here are a few nits... Feel free to ignore.

/** Initialize a context for usage with Pedersen commitments. */
void secp256k1_pedersen_context_initialize(secp256k1_context* ctx);

/** Generate a pedersen commitment.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/pedersen/Pedersen

Copy link
Contributor Author

@apoelstra apoelstra May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a commit that fixes these typos and renames some things. I'll leave any re-ordering of arguments to future PRs because that'll break Elements.

* In: ctx: pointer to a context object (cannot be NULL)
* blinds: pointer to pointers to 32-byte character arrays for blinding factors. (cannot be NULL)
* n: number of factors pointed to by blinds.
* npositive: how many of the initial factors should be treated with a positive sign.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation of the second column

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found initial initially confusing here. ;)

size_t npositive
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Verify a tally of pedersen commitments
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedersen

* In: ctx: pointer to a context object (cannot be NULL)
* commits: pointer to array of pointers to the commitments. (cannot be NULL if pcnt is non-zero)
* pcnt: number of commitments pointed to by commits.
* ncommits: pointer to array of pointers to the negative commitments. (cannot be NULL if ncnt is non-zero)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the above function, n was the prefix for "number", so I expected ncommits to be the number of commitments, but this function apparently uses a cnt suffix.

* generator_blind: array of asset blinding factors, `r` in the above paragraph
* May not be NULL unless `n_total` is 0.
* n_total: Total size of the above arrays
* n_inputs: How many of the initial array elements represent commitments that
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the prefix is n_ even.

* generator_blind: array of asset blinding factors, `r` in the above paragraph
* May not be NULL unless `n_total` is 0.
* n_total: Total size of the above arrays
* n_inputs: How many of the initial array elements represent commitments that
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In secp256k1_pedersen_commit, we had the positive summands first, then the negative ones. Maybe this makes sense like this, I don't know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not move arguments around unless there's a strong argument, as they're in use already.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to read the callback function in innerproduct_impl but I got lost in too many variables. The long comment is very nice but it's kind of hard to map the symbols in the comment to actual variable names, and all the cached/helper values add to my confusion.

I think a few comments to explain variables and to explain how some code lines relate to the verification formula would be helpful for further review.

secp256k1_scalar_set_b32(sec, data, NULL);
memset(data, 0, 32);
}
/* sec * G + value * G2. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/G2/H

#define SECP256K1_BULLETPROOF_CIRCUIT_VERSION 1

/* Maximum depth of 31 lets us validate an aggregate of 2^25 64-bit proofs */
#define SECP256K1_BULLETPROOF_MAX_DEPTH 60
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

31 or 60?

* Args: ctx: pointer to a context object (cannot be NULL)
* In: blinding_gen: generator that blinding factors will be multiplied by (cannot be NULL)
* n: number of NUMS generators to produce
* precomp_n: for each NUMS generator, plus the blinding factor generator, how many multiples to precompute
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n_precomp?

secp256k1_fe_normalize(&pointx);
secp256k1_fe_get_b32(&out[bitveclen + i*32], &pointx);
if (!secp256k1_fe_is_quad_var(&pt[i].y)) {
out[i/8] |= (1ull << (i % 8));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ull seems overkill

*
*/

/* For the G and H generators, we choose the ith generator with a scalar computed from the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New comment intentional? You may want to wrap formulas in ``.

unsigned char tmp[32] = { 0 };
secp256k1_generator gen;
secp256k1_rfc6979_hmac_sha256_generate(&rng, tmp, 32);
CHECK(secp256k1_generator_generate(ctx, &gen, tmp));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's certainly okay to bail out here but usually we handle (computationally) unreachable code more gracefully.

size_t n;
secp256k1_ge *gens;
secp256k1_ge *blinding_gen;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't secp256k1_bulletproof_generators store precomp_n too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should just drop the precomp stuff for this PR since it isn't actually used (I had hacked it in when testing Jonas' multiexp code but never implemented it in an acceptable way).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped precomp.

size_t n;
secp256k1_ge *gens;
secp256k1_ge *blinding_gen;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explanation of what is stored at what array index would have helped me a lot, including a comment that says that blinding_gen points into gens[]

#include "modules/bulletproofs/rangeproof_impl.h"
#include "modules/bulletproofs/util.h"

secp256k1_bulletproof_generators *secp256k1_bulletproof_generators_create(const secp256k1_context *ctx, const secp256k1_generator *blinding_gen, size_t n, size_t precomp_n) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/secp256k1_bulletproof_generators_create/secp256k1_bulletproof_generators_generate
would be slightly more consistent with the naming in the generators module

size_t nbits,
const secp256k1_generator* value_gen,
const unsigned char* const* extra_commit,
size_t *extra_commit_len
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const size_t *extra_commit_len

@@ -6,6 +6,9 @@ bench_verify
bench_schnorr_verify
bench_recover
bench_internal
bench_generator
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you're adding bench_generator', you could also add the missing bench_whitelist`

@apoelstra apoelstra force-pushed the bulletproofs-rangeonly branch 5 times, most recently from 4706f75 to 8822988 Compare June 6, 2018 00:23
@HaxThePlanet
Copy link

Any news on this making it's way into Elements? Looking forward to it

@apoelstra
Copy link
Contributor Author

@real-or-random I replaced all the comments in the inner-product verification callback function. It should be possible to follow now.

Copy link
Contributor

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some preliminary comments with looking towards usage in Elements.

configure.ac Outdated Show resolved Hide resolved
include/secp256k1_bulletproofs.h Outdated Show resolved Hide resolved
include/secp256k1_bulletproofs.h Outdated Show resolved Hide resolved
include/secp256k1_bulletproofs.h Outdated Show resolved Hide resolved
* Returns a list of generators, or NULL if allocation failed.
* Args: ctx: pointer to a context object (cannot be NULL)
* In: blinding_gen: generator that blinding factors will be multiplied by (cannot be NULL)
* n: number of NUMS generators to produce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any realistic upper-limit as per above constants, or can we go all the way up size_t modulo memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really any upper limit. They take a while to generate, on the order of 1000/sec.

include/secp256k1_bulletproofs.h Outdated Show resolved Hide resolved
include/secp256k1_bulletproofs.h Outdated Show resolved Hide resolved
include/secp256k1_bulletproofs.h Show resolved Hide resolved
* generator_blind: array of asset blinding factors, `r` in the above paragraph
* May not be NULL unless `n_total` is 0.
* n_total: Total size of the above arrays
* n_inputs: How many of the initial array elements represent commitments that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not move arguments around unless there's a strong argument, as they're in use already.

/** Standard secp256k1 generator G */
extern const secp256k1_generator secp256k1_generator_const_g;

/** Alternate secp256k1 generator from Elements Alpha */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really don't need to support Elements Alpha.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful to have a standard second generator though for non-CA applications.

@apoelstra
Copy link
Contributor Author

apoelstra commented Jun 20, 2018

@instagibbs fixed your comments

@yeastplume
Copy link

We haven't been able to get secp_bulletproof_rangeproof_verify_multi working thus far. In our testing, it seems to work fine when passing in a single proof and a single commit but as soon as we pass in multiples (which verify when passing in one-by-one,) it refuses to validate.

Also noticing that none of the tests for verify_multi actually specify a length of more than a single commit. Might be worth adding a test or case or two for this?

…commitments

We now use ecmult_const rather than ecmult_gen, which will slow down
the generation of Pedersen commitments. However as far as I'm aware,
this is never the bottleneck in proof generation.
@apoelstra
Copy link
Contributor Author

Rebased on #35

@real-or-random
Copy link
Collaborator

As pointed out by drexl in IRC, we should make sure to read https://grin-tech.org/audits/jpa-audit-report.html and mimblewimble/secp256k1-zkp#37

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the missing deallocate is the only thing that applies to this PR (but someone else should have a second look).

int overflow;
secp256k1_gej commitj;
secp256k1_scalar_set_b32(&blinds[i], blind[i], &overflow);
if (overflow || secp256k1_scalar_is_zero(&blinds[i])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (overflow || secp256k1_scalar_is_zero(&blinds[i])) {
if (overflow || secp256k1_scalar_is_zero(&blinds[i])) {
secp256k1_scratch_deallocate_frame(scratch);

src/modules/bulletproofs/main_impl.h Show resolved Hide resolved
@real-or-random
Copy link
Collaborator

cc @veorq, just for reference

/* Correct for blinding factors and do the commitments */
CHECK(secp256k1_pedersen_blind_generator_blind_sum(ctx, value, (const unsigned char * const *) generator_blind, pedersen_blind, n_generators, n_inputs));
for (i = 0; i < n_generators; i++) {
CHECK(secp256k1_pedersen_commit(ctx, &commit[i], pedersen_blind[i], value[i], &generator[i], &secp256k1_generator_const_h));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last parameter should be secp256k1_generator_const_g instead of secp256k1_generator_const_h.

Related PR in Grin: mimblewimble/secp256k1-zkp#44

tomtau pushed a commit to crypto-com/secp256k1-zkp that referenced this pull request Jul 9, 2020
@dr-orlovsky
Copy link

@apoelstra any chances of having this rebased and getting the great work and effort be finally merged?

@apoelstra
Copy link
Contributor Author

@dr-orlovsky yes, very good chance in the next week or two :)

@apoelstra
Copy link
Contributor Author

Closing this. Will retry in a more staged fashion.

@apoelstra
Copy link
Contributor Author

Closing in favor of #108. Note that the new proofs are not remotely compatible with the old ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet