-
Notifications
You must be signed in to change notification settings - Fork 39
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
added multiple miller-loops, probabilistic and batch verification #9
Conversation
@daira : @ebfull thought that you might want to take a look at the pdf |
"One can show that a set of valid proofs will always be accepted, and a set of proof of which at least one is non-valid, will be accepted with probability at most 1/s." Intuitively I would have expected this probability to also depend on the batch size. Why does it not? |
Well..first of all where you do pay for the batch size is in the time, and number of random bits you need for the test, which grows linearly with the size of the batch. |
I'm opposed to replacing the SNARK verifier in production MagicBean. However, I'm +1 on adding a redundant (ideally parallel, but serial would do for now) SNARK verifier on top of the current verifier. This is because a bug in the SNARK verifier could have big consequences. There are only a few ways that someone could counterfeit Zcash, i.e. give a payment to an innocent person, so that if the innocent person then spends the money to another innocent person the second innocent person accepts it:
These are listed in increasing order of likelihood in my ever-so-humble opinion. I think 4 is a lot more likely than 3, which is a lot more likely than 2, etc. So, I oppose making any changes to the SNARK verifier unless we have a very good argument that the resulting thing is safer against attack number 4. Redundantly executing both old and new implementations of the verifier would be such an argument. (But we'd have to be very careful about the implementation of the composition itself to avoid that a bug in that accidentally introduces the flaw we're trying to prevent!) It would also be a good next-step toward a future improvement to the performance of the verification, by getting a new implementation into production and tested. (Note: this kind of testing-in-production does not provide evidence that the new version is immune to malicious attack, but it does provide evidence that the new version doesn't have bugs in the non-malicious case.) To be clear, I am -10 on a patch which replaces or changes the SNARK verifier, but +1 on a patch that adds a second, redundant SNARK verifier (which slows down performance for now but increases safety). The code that implements the redundancy/composition/serial-or-parallel computation would need to be unit-tested and reviewed carefully to make sure it doesn't introduce any vulnerability. P.S. I don't think performance of the SNARK verifier is currently a performance problem. Even if we add this new implementation in serial mode and it slows the verifier down so that it takes about 175% as long as the old verifier, I still don't think it would be a performance problem for now. P.P.S. I also usually oppose "options" that we tell users to become aware of or to make a choice about. The default behavior for users who are unaware of options or who make an ill-informed choice must be security over performance in this case, especially because there is a system-wide effect, such that each user's individual choice could have a consequence on all other users of the currency. |
I agree with @zookozcash 's conservative approach, with this exception: We should consider using batch-only verification if and when it enables a use case that is otherwise infeasible. And even then, strive to keep the consensus based on the most conservative (unbatched) verifier. (This is analogous to SPV clients: worse security than full nodes, but they enable new use cases, the damage is bounded, and consensus is not affected.) |
I really like your perspective in #9 (comment), @tromer. Here's another thought: someone could run a service which tests all transactions (including old historical ones from the early days of the blockchain and fresh ones) with all verifiers (including old verifiers that have long-since been phased out of service and new experimental verifiers). |
The test could also include fuzzing to ensure all verifiers produce the same result. |
@bitcartel : What does fuzzing mean? |
Nitpicking on Zooko's comment:
That attack isn't against the elliptic curve, it's against the target group of the pairing GT, which is an extension field (and it's not brute force). |
@@ -471,6 +471,96 @@ bool r1cs_ppzksnark_affine_verifier_weak_IC(const r1cs_ppzksnark_verification_ke | |||
const r1cs_ppzksnark_primary_input<ppT> &primary_input, | |||
const r1cs_ppzksnark_proof<ppT> &proof); | |||
|
|||
/****Batch and probabilistic verification | |||
* using randomness and the bilinearity of the pairing operation, the Pinocchio verifier can be made more efficient | |||
* with the price of making the verification procedure probabilisitc, introducing a negligible chance of accepting a bad proofs |
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.
"probabilistic", "bad proof"
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.
New sentence here.
enter_block("Preparing 7 ML factors"); | ||
|
||
//computing left input for the first ML factor | ||
// r3Pi_a and vk_A |
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.
These comments are copied from r1cs_ppzksnark_probabilistic_verifier
and might not be appropriate for this function.
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.
fixed
{ | ||
enter_block("Call to r1cs_ppzksnark_batch_verifier_process_vk"); | ||
r1cs_ppzksnark_processed_batch_verification_key<ppT> pvk; | ||
pvk.pair1 = ppT::precompute_G2(vk.alphaA_g2); |
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.
Missing comment "computing the second input for the first ML factor..".
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.
done
//−r 4(vk x + πA + πC) and vk^2_betagamma | ||
pvk.pair5 = ppT::precompute_G2(vk.gamma_beta_g2); | ||
|
||
//computing the second input for the six ML factor |
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.
s/six/sixth/
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.
fixed
//computing left input for the fourth ML factor | ||
// r4Pi_K and vk_gamma | ||
acc.pair4 = acc.pair4 + r_4*proof.g_K; | ||
//computing left input for the fifth ML factor |
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.
Fix indentation and make line spacing consistent.
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.
fixed
|
||
enter_block("Preparing 7 ML factors"); | ||
|
||
//computing left input for the first ML factor |
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.
s/left/first/ for consistency with "second input" in r1cs_ppzksnark_processed_batch_verification_key
. (Same change in r1cs_ppzksnark_batcher
.)
I considered s/second/right/ instead, but "right" is ambiguous with "correct".
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's fine as is, in this context it's clear right is not correct. Also, I'm using first,second,.. for the ML factors
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.
But "second input" is already used in r1cs_ppzksnark_processed_batch_verification_key
, so there is no consistency at present.
I'm with Zooko: -10 on switching verifier, +1 with redundancy. I assume conservative redundancy would be to fail verification if either verifier fails. The simplest composition would be a very small-scoped change to the JoinSplit verification which says something like: We'd be using To exercise the batched-nature of batch verification, it could be applied to blocks of transactions at a time in |
Thanks for going over the code and your comments @daira ! I'll revise according to them tomorrow or Monday |
Please let me know if there are further requests/comments. |
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.
Mostly nits.
@@ -830,8 +831,8 @@ void r1cs_ppzksnark_batcher(const r1cs_ppzksnark_verification_key<ppT> &vk, | |||
//computing left input for the fourth ML factor |
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'd still prefer these to say "first" rather than "left".
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.
(similarly in r1cs_ppzksnark_probabilistic_verifier
).
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 still disagree. Changed "second" to "right" in r1cs_ppzksnark_processed_batch_verification_key
mentioned below
//computing left input for the fifth ML factor | ||
//−r 4(vk x + πA + πC) and vk^2_betagamma | ||
//computing left input for the fifth ML factor | ||
//−r4(vk x + πA + πC) and vk^2_betagamma | ||
acc.pair5 = acc.pair5 + -r_4*(accu + proof.g_A.g + proof.g_C.g); | ||
|
||
//computing left input for the six ML factor |
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.
"sixth"
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.
Fixed
Makefile
Outdated
src/gadgetlib1/constraint_profiling.cpp \ | ||
|
||
src/gadgetlib1/constraint_profiling.cpp | ||
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.
Remove trailing spaces/tabs.
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.
Done
Makefile
Outdated
@@ -109,14 +107,11 @@ DISABLED_EXECUTABLES = \ | |||
src/zk_proof_systems/zksnark/ram_zksnark/tests/test_ram_zksnark | |||
|
|||
EXECUTABLES = \ | |||
src/algebra/fields/tests/test_bigint |
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.
Keep test_bigint
.
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.
Added
alt_bn128_Fq12 alt_bn128_ate_double_miller_loop(const alt_bn128_ate_G1_precomp &prec_P1, | ||
const alt_bn128_ate_G2_precomp &prec_Q1, | ||
const alt_bn128_ate_G1_precomp &prec_P2, | ||
const alt_bn128_ate_G2_precomp &prec_Q2) |
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.
Fix indentation.
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.
Done, I think, not sure how you wanted it
|
||
enter_block("Preparing 7 ML factors"); | ||
|
||
//computing left input for the first ML factor |
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.
But "second input" is already used in r1cs_ppzksnark_processed_batch_verification_key
, so there is no consistency at present.
@@ -19,16 +19,54 @@ | |||
|
|||
using namespace libsnark; | |||
|
|||
//****Ariel code-testing batch verifier****/// |
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.
Remove this comment.
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.
removed
r1cs_ppzksnark_processed_batch_verification_key<ppT> pvk = r1cs_ppzksnark_batch_verifier_process_vk<ppT>(keypair.vk); | ||
|
||
batch_verification_accumulator<ppT> acc; | ||
//const bool test_serialization = true; |
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.
Remove this ^.
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.
removed
} | ||
|
||
|
||
///****End Ariel code ******////// |
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.
Remove this comment.
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.
removed
@@ -37,6 +75,7 @@ int main() | |||
{ | |||
default_r1cs_ppzksnark_pp::init_public_params(); | |||
start_profiling(); | |||
|
|||
test_r1cs_ppzksnark<default_r1cs_ppzksnark_pp>(1000, 100); |
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.
Don't we want to test both the unbatched and batched verification?
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.
removed comment from test of unbatched verifier in line 71 of file
@ebfull wanted to run some additional tests, so don't merge yet.. |
Given that Zcash no longer uses libsnark when validating blocks after Sapling activation, I suggest that we close this and reimplement it for the Rust Groth16 verifier. |
We are no longer maintaining zcash/libsnark; closing all PRs. |
No description provided.