-
Notifications
You must be signed in to change notification settings - Fork 209
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
Bp++: Rangeproof PR #207
base: master
Are you sure you want to change the base?
Bp++: Rangeproof PR #207
Conversation
36329d9
to
b8009af
Compare
#include "scalar.h" | ||
#include "bulletproofs_util.h" | ||
|
||
/* Initialize the transcript with a fix protocol string*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 83b18e2:
lol pls provide a comment explaining how to reproduce these values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, yep. The commit history in this PR is messed up. I was not expecting such a quick review here. Updating the PR to have a cleaner history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can double-check via https://github.com/sanket1729/rust-bitcoin/tree/bp_comm_reproduce cargo test -- --nocapture
0fb6aa7
to
1176e1e
Compare
PR #216 contains two commits that fix compilation and tests. |
a14b562
to
c0da897
Compare
q_inv_pows = (secp256k1_scalar*)secp256k1_scratch_alloc(&ctx->error_callback, scratch, g_offset * sizeof(secp256k1_scalar)); | ||
g_vec_pub_deltas = (secp256k1_scalar*)secp256k1_scratch_alloc(&ctx->error_callback, scratch, g_offset * sizeof(secp256k1_scalar)); | ||
|
||
if ( q_pows == NULL || q_inv_pows == NULL ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( q_pows == NULL || q_inv_pows == NULL ) { | |
if ( q_pows == NULL || q_inv_pows == NULL || g_vec_pub_deltas == NULL) { |
c0da897
to
e0c243f
Compare
ceae9dc
to
4201276
Compare
We can supply additional generators
- The first two bytes can be anything. - Also put the correct index for R_tmp
Liam had previsouly pointed that this should also commit to the generators. Tim mentioned that generators should be exposed in API at all. Porbably a good time to think through these issues
This commit also updates norm argument to use asset_gen instead of G for storing the inner product. This is an artifact of how CTs are used in elements/liquid today.
For compatibility with exisiting Pedersen commitments data structures, it is necessary to have commitments be of the G_a*v + G*gamma where G_a is asset gen and gamma is blinding factors. However, in BP++ design, the blinding values are along H_vec. In order make these compatible with BP++, we make h0 = G Also updates a few test cases to start from |H| = 8.
Commit to digits and multiplicities
Commit to reciprocals of digits as 1/(e + d_i)
Commit to S. Compute l's adaptive to create a zero polynomial
Run the run norm proof argument on the computed C
Combines all four rounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #220 for discussion on the generators API.
* Args: ctx: pointer to a context object | ||
* scratch: pointer to a scratch space | ||
* gens: pointer to the generator set to use, which must have exactly | ||
* `n = max(num_digits, base) + 7` generators, where num_digits is the number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"num_digits is the number." -> "num_digits is floor(log_base(value)) + 1
?
I think you already did these changes, but, for reference, I made a checklist for rebasing #266. |
/** Verifies an Bulletproofs++ 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 at least 2*n_bits generators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bench_bppp
uses 24 generators for both proving/verification; it looks like this should also be updated to reflect floor(log_base(value)) + 1
.
Is there anything which has to be done to get this merged? I mean I am trying to estimate when we may have BPP rangeproofs in secp256k-zkp |
@dr-orlovsky Yes, this needs a lot more review and polishing. As far as I know, no one is working on this right now. |
Thank you for providing the update! So I assume it sounds more like ~year(s) and certainly not weeks/month |
g_len = num_digits > base ? num_digits : base; | ||
h_len = 8; | ||
n_rounds = secp256k1_bppp_log2(g_len > h_len ? g_len : h_len); | ||
return 33 * 4 + 65*n_rounds + 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Bulletproofs++ paper claims on page 4, in Table 1, that a 64-bit range proof consists of 10 group elements and 3 scalars. The calculation here counts 12 group elements and 2 scalars (since n_rounds
is 4 with base 16). What is the explanation for this discrepancy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tevador, this does not implement all the optimizations with early terminations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for "early termination" in the paper gives nothing. Can you point me to the relevant section of the paper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I figured it out from the code. If you do just 3 rounds instead of 4, you'll end up with n_vec
of length 2 and l_vec
of length 1, which is actually more compact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. In the paper, we stop the recursion already when len(l_vec) + len(n_vec) <= 6
, see page 13.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be len(l_vec) + len(n_vec) < 6
. After 2 rounds, we get len(l_vec) = 2
and len(n_vec) = 4
, but the proof can still be reduced by 31 or 32 bytes by performing the 3rd round.
This second PR introduces Bulletproofs++ Rangeproof argument. Builds on top of #205 . The companion protocol is described here:
BP_PP_proofs.pdf
The companion rust code is available at https://github.com/sanket1729/rust-bulletproofs-pp
This includes the core protocol with some optimizations but does not include
Benchmarks show 0.95 ms for verification and ~4 ms prover time for 64 bit rangeproofs