-
Notifications
You must be signed in to change notification settings - Fork 82
update zkp-based Sumcheck Algorithm-Component #572
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @SaaRaaS-1300, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the zero-knowledge proof (ZKP) examples by integrating two fundamental cryptographic primitives: the Sumcheck protocol and a Matrix Multiplication Check. These additions provide essential building blocks for constructing and verifying more complex ZKP systems, enhancing the educational and practical utility of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
All contributors have signed the CLA ✍️ ✅ |
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.
Code Review
This pull request introduces a sumcheck protocol and a matrix multiplication check, which are valuable additions for zero-knowledge proof examples. The implementation is mostly correct and well-structured. My review focuses on improving code quality, maintainability, and performance. Key suggestions include removing unnecessary dependencies and includes, refactoring duplicated code into helper functions, fixing a bug in random number generation, and optimizing polynomial evaluation and vector operations. The tests are a good start and cover basic honest and fraudulent scenarios.
} | ||
|
||
FieldElem RandFieldElem(const FieldElem& modulus) { | ||
const size_t num_bytes = modulus.BitCount(); |
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.
modulus.BitCount()
returns the number of bits in the modulus, not the number of bytes. Using this value directly as the length for SecureRandBytes
generates far more random data than necessary, which is inefficient. You should calculate the number of bytes by dividing the bit count by 8 and rounding up.
const size_t num_bytes = (modulus.BitCount() + 7) / 8;
|
||
#include "yacl/base/exception.h" | ||
|
||
#include <cmath> |
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.
std::vector<FieldElem> y(log_N); | ||
size_t temp_i = i; | ||
for (size_t j = 0; j < log_N; ++j) { | ||
y[log_N - 1 - j] = FieldElem(temp_i & 1); | ||
temp_i >>= 1; | ||
} |
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.
This logic to convert an integer to its binary representation is duplicated in mat_vec_multiplication
(lines 93-98). To improve maintainability and reduce code duplication, consider extracting this logic into a helper function within an anonymous namespace.
For example:
namespace {
std::vector<FieldElem> IntToBinaryVector(size_t value, size_t num_bits) {
std::vector<FieldElem> bits(num_bits);
for (size_t i = 0; i < num_bits; ++i) {
bits[num_bits - 1 - i] = FieldElem(value & 1);
value >>= 1;
}
return bits;
}
} // namespace
Then you could call it like std::vector<FieldElem> y = IntToBinaryVector(i, log_N);
in both places.
std::vector<FieldElem> point_A = u; | ||
point_A.insert(point_A.end(), y.begin(), y.end()); | ||
std::vector<FieldElem> point_B = y; | ||
point_B.insert(point_B.end(), v.begin(), v.end()); |
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.
Constructing vectors by copying and then inserting can be inefficient due to potential reallocations. It's more performant to reserve the required capacity upfront and then insert the elements.
std::vector<FieldElem> point_A = u; | |
point_A.insert(point_A.end(), y.begin(), y.end()); | |
std::vector<FieldElem> point_B = y; | |
point_B.insert(point_B.end(), v.begin(), v.end()); | |
std::vector<FieldElem> point_A; | |
point_A.reserve(u.size() + y.size()); | |
point_A.insert(point_A.end(), u.begin(), u.end()); | |
point_A.insert(point_A.end(), y.begin(), y.end()); | |
std::vector<FieldElem> point_B; | |
point_B.reserve(y.size() + v.size()); | |
point_B.insert(point_B.end(), y.begin(), y.end()); | |
point_B.insert(point_B.end(), v.begin(), v.end()); |
using yacl::math::MPInt; | ||
using FieldElem = MPInt; |
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.
A using
declaration in a header file can introduce names into the global or enclosing namespace of any file that includes it. It's better to use a single type alias to avoid this.
This can be simplified to using FieldElem = yacl::math::MPInt;
.
using yacl::math::MPInt; | |
using FieldElem = MPInt; | |
using FieldElem = yacl::math::MPInt; |
|
||
#include <cmath> | ||
|
||
#include <numeric> |
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.
FieldElem EvaluateMultilinearTest(const std::vector<FieldElem>& g, | ||
absl::Span<const FieldElem> r, | ||
const FieldElem& modulus) { | ||
size_t num_vars = r.size(); | ||
YACL_ENFORCE(g.size() == (1U << num_vars), | ||
"Polynomial evaluation size mismatch number of variables"); | ||
|
||
std::vector<FieldElem> evals = g; | ||
for (size_t i = 0; i < num_vars; ++i) { | ||
std::vector<FieldElem> next_evals; | ||
size_t current_size = evals.size() / 2; | ||
next_evals.reserve(current_size); | ||
const auto& r_i = r[i]; | ||
|
||
for (size_t j = 0; j < current_size; ++j) { | ||
const auto& eval_at_0 = evals[j]; | ||
const auto& eval_at_1 = evals[j + current_size]; | ||
|
||
FieldElem one(1); | ||
FieldElem one_minus_ri; | ||
FieldElem::SubMod(one, r_i, modulus, &one_minus_ri); | ||
|
||
FieldElem term1, term2, new_eval; | ||
FieldElem::MulMod(eval_at_0, one_minus_ri, modulus, &term1); | ||
FieldElem::MulMod(eval_at_1, r_i, modulus, &term2); | ||
FieldElem::AddMod(term1, term2, modulus, &new_eval); | ||
next_evals.push_back(new_eval); | ||
} | ||
evals = std::move(next_evals); | ||
} | ||
return evals[0]; | ||
} |
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.
examples/zkp/sumcheck/sumcheck.cc
Outdated
|
||
#include "yacl/base/exception.h" | ||
|
||
#include <iostream> |
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.
FieldElem EvaluateUnivariate(const UnivariatePolynomial& poly, | ||
const FieldElem& x, | ||
const FieldElem& modulus) { | ||
FieldElem result(0); | ||
FieldElem x_pow(1); | ||
|
||
for (const auto& coeff : poly) { | ||
FieldElem term; | ||
FieldElem::MulMod(coeff, x_pow, modulus, &term); | ||
FieldElem::AddMod(result, term, modulus, &result); | ||
FieldElem::MulMod(x_pow, x, modulus, &x_pow); | ||
} | ||
return result; | ||
} |
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 current implementation of EvaluateUnivariate
recomputes powers of x
in each iteration. A more efficient approach is to use Horner's method, which avoids explicit power calculations and reduces the number of multiplications.
FieldElem EvaluateUnivariate(const UnivariatePolynomial& poly,
const FieldElem& x,
const FieldElem& modulus) {
FieldElem result(0);
for (auto it = poly.rbegin(); it != poly.rend(); ++it) {
FieldElem::MulMod(result, x, modulus, &result);
FieldElem::AddMod(result, *it, modulus, &result);
}
return result;
}
|
把代码整体移到 yacl/crypto/experimental目录,解决下 CI 的问题
|
I have read the CLA Document and I hereby sign the CLA |
virtual FieldElem evaluate(const std::vector<FieldElem>& point) const = 0; | ||
}; | ||
|
||
FieldElem mat_mat_multiplication( |
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.
统一使用驼峰命名,类似的都修改下
|
||
using yacl::math::MPInt; | ||
using FieldElem = MPInt; | ||
using MultiLinearPolynomial = std::vector<FieldElem>; |
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.
建议换个名字,和 logup 中的类同名
using yacl::math::MPInt; | ||
using FieldElem = MPInt; | ||
|
||
class MultivariatePolynomial { |
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.
MultivariatePolynomial
和MultilinearPolynomial
的区别是什么,sumcheck 中只需要处理MultilinearPolynomial
?
DenseMultilinearPolynomial
这几个 class 用一个就可以表示?
|
||
namespace examples::zkp { | ||
|
||
FieldElem EvaluateMultilinearTest(const std::vector<FieldElem>& g, |
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.
与 logup.cc 中 Evaluate
的实现类似,可以提取出一个共用的函数?
add sumcheck protocol and matrix-multiplication-check in
examples/zkp