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

18 update changes from crypto3 zk #19

Merged
merged 15 commits into from
Oct 31, 2023
Merged

Conversation

vo-nil
Copy link
Contributor

@vo-nil vo-nil commented Oct 26, 2023

This PR contains synchronization of current work at crypto3-zk with actor-zk
Additions include algorithmic performance improvements as well as addition of new mask parameter.

ETatuzova and others added 10 commits October 25, 2023 19:32
Adding some missing typedefs, they are used in marshalling.
* Updated the performance test.

* Adding data for perf tests, updating the performance test

* Changing the cpp and input files for merkle tree poseidon examples.
* Add lookup table definition type#190

* Update some types for blueprint compatibility#190

* Lookup table definition changed not to generate lookup table if it's not necessary#190

* Do lookup_table_definition_class abstract to generate tables only if we need them #190

* Add get_rows_number and get_columns_number to lookup table definition

* Some changes after review #190
* Add gate argument multiplication on mask #207

* Padding rows >= usable_rows with random numbers #211

* Derandomize placeholder_placeholder_test #211

* ACTOR_ZK_systems_plonk_plonk_constraint_test updated #211

* Add some includes #211

* Fixed polys values in _etha points is memorized in preprocessor #206

* Move commitment_scheme preprocessed data outside of commitment scheme object #206

* Minor changes in LPC#206
@vo-nil vo-nil requested review from martun and Zerg1996 October 26, 2023 19:31
@vo-nil vo-nil marked this pull request as draft October 26, 2023 19:33
@vo-nil vo-nil marked this pull request as ready for review October 27, 2023 06:14
Copy link
Contributor

@Zerg1996 Zerg1996 left a comment

Choose a reason for hiding this comment

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

I have minor code style notes.

@@ -600,7 +600,7 @@ namespace nil {

auto right = commit_g2<KZG>(params, create_polynom_by_zeros<KZG>(public_key.T));
auto right_side_pairing = crypto3::algebra::pair_reduced<typename KZG::curve_type>(proof, right);

return left_side_pairing == right_side_pairing;
// return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove commended lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 62ff812

@@ -63,35 +65,52 @@ namespace nil {
using poly_type = PolynomialType;
using lpc = LPCScheme;
using eval_storage_type = typename LPCScheme::eval_storage_type;
using preprocessed_data_type = std::map<std::size_t, std::vector<typename field_type::value_type>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

We already declare
using value_type = typename field_type::value_type; so we can set preprocessed_data_type as
using preprocessed_data_type = std::map<std::size_t, std::vector<value_type>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 78fb71c

@@ -129,7 +136,10 @@ namespace nil {
}
this->terms.clear();
for (const auto& it: unique_terms) {
this->terms.emplace_back(it.first.get_vars(), it.second);
if (it.second != assignment_type::zero())
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use one code style for if/else constructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 2c7efa0

for( const auto&[k, table]:lookup_tables ){
// Place table into constant_columns.
for( std::size_t i = 0; i < table->get_table().size(); i++ ){
if(constant_columns[i].size() < start_row + table->get_table()[i].size()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's add new variable
auto end = start_row + table->get_table()[i].size()
Because we use it's expression 4 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in ab70e48

@@ -615,13 +622,17 @@ ACTOR_THREAD_TEST_CASE(permutation_argument_test) {
for (int i = 0; i < argument_size; i++) {
BOOST_CHECK(prover_res.F_dfs[i].evaluate(y) == verifier_res[i]);
for (std::size_t j = 0; j < desc.rows_amount; j++) {
BOOST_CHECK(
prover_res.F_dfs[i].evaluate(preprocessed_public_data.common_data.basic_domain->get_domain_element(j)) == field_type::value_type::zero());
if(prover_res.F_dfs[i].evaluate(preprocessed_public_data.common_data.basic_domain->get_domain_element(j)) != 0){
Copy link
Contributor

Choose a reason for hiding this comment

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

It's something interesting.
We always check inside prover values with field_type::value_type::zero()
Such case means - our equation not always 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.

It seems that some debug checks creeped in.
fixed in c1c33e8

@Zerg1996 Zerg1996 self-requested a review October 30, 2023 18:16
@vo-nil vo-nil merged commit 1c63cc8 into master Oct 31, 2023
1 of 2 checks passed
@vo-nil vo-nil deleted the 18-update-changes-from-crypto3-zk branch October 31, 2023 07:38
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.

4 participants