Proof aggregation

[larraia]

• Sequential and batch aggregation
• Base proofs erased upon successful aggregation

Not in this PR - to be done:

• Benchmarks. See here.
• Btree strategy. See here

[larraia]

@XuyangSong @vveiln the PR is now ready for review.

[XuyangSong]

It looks good overall. I've left some feedback.

Could you rebase onto the develop branch and resolve the conflicts? We updated some basic structs.

I've reviewed the circuit implementation—it looks nice. I'll run the tests and benchmarks after the rebase.

[XuyangSong]

Thank you for the work. Left a few comments.

Could you fix cargo clippy --workspace --all-targets -- -D warnings warnings, as the current CI does not automatically cover the aggregation feature.

In most cases, using Result instead of Option is better for proper error handling. I'm also handling errors in arm concurrently. Maybe you can wait until the error handling in arm is done.

Do you have a comparison of the two strategies? When should we use batch versus sequential?

Could you add a brief description in the README about proof types, including inner and aggregated proof types, and how to specify them?

[XuyangSong]

Does the inner proof type affect the aggregation proof type?
Default succinct proofs are used in tests. Can other proof types be aggregated?

[larraia]

By inner proof type do you mean the InnerReceipt of the compliance/logic proofs? If so, I would say it does not affect it. RISC0 allows to add a Receipt as an assumption without differentiation.

Aggregation should be agnostic to what is aggregating. This can be empirically confirmed when running benches. (I can't from my machine.)

[XuyangSong]

What inner proof types should we use in practice, and what are the differences between them? Do they vary in performance? I don't fully understand the agnosticism of the inner types—does it mean the outer (aggregation) circuit can verify different inner proof types dynamically?

[larraia]

does it mean the outer (aggregation) circuit can verify different inner proof types dynamically

That's my understanding, yes. Boils down to proving a computational trace for a stark/groth16 verifier(s). Aggregating succinct proofs (single stark) should yield the fastest aggregation.

[larraia]

Added note in the README about this

[XuyangSong]

Suggested change

	if agg_proof.is_some() {
	self.aggregation_proof = bincode::serialize(&agg_proof.unwrap()).ok();
	}
	if self.aggregation_proof.is_some() {
	self.erase_base_proofs();
	Some(())
	} else {
	// Do nothing.
	None
	}
	if agg_proof.is_some() {
	self.aggregation_proof = bincode::serialize(&agg_proof.unwrap()).ok();
	self.erase_base_proofs();
	Some(())
	} else {
	None
	}

[larraia]

I only want to remove the base proofs if the serialized aggregation actually contains some bytes after L133. (There maybe simpler ways of checking self.aggregation_proof is Some after line 133, but I chose clarity)

[larraia]

@XuyangSong :

In most cases, using Result instead of Option is better for proper error handling. I'm also handling errors in arm concurrently. Maybe you can wait until the error handling in arm is done.

This PR was created before error handling exist in the library. The use of Option is to later introduce Result with minimal impact in calling code. So, IMO this is technical debt outside this PR's scope. See this issue.

[larraia]

Could you add a brief description in the README about proof types, including inner and aggregated proof types, and how to specify them?

The README explains how to specify the different aggregation proof types in the Features table. Is that what you mean? Re inner proof types, not sure what you mean, but this comment might be relevant.

[larraia]

Do you have a comparison of the two strategies? When should we use batch versus sequential?

In the README (proof aggregation section) the difference between batch and sequential is mentioned. When to use each, I guess aggregation (proving) performance is the most relevant metric -- we should wait for benches i guess... :_) Let me know otherwise.

[larraia]

This PR also closes #116 (error handling for proof aggregation)

[XuyangSong]

lgtm
We can merge after PA testing.

[XuyangSong]

Do we need the print here? We can consider adding it to the log in the future.

[larraia]

my bad! thanks for catching it

[larraia]

fixed

[agureev]

Works as checked in anoma/pa-evm#336

Feel free to merge

[heueristik]

Is it necessary to introduce this as a dependency if it is only used once? We should minimize dependencies and be wary of supply chain attacks.

[larraia]

Compliance instances are too large to be serialized with serde. What do you suggest instead?

[heueristik]

I would suggest removing serde_with or to double check what the other dependencies it introduces are ("darling_core 0.20.11", "darling_macro 0.20.11").