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

Uncompressed Bulletproof Rangeproofs #108

Conversation

apoelstra
Copy link
Contributor

@apoelstra apoelstra commented Nov 24, 2020

This PR makes very little effort to be optimized, preferring ease of review. Future PRs will add

  • rewinding
  • optimizations (combining the two verifier ecmults, reducing scalar ops)
  • inner product argument (i.e. "compressed Bulletproofs")
  • batch verification (after upstream has decided on an API for schnorr/Taproot batching)
  • streaming verification (this will be significantly slower, is it worth it?)
  • precomputation of S commitment in proof

@real-or-random real-or-random self-requested a review November 24, 2020 07:56
@jonasnick jonasnick closed this Jan 12, 2021
@jonasnick jonasnick deleted the branch BlockstreamResearch:master January 12, 2021 20:28
@jonasnick
Copy link
Contributor

Sorry this was automatically closed. PR needs to be reopened against the master branch.

@jonasnick jonasnick reopened this Jan 12, 2021
@apoelstra apoelstra changed the base branch from secp256k1-zkp to master January 12, 2021 20:39
@apoelstra apoelstra force-pushed the 2020-11--bulletproofs1-uncompressed branch from 23efce5 to 027adf3 Compare June 15, 2021 22:14
@apoelstra
Copy link
Contributor Author

Rebased

@dr-orlovsky
Copy link

Is the PR just pending reviews?

I will work on code reviewing in the next few days, but I am not expert in bulleproofs at all, so who also can be asked to do the review and get this merged?

@apoelstra
Copy link
Contributor Author

It needs a review from a cryptographer (and it needs to be rebased, which I can do, but I need some commitment from a cryptographer to move forward).

@dr-orlovsky
Copy link

LNP/BP Association - a bitcoin-only non-profit me and Giacomo Zucco are managing - is willing to put a bounty/grant for a high-profile cryptographers to re-view this PR. Do you have anyone in mind who may be interested in that?

@apoelstra
Copy link
Contributor Author

Thanks for the offer! Unfortunately not -- the high-profile cryptographers I know are more in need of time than money these days.

@dr-orlovsky
Copy link

dr-orlovsky commented Jan 13, 2022

We may ask Peter Todd who is working with our LNP/BP Association for some time now - but will his review be sufficient to get this merged?

@dr-orlovsky
Copy link

Yeah, I always say that time->money is a hash function :) One can always get money from their time - but never time from their money.

@dr-orlovsky
Copy link

dr-orlovsky commented Jan 13, 2022

Another person I have in mind who may agree to do the review on a grant-sponsored basis (and with whom we worked on other our projects before) is Rene Pickhardt

@apoelstra
Copy link
Contributor Author

Neither of them would be sufficient. I need somebody who is a maintainer of this library, and/or somebody who is an author of the BPs paper or similar (and who has experience writing crypto code).

@dr-orlovsky
Copy link

@apoelstra reasonable. Where I can get the list of the maintainers of this lib?

@ChristopherA
Copy link

Maybe Mark Friedenbach @maaku could be incentivized to review this PR?

@apoelstra
Copy link
Contributor Author

@dr-orlovsky the maintainers are me, @jonasnick and @real-or-random. All of us have higher priorities right now. Is there a reason you'd like to push forward on this? I'm happy to start things moving again but there's no way that we can do it in the next week or two.

@dr-orlovsky
Copy link

Oh, it was not about having this moving in the next week or two, it is about having this moving in the next months :)

The reason is that we have dependencies on bulletproofs in other LNP/BP Association projects and right now have to use Grin bulletproofs implementation, which, as you said, is diverged from the spec, but the only secp-based bulleptroofs implementation available right now.

@apoelstra apoelstra force-pushed the 2020-11--bulletproofs1-uncompressed branch from a16c9e5 to 7dc6e3f Compare July 12, 2022 20:20
@apoelstra
Copy link
Contributor Author

Rebased. Had to revert a commit which removes secp256k1_scalar_sqr. No other major changes.

Copy link
Contributor

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

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

Concept ACK

What a PR! Overall looks good (though I haven't checked the test code yet).

The main thing I still need to think about is the way you obtained the hash challenges. For example, right now you have y = hash(state), z = hash(y), whereas I could imagine wanting something like y = hash(state || 0), z = hash(state || 1) instead (not sure yet, will look into this more).

General thoughts:

  • Is the tradeoffs between space savings and nonstandard serialization worth it, for amortizing parity data for two points into a single byte?
  • Should we document the correspondence between generator notation in the paper and generators used in this PR somewhere?

BTW If you prefer, I can make the suggested changes and push commits somewhere.


struct secp256k1_bulletproofs_generators {
size_t n;
/* `G_i`, `H_i` generators, `n` each of them which are generated when creating this struct */
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: It seemed kind of ambiguous whether there were n or 2*n total generators. Would something like this make sense?

Suggested change
/* `G_i`, `H_i` generators, `n` each of them which are generated when creating this struct */
/* n total generators; set n = 2*k to get G_i and H_i values for i in [1..k] */

VERIFY_CHECK(ctx != NULL);

ret = (secp256k1_bulletproofs_generators *)checked_malloc(&ctx->error_callback, sizeof(*ret));
if (ret == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to check the return value of checked_malloc in these 4 calls, given that it already does this check internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because after doing the check it may still return NULL.

secp256k1_generator gen;
if (!secp256k1_generator_parse(ctx, &gen, &data[33 * n])) {
free(ret->gens);
free(ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to return NULL 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.

Yes! This is actually a use-after-free. I should add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, it's a use-after-free and a double-free because it's in a loop. Fixed.

ARG_CHECK(data_len != NULL);

memset(data, 0, *data_len);
if (*data_len < 33 * gens->n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we require *data_len be exactly 33 * gens->n since parsing depends on the size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, typically we allow an arbitrarily large buffer and then return the actually-parsed size through the outpointer.

return ret;
}

int secp256k1_bulletproofs_generators_serialize(const secp256k1_context* ctx, secp256k1_bulletproofs_generators* gens, unsigned char* data, size_t *data_len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should gens be declared as const?

/** A function-like macro returning the size, in bytes, of an uncompressed
* Bulletproof proving that a value lies in the range [0, 2^(n_bits) - 1]
*/
#define SECP256K1_BULLETPROOFS_UNCOMPRESSED_SIZE(n_bits) (194UL + (n_bits) * 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: Is it worth mentioning where 194 came from (65 for A, S, 65 for T1, T2, 64 for tau_x, mu)?

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'll leave it be, because this commit isn't mine, and I don't think it's a huge deal, and also this'll change with BP++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I have to edit that commit anyway to address the above comment. Ok.

* gens: pointer to the generator set to use, which must have 2*n_bits generators (cannot be NULL)
* asset_gen: pointer to the asset generator for the CT commitment (cannot be NULL)
* Out: proof: pointer to a byte array to output the proof into
* In/OUt: plen: pointer to the size of the above array; will be set to the actual size of
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: Do we also need to describe the remaining parameters?

/** Produces an uncompressed rangeproof. Returns 1 on success, 0 on failure.
* Args: ctx: pointer to a context object (cannot be NULL)
* gens: pointer to the generator set to use, which must have 2*n_bits generators (cannot be NULL)
* asset_gen: pointer to the asset generator for the CT commitment (cannot be NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the concept of an "asset generator" still relevant in the context of bulletproofs?

If not, would it be safer to create one ourselves, instead of risking that the user passes in an insecure (known discrete log relation with others) generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, BPs don't change anything about how we manipulate assets.

secp256k1_scalar_add(&t1, &t1, &l);
}

/* B = t0 - t1 + t2 = l(-1) dot r(-1) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: Is it worth extracting a helper function that does <l(k), r(k)> since this is done for k = 0, 1, -1?

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'm gonna say no, but I could go either way.

* In: data: data that came from `secp256k1_bulletproofs_generators_serialize`
* data_len: the length of the `data` buffer
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT secp256k1_bulletproofs_generators* secp256k1_bulletproofs_generators_parse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want parsing and serialization in the public API (especially since I don't see it called anywhere outside of tests)? It seems dangerous to trust a serialized list of generators that we didn't create ourselves (there might be some hidden discrete log relation between them), so we might as well always just call create.

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 seems dangerous to trust a serialized list of generators that we didn't create ourselves

Many things in life are dangerous :).

In practice I expect this list to be hardcoded, so yes, we need an API that can just blindly accept them.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL, fair. Maybe we can include a whitelist of hashes for known safe serialized lists of generators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How long would this list be?

Copy link
Contributor

@robot-dreams robot-dreams Jul 22, 2022

Choose a reason for hiding this comment

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

Length 1 :) if you want to use something that's not the canonical list of generators then it's on you (I'm assuming there's some "unsafe mode" flag or something).

BTW This is not a huge deal, just wanted to note a possible foot gun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How many generators are on the list?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking 128

Copy link
Contributor Author

Choose a reason for hiding this comment

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

128 won't be enough for zkps.

@real-or-random real-or-random removed their request for review July 29, 2022 16:21
@apoelstra
Copy link
Contributor Author

Ok @robot-dreams I've gone through all of your comments. I am locally checking that every commit passes valgrind ./tests 1 then will update this branch. I've added fixup commits after every commit I intend to modify; I'll squash them into their parents after you re-review.

@apoelstra apoelstra force-pushed the 2020-11--bulletproofs1-uncompressed branch from 7dc6e3f to 4122783 Compare August 3, 2022 18:10
Copy link
Contributor

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the previous comments! Took one more pass through everything, just a few more comments below.

I'm still somewhat uneasy about how we do Fiat-Shamir (it might be ideal to take a systematic approach similar to Merlin), but I'm fine with declaring that out of scope for this PR.

void run_rangeproof_tests(void) {
int i;
test_api();
test_rangeproof_fixed_vectors();
test_pedersen_commitment_fixed_vector();
for (i = 0; i < count / 2 + 1; i++) {
test_pedersen();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also need to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch.

test_shallue_van_de_woestijne();
test_generator_fixed_vector();
test_generator_api();
test_generator_generate();
test_pedersen_api();
test_pedersen_commitment_fixed_vector();
for (i = 0; i < 5*count; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming, you intended to increase the iterations from count / 2 + 1 to 5*count when you moved this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure. It seems suspicious that I'd bump this deliberately in a commit marked "MOVE ONLY". I'll revert it.

Comment on lines 134 to 135
/** Initialize a context for usage with Pedersen commitments. */
void secp256k1_pedersen_context_initialize(secp256k1_context* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can this be removed, since it isn't part of the move-only commit (it looks like a later commit deletes it anyway)?

(BTW I didn't know about color-moved before, that's a helpful feature.)

Comment on lines 240 to 241
/** Initialize a context for usage with Pedersen commitments. */
void secp256k1_rangeproof_context_initialize(secp256k1_context* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can this be removed, since it isn't part of the move-only commit (it looks like a later commit deletes it anyway)?

configure.ac Outdated
Comment on lines 145 to 146
[enable_module_bulletproofs=$enableval],
[enable_module_bulletproofs=no])
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should this be consistent with the other options?

Suggested change
[enable_module_bulletproofs=$enableval],
[enable_module_bulletproofs=no])
[],
[SECP_SET_DEFAULT([enable_module_bulletproofs], [no], [yes])])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, things changed since these commits were first written :)

/** Verifies an uncompressed rangeproof. Returns 1 on success, 0 on failure.
* Args: ctx: pointer to a context object
* scratch: pointer to a scratch space
* gens: pointer to the generator set to use, which must have 2*n_bits generators
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* gens: pointer to the generator set to use, which must have 2*n_bits generators
* gens: pointer to the generator set to use, which must have at least 2*n_bits generators

@@ -186,6 +185,9 @@ static int secp256k1_bulletproofs_rangeproof_uncompressed_prove_step1_impl(
return 0;
}

/* Generate new blinding factors tau1 and tau2 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should this be moved to the "step 1" commit?

secp256k1_fe_get_b32(&output[33], &ge.x);

/* get challenges y and z, store them in the prover context */
secp256k1_sha256_initialize(&sha256);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense for this to be a tagged hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but let's leave this for a followup PR because there's a bit of boilerplate involved in setting up tagged hashes and it'd be worth getting separate reviews on that.

secp256k1_fe_get_b32(&output[33], &ge.x);

/* get challenge x */
secp256k1_sha256_initialize(&sha256);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense for this to be a tagged hash?

* bytes 64-96: z (hash challenge)
* bytes 96-160: lr_generator data
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* Generators (paper -> implementation):
* g -> asset_gen
* h -> default secp256k1 generator
* *g*, *h* vectors -> gens
*/

@apoelstra
Copy link
Contributor Author

Lol, sure. Rebased to edit the commit message.

@robot-dreams
Copy link
Contributor

ACK 0ab55a5 :)

@apoelstra
Copy link
Contributor Author

@real-or-random are we ok to merge this (once CI passes)? I think it's too heavy to ask for another reviewer.

@sanket1729
Copy link
Member

I have started reviewing this.You can merge this, my comments can be addressed in a followup PR

@real-or-random
Copy link
Collaborator

@real-or-random are we ok to merge this (once CI passes)? I think it's too heavy to ask for another reviewer.

Yep, feel free to merge (or wait for @sanket1729's review).

@apoelstra
Copy link
Contributor Author

Thanks @sanket1729! I'll wait for your review.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

moslty ACK. Just some questions, some nits, and doc fixes. How would rewinding work if need to add data that is more than 32 bytes?

* Returns 1 on success, 0 if the provided array was not large enough
* Args: ctx: pointer to a context object
* gen: pointer to the generator set to be serialized
* In: data: data that came from `secp256k1_bulletproofs_generators_serialize`
Copy link
Member

@sanket1729 sanket1729 Aug 12, 2022

Choose a reason for hiding this comment

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

In 107f5e3:

The documentation here is incorrect. data should Out instead of In and the description is also incorrect

for (i = 0; i < gens->n; i++) {
secp256k1_generator gen;
secp256k1_generator_save(&gen, &gens->gens[i]);
if (!secp256k1_generator_serialize(ctx, &data[33 * i], &gen)) {
Copy link
Member

Choose a reason for hiding this comment

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

In 107f5e3
secp256k1_generator_serialize always returns 1, so you can remove this if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, removed. Interestingly this is the only place that this function is called internal to the codebase.


len = sizeof(gens_ser);
CHECK(secp256k1_bulletproofs_generators_serialize(ctx, gens, gens_ser, &len));
CHECK(memcmp(gens_ser, fixed_first_3, sizeof(fixed_first_3)) == 0);
Copy link
Member

Choose a reason for hiding this comment

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

In 107f5e3:

It would be good to have a roundtrip test. Just to make sure that parse and serialize API are consistent. From the code review it looks that are consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll add one to the API tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, so adding this (indirectly) uncovered a memory leak in the API tests, and (directly) uncovered that our generator-parsing logic doesn't set the n field of the generators struct correctly.

secp256k1_ge sterm;
secp256k1_gej stermj;

secp256k1_scalar_chacha20(&tmp_l, &tmp_r, nonce, i + 2);
Copy link
Member

Choose a reason for hiding this comment

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

In 05e1d6c

nit: Any special reason why this is i+2? Would make sense to go from i+1 as we already used i=0

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, figured it out in a subsequent commit


secp256k1_scalar_chacha20(&tmp_l, &tmp_r, nonce, i + 2);

secp256k1_ecmult_const(&stermj, &gens->gens[2 * i], &tmp_l, 256);
Copy link
Member

Choose a reason for hiding this comment

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

The documentation of this secp256k1_ecmult_const says:

* Here `bits` should be set to the maximum bitlength of the _absolute value_ of `q`, plus
 * one because we internally sometimes add 2 to the number during the WNAF conversion.

I am unsure if this means len(bits(q+1)) or len(q) + 1. Everywhere else in the codebase, it is called with value 256, so I presume the value here is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe this is correct. I remember thinking about this around the time we implemented ecmult_const but I don't remember my exact logic. I think: regardless of what we do to scalars, we can't exceed 256 bits because we'll just wrap around.

const secp256k1_ge *t2p;
const secp256k1_ge *asset_genp;
const secp256k1_ge *commitp;
secp256k1_scalar neg_t;
Copy link
Member

Choose a reason for hiding this comment

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

In 04ce24e:
minor nit: neg_t name is highlighted as a typedef. Perhaps, we can rename it? neg_t_hat?

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 checked with both vim and emacs and neither has the behavior. It's unfortunate that Github is broken in this way but I don't think we should change our code because of it.

const unsigned char* extra_commit,
size_t extra_commit_len
) {
secp256k1_sha256 sha256;
Copy link
Member

Choose a reason for hiding this comment

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

In 05e1d6c:
Trying to understand the scratch space rationale here. It's just 65 bytes? Can't we use the stack for it instead of passing it around? I am sure that we are using more stack space elsewhere in the codebase for creating temp results in ec computations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah ... this is really weird. I'll try to dig through the git history and see what I was thinking here. I actually did not even notice that this scratch was a byte vector instead of a real scratch space.. I will remove the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, the reasoning is that in one place we can use the output array as "scratch space".

Yes, we are using more than this elsewhere, but 65 bytes is 65 bytes.

Anyway I'll remove this since it makes the code more complicated. If we get feedback that we're using too much scratch space then we can go hyperoptimizing like this. But my original motivation was for operation on the Ledger Nano, which can't directly use this library anyway since it has its own crypto chip with its own C library.

}

/* Commit to all input data: min value, pedersen commit, asset generator, extra_commit */
secp256k1_bulletproofs_commit_initial_data(commit, scratch_bytes, n_bits, min_value, commitp, asset_genp, extra_commit, extra_commit_len);
Copy link
Member

Choose a reason for hiding this comment

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

In 04ce24e:

Is it not possible to use scratch from the function input? Or is it simpler to do this way and we don't care that much about stack space for verifier?

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 just removed the scratch parameter entirely (see above comment).

@@ -20,6 +20,22 @@ static void secp256k1_bulletproofs_serialize_points(unsigned char *output, const
secp256k1_fe_get_b32(&output[33], &rpt->x);
}

/* Initializes SHA256 with fixed midstate. This midstate was computed by applying
* SHA256 to SHA256("Bulletproofs/commitment")||SHA256("Bulletproofs/commitment"). */
Copy link
Member

@sanket1729 sanket1729 Aug 14, 2022

Choose a reason for hiding this comment

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

In 9db5944
Lol, I can't reproduce this :P . See what I did to reproduce this in this commit. sanket1729/rust-bitcoin@5b289a9 . Even if this is displayed reverse this is not matching

The tagged hash code should 100% be correct because it computes the values for taproot hashes that we have tested on bitcoin network.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 Sorry I missed this during review

Copy link
Contributor

Choose a reason for hiding this comment

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

I got this:

    sha->s[0] = 0x50b6a879ul;
    sha->s[0] = 0x0d9a7470ul;
    sha->s[0] = 0xb4400e54ul;
    sha->s[0] = 0x32d29ac7ul;
    sha->s[0] = 0xde938408ul;
    sha->s[0] = 0x923fc797ul;
    sha->s[0] = 0x29f973a6ul;
    sha->s[0] = 0xa25e1a1cul;

Copy link
Member

Choose a reason for hiding this comment

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

I get the same. "1c1a5ea2a673f92997c73f92088493dec79ad232540e40b470749a0d79a8b650" which is just displayed reverse

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 agree. thanks for catching this. I'm not sure where my original values came from.

int overflow;

memset(prover_ctx->data, 0, sizeof(prover_ctx->data));
memset(output, 0, 65);
Copy link
Member

Choose a reason for hiding this comment

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

In 05e1d6c:
ultra minor nit:
Doing these memsets seems unnecessary as we are always writing data first before reading. But it's a good habit to follow in general, these are not done in steps 2 and 3.

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'll leave this for a followup. It's a fair bit of work to audit 0-memsets for whether they're no-ops and/or whether they have some hypothetical security justification, and in the end the compiler will just do what it wants to.

@apoelstra apoelstra force-pushed the 2020-11--bulletproofs1-uncompressed branch from 0ab55a5 to ad4c46f Compare August 27, 2022 17:16
@apoelstra
Copy link
Contributor Author

Rebased and addressed all of Sanket's comments.

apoelstra and others added 15 commits August 27, 2022 17:20
…ngeproof module

You can verify this commit with `git diff --color-moved=zebra`
Silence a compiler warning about an unitialized use of a scalar in case
the user tries to provide a 0-length list of commitments.

Also ensures that commitments have normalized field elements when they
are loaded into ges.
This PR adds a convenience macro enabling callers to allocate the
necessary and sufficient space for an uncompressed rangeproof. In
particular, this makes it tractible to avoid pessimistically
over-allocating the maximum possible space an uncompressed
rangeproof might ever occupy.

Also, SECP256K1_BULLETPROOFS_RANGEPROOF_UNCOMPRESSED_MAX_LENGTH_ is
redefined as a short-cut leveraging the new convenience macro. This
both makes clear from where the maximum length is derived, and
should avoid the two macros drifting apart should the expected size
change (e.g., if compression is supported in the future).
@apoelstra apoelstra force-pushed the 2020-11--bulletproofs1-uncompressed branch from ad4c46f to 6463ffb Compare August 27, 2022 17:20
@apoelstra
Copy link
Contributor Author

Rebased again just to sign the commits for github

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 6463ffb. Reviewed the range-diff from my previous review.

@apoelstra
Copy link
Contributor Author

Closing in favor of #205 and #207.

@apoelstra apoelstra closed this Dec 14, 2022
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.

8 participants