feat: FFLONK support for compressor #3359
Conversation
# Conflicts: # Cargo.lock # prover/Cargo.lock # zkstack_cli/crates/zkstack/src/commands/chain/init/configs.rs
There was a problem hiding this comment.
Overall looks good.
My only concern -- this PR is massive as it is. It would've been better if we could have 2 PRs -- Igor's changes & then yours. I understand the context and won't push this PR for a split, but might be relevant for future follow-ups.
One concern I have, if anything's broken in either of the implementations, we'll have to revert both.
I'll wait for Crypto changes to VKs then do another review.
Thanks for the work!
| @@ -58,11 +58,15 @@ async fn run_init(args: InitArgs, shell: &Shell) -> anyhow::Result<()> { | |||
| let chain_config = config | |||
| .load_current_chain() | |||
| .context(MSG_CHAIN_NOT_FOUND_ERR)?; | |||
|
|
|||
| if args.update_submodules.is_none() || args.update_submodules == Some(true) { | |||
There was a problem hiding this comment.
What would happen if these changes were packed in a different PR?
There was a problem hiding this comment.
this change is needed to be able to switch between different contracts commits, since when submodules are updated, they will be changed back to the current ones
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct InsertVersionArgsFinal { | ||
| pub snark_wrapper: String, | ||
| pub fflonk_snark_wrapper: String, |
There was a problem hiding this comment.
Do I understand correctly that we'll ask everyone to provide a snark wrapper & fflonk wrapper key always? If so, I wonder if this won't be harder for folks running our system. On the other hand, I don't see a clear technical alternative.
Maybe we should add some docs to explain how they work, why there's 2 and how they interact.
There was a problem hiding this comment.
actually it should work fine if the hash is not provided, I might've missed this option during implementation
|
|
||
| use crate::messages::MSG_SETUP_COMPRESSOR_KEY_PATH_PROMPT; | ||
|
|
||
| #[derive(Debug, Clone, Parser, Default)] | ||
| pub struct CompressorKeysArgs { | ||
| #[clap(long)] | ||
| pub path: Option<String>, | ||
| #[clap(long, default_value = "plonk")] |
There was a problem hiding this comment.
For some reason, I understood that you can use both PLONK and FFLONK at the same time. If that's not the case, doesn't it make more sense to have the CLI accept one or another? (re: my other comment in insert_version.rs)
There was a problem hiding this comment.
you can use them at the same time, I can add option all for this
| CompressorType::Fflonk => { | ||
| let path = args.clone().path.context(MSG_SETUP_KEY_PATH_ERROR)?; | ||
|
|
||
| download_compressor_key( |
There was a problem hiding this comment.
Do we really need both keys here? How does the FFLONK part work? I was expecting just the expanded CRS.
| @@ -120,6 +161,8 @@ mod tests { | |||
|
|
|||
| use super::*; | |||
|
|
|||
| // todo: test is ignored due to serialization issues for now | |||
| #[ignore] | |||
There was a problem hiding this comment.
What's the plan to fix this test?
| @@ -59,6 +59,7 @@ struct Opt { | |||
| /// This function recalculates the commitment in order to check the exact code that | |||
| /// will run, instead of loading `commitments.json` (which also may correct misaligned | |||
| /// information). | |||
| #[allow(dead_code)] // todo: remove after VK issue is resolved | |||
There was a problem hiding this comment.
I reckon we'll get rid of this comment as well.
| ), | ||
| ); | ||
|
|
||
| env::set_var("CRS_FILE", crs_path); |
There was a problem hiding this comment.
Nit: Early return here to get rid of else branch?
…o/fflonk (#3401) "Update generated Prover FRI GPU setup-data keys from branch afo/fflonk"
# Conflicts: # core/lib/protobuf_config/src/proto/config/eth_sender.proto
What ❔
Enables support of FFLONK compression
--flonk=trueflag to run compressor in FFLONK mode(or by runningzkstack prover run --component compressor --mode=fflonk), default mode is still PLONKL1BatchProofForL1::FflonkorL1BatchProofForL1::Plonkcontaining the proof insidecompression_modeenvironment variable - FFLONK compressor can be run in 5 modes - 5th one is the most effective, so default value is 5.Why ❔
Checklist
zk fmtandzk lint.