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

feat: FFLONK support for compressor #3359

Merged
merged 249 commits into from
Jan 2, 2025
Merged

feat: FFLONK support for compressor #3359

merged 249 commits into from
Jan 2, 2025

Conversation

Artemka374
Copy link
Contributor

@Artemka374 Artemka374 commented Dec 4, 2024

What ❔

Enables support of FFLONK compression

  • Compressor now can be run with --flonk=true flag to run compressor in FFLONK mode(or by running zkstack prover run --component compressor --mode=fflonk), default mode is still PLONK
  • Final proof is now an enum: it can be either L1BatchProofForL1::Fflonk or L1BatchProofForL1::Plonk containing the proof inside
  • We are now making use out of compression_mode environment variable - FFLONK compressor can be run in 5 modes - 5th one is the most effective, so default value is 5.
  • VK setup data generator is updated to generate setup and verification keys for FFLONK as well.

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

Copy link
Contributor

@EmilLuta EmilLuta left a comment

Choose a reason for hiding this comment

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

Overall looks fine. I reckon the latest changes on ZK Stack API will need a .sh regen (I doubt it's a flaky test).

Then there's the follow-ups we discussed on this:

  • speeding up the generation
  • move compressor to the new processing interface
  • make keystore use env var, rather than compilation flag

As soon as the CI passes, happy to re-review & approve.

core/lib/basic_types/src/protocol_version.rs Show resolved Hide resolved
etc/env/base/contracts.toml Show resolved Hide resolved
prover/Cargo.toml Outdated Show resolved Hide resolved
EmilLuta
EmilLuta previously approved these changes Jan 2, 2025
@Artemka374 Artemka374 enabled auto-merge January 2, 2025 10:50
.github/workflows/ci-prover-e2e.yml Show resolved Hide resolved
.github/workflows/ci-prover-e2e.yml Outdated Show resolved Hide resolved
alexandrst88
alexandrst88 previously approved these changes Jan 2, 2025
@Artemka374 Artemka374 disabled auto-merge January 2, 2025 11:30
@Artemka374 Artemka374 enabled auto-merge January 2, 2025 12:54
@Artemka374 Artemka374 added this pull request to the merge queue Jan 2, 2025
@Artemka374 Artemka374 removed this pull request from the merge queue due to a manual request Jan 2, 2025
@Artemka374 Artemka374 added this pull request to the merge queue Jan 2, 2025
Merged via the queue into main with commit 1a297be Jan 2, 2025
44 checks passed
@Artemka374 Artemka374 deleted the afo/fflonk branch January 2, 2025 14:09
Comment on lines +231 to +233
circuit_encodings = "0.150.19"
circuit_sequencer_api = "0.150.19"
circuit_definitions = "0.150.19"
Copy link
Member

Choose a reason for hiding this comment

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

It is a bad idea to unlock the version. The latest version of crypto/protocol crates must be pinned to the last one; otherwise cargo update can bump the version in lockfile automatically.

Pls return = to all the crates where it was removed.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+=1. My miss.

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.

6 participants