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

Unify proof type + link public values #989

Closed
wants to merge 7 commits into from

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Apr 17, 2023

This PR unifies the proof type in fixed_recursive_verifier, so that we deal with the same EvmProof at any level of the tree, and includes check on the public values trie roots when merging proofs. A few design questions / assumptions I wasn't sure on how you were envisioning things:

  • I assumed two proofs to be aggregated to be representing consecutive chain states
  • I assumed root proofs may not represent an entire block, hence the need to check consistency across txn / receipt tries as well in the case where block metadata of two proofs to merge match
  • For checking that block_metadata match, I only considered block_timestamp, although we could extend it to the other fields as well.
  • The above also stores the latest block_metadata when aggregating two proofs. Do we want to keep track of all the block metadatas from past proofs?
  • Given the function definition of prove_block(), the consistency check between trie roots assumes the block proof (if any) is always on the left side.

@Nashtare
Copy link
Collaborator Author

Actually there's an issue in my current block_metadata equality check approach, we'd need at least the leftmost/rightmost pair for an aggregation (whichever the depth) to be able to aggregate further from both sides with another proof.

@Nashtare
Copy link
Collaborator Author

Switched to AggregatedPublicValues type to gather info on leftmost / rightmost block_metadata for an aggregation proof to alleviate issue raised in comment above.

@dlubarov
Copy link
Contributor

dlubarov commented May 3, 2023

I assumed root proofs may not represent an entire block

Sorry for the confusing naming, but I think you mean the "aggregation root" proof (not related to RootCircuitData)? Our intention was that RootCircuitData would typically be for a single txn (although it could be a small chunk of txns in the same EVM proof), while the aggregation root would be for an entire block.

@Nashtare
Copy link
Collaborator Author

Nashtare commented May 4, 2023

Ah ok, thanks for the precision!
So we could lighten the current approach on the BlockCircuitData side as we'd always expect "full" aggregation root proofs, i.e. two blocks being merged together, and check only the connection between state tries. For aggreg proofs, we'd still need the conditional check on the txn/receipt tries as we'd perform several aggregations until having a whole block proven.

I have two related questions:

  • is there any efficiency reason for doing so? I get the will to prove each txn individually, but nothing would prevent a prover to compute block proofs from "intermediate" aggregation root proofs (hear by intermediate that they don't prove an entire EVM block). It seems cheaper given the lighter circuit for BlockCircuitData, although this may be harder to handle in a distributed cluster perhaps?
  • When you mention that RootCircuitData could be for a small chunk of txns, would this be rather a hardcoded parameter (say based on the chosen hardware, we can expect to handle this amount of txn in an acceptable time) or on a dynamic basis, with a central sequencer "analyzing" the incoming set of transactions and mapping it with their "in-circuit" cost?

I was wondering also if this approach I took made sense to you, or if you were thinking of doing things fundamentally differently?

@Nashtare Nashtare force-pushed the public_values branch 3 times, most recently from b6a8abd to 88c9895 Compare May 9, 2023 07:50
@Nashtare
Copy link
Collaborator Author

@dlubarov I've reverted some changes, using now PublicValues as for AllProof, assuming aggregations being done only between transactions within a same block, as I think you were envisioning it?

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