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

fix: remove table shifts #10814

Merged
merged 5 commits into from
Dec 18, 2024
Merged

fix: remove table shifts #10814

merged 5 commits into from
Dec 18, 2024

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Dec 17, 2024

Table shifts have been obsolete since we moved to a log derivative lookup argument and the table polynomials were still incorrectly considered part of the to_be_shifted polynomials set. This PR addresses the issue and, in turn, the proof size because smaller by 4 frs. Additionally, this brings some flavor simplifications.

@@ -807,12 +807,12 @@ template <typename Curve_> class IPA {
Fr challenge_poly_eval = 1;
Fr r_pow = r;
if (uint32_t(log_poly_length.get_value()) > CONST_ECCVM_LOG_N) {
throw_or_abort("IPA log_poly_length is too large");
throw_or_abort("IPA log_poly_length is too large: " + std::to_string(uint32_t(log_poly_length.get_value())));
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 forgot to update a constant and this was throwing so i figured it's useful for the throw to display the problematic size

Copy link
Contributor

Changes to circuit sizes

Generated at commit: 0f745d3fdd2b1508659252d6a2830cb79270fe08, compared to commit: a3fba8442fdd62f429054c3367984fd4206bbbeb

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public -6 ✅ -0.00% -37,029 ✅ -0.29%
rollup_base_private 0 ➖ 0.00% -37,030 ✅ -0.33%
rollup_block_root 0 ➖ 0.00% -111,087 ✅ -2.52%
rollup_block_merge 0 ➖ 0.00% -74,057 ✅ -3.75%
rollup_root 0 ➖ 0.00% -74,057 ✅ -3.75%
rollup_merge 0 ➖ 0.00% -74,025 ✅ -4.07%
parity_root 0 ➖ 0.00% -148,083 ✅ -4.07%
private_kernel_empty 0 ➖ 0.00% -36,997 ✅ -4.10%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public 3,851,246 (-6) -0.00% 12,617,705 (-37,029) -0.29%
rollup_base_private 3,619,601 (0) 0.00% 11,224,473 (-37,030) -0.33%
rollup_block_root 682,326 (0) 0.00% 4,293,399 (-111,087) -2.52%
rollup_block_merge 32,406 (0) 0.00% 1,900,801 (-74,057) -3.75%
rollup_root 32,390 (0) 0.00% 1,900,787 (-74,057) -3.75%
rollup_merge 1,791 (0) 0.00% 1,744,952 (-74,025) -4.07%
parity_root 4,290 (0) 0.00% 3,487,933 (-148,083) -4.07%
private_kernel_empty 965 (0) 0.00% 865,003 (-36,997) -4.10%

Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

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

LG, thanks!

@@ -243,6 +243,7 @@ void prove_tube(const std::string& output_path)
using Verifier = UltraVerifier_<UltraFlavor>;
Prover tube_prover{ *builder };
auto tube_proof = tube_prover.construct_proof();
info("tube proof length: ", tube_proof.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to keep this?

@@ -237,7 +233,10 @@ class MegaFlavor {
this->return_data_inverses,
};
}
auto get_to_be_shifted() { return DerivedEntities<DataType>::get_to_be_shifted(); }
auto get_to_be_shifted()
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm was this broken before??

Copy link
Contributor Author

@maramihali maramihali Dec 18, 2024

Choose a reason for hiding this comment

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

not broken, we have this weird distinction between wire witnesses and derived witnesses

@@ -183,15 +179,15 @@ class UltraFlavor {
lookup_read_tags) // column 7

auto get_wires() { return RefArray{ w_l, w_r, w_o, w_4 }; };
auto get_to_be_shifted() { return RefArray{ z_perm }; };
auto get_to_be_shifted() { return RefArray{ w_l, w_r, w_o, w_4, z_perm }; };
Copy link
Contributor

Choose a reason for hiding this comment

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

again, weird. Is this somehow equivalent to what we had before? was this jsut not used?

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 wasn't broken, but theto_be_shifted() contribution from WitnessEntities was a weird concatenation between get_wires() and get_to_be_shifted() which only included z_perm

@maramihali maramihali merged commit 469476b into master Dec 18, 2024
72 checks passed
@maramihali maramihali deleted the mm/remove-shifted-table branch December 18, 2024 09:14
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.

2 participants