-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add independent block proof aggregation pipeline #656
base: develop
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,36 @@ | |||
use clap::{Parser, Subcommand}; |
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.
nonblocking: Adding a new file for a single struct seems a bit excessive - could we fold this in?
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.
If we want to support additional modes than just STDIO, then we'll keep the 2nd struct, which will make it similar in essence to the leader and verifier binaries cli
modules. I find it a tad clearer than bloating a single file, but happy to refactor all binaries at once if preferred to go down this path
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.
I find it a tad clearer than bloating a single file
Agreed
AggregatableBlockProof::Block(proof) | ||
} | ||
Ok(Err(e)) => { | ||
anyhow::bail!("Proving task finished with error: {e:?}"); |
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.
String interpolation here is just a worse version of anyhow::Context::context
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.
To be fair this is copy-pasta, but happy to change
Co-Authored-By: Marko Atanasievski <[email protected]>
Adds an
aggregator
binary that can perform:Also includes change in
VerifierData
to support all 3 upper layer proofs by theverifier
binary.Addresses the first part of 2-to-1 aggregation logic (i.e. these are still circuit dependent), next step is to make the aggregator support a set of distinct preprocessed circuits (i.e. type 1 Eth, type 1 PoS, type 2 CDK, etc...) and support arbitrary aggregation of such block proofs through N-to-1 conditional aggregation, see #622.
Only supports
stdio
mode for now, though we could add the others (http
/rpc
) should we want it, easily, but I'd rather wait for it to stabilize first (i.e. once #622 is dealt with) before proceeding to supporting these additional modes.One thing that bothers me is the output to disk, which currently looks like so:
proofs/b7.zkproof
proofs/b7_wrapper.zkproof
-> i.e. only height indicator, no chain IDproofs/block_aggreg.zkproof
-> i.e. no indicator at allAdding the chain ID for the middle layer is easy, but I worry about readability on the last layer, as we may aggregate an arbitrary number of proofs coming from an arbitrary number of distinct chains, hence aiming at adding this kind of information would just blow up the filename length. An alternative could be to mark the file with the hash used as public input (at least to have some unique identifier, though it doesn't carry much meaning on its own..)
closes #387