Skip to content

Conversation

@MarcosNicolau
Copy link
Member

@MarcosNicolau MarcosNicolau commented Nov 25, 2025

Description

This prs adds the proving system id as part of the proof commitments. This is to avoid a confusion between leaf hashes (proof commitments) and compression hashes (authenticity paths) when verifying that proofs have been indeed verified by the aggregation program. This also fixes an special case were a proof for one proving system could be miss identified as being verified by another one.

As stated by 3MI audit:

The leaf hash (as computed in e.g. sp1/lib.rs line 12, ref. Finding 9) can be confused for a compression hash (e.g. line 51) when the public input consists of 32 bytes. Since no guarantees on the depth of the tree are made, and no check on the length of the authentication path are performed in the verification of a batch, this leads to internal nodes of the merkle tree being considered as proven/verified program IDs.

Suggestion: Provide clear domain separation between leaf and compression hashes in the
merkle tree.

How to test

  1. Start ethereum package:
make ethereum_package_start
  1. Start batcher:
make batcher_start_ethereum_package
  1. Send SP1 and Risc0 proofs:
make batcher_send_sp1_burst BURST_SIZE=1
make batcher_send_risc0_burst BURST_SIZE=1
  1. Run proof aggregator for SP1 and RISC0:
make proof_aggregator_start AGGREGATOR=sp1
make proof_aggregator_start AGGREGATOR=risc0
  1. Verify the proofs have been aggregated:
make verify_aggregated_proof_sp1 FROM_BLOCK=0
make verify_aggregated_proof_risc0 FROM_BLOCK=0

Note: you might need to wait until the fulu fork gets activated on ethereum package (on epoch = 1), you can inspect that with:

curl -X 'GET' \
  'http://localhost:58801/eth/v1/beacon/states/head/fork' \
  -H 'accept: application/json'

Test L2 example

Follow the instructions here.

Type of change

Please delete options that are not relevant.

  • New feature
  • Bug fix
  • Optimization
  • Refactor

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change crates/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

Copy link
Collaborator

@JuArce JuArce left a comment

Choose a reason for hiding this comment

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

Warning

This PR breaks compatibility with previous version, we should coordinate the upgrade with aggregation mode clients

Base automatically changed from fix/fusaka-blocks-with-alloy to testnet November 26, 2025 14:36
Copy link
Contributor

@MauroToscano MauroToscano left a comment

Choose a reason for hiding this comment

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

  • Add IDs to interface constants
  • Make proving system u16

@MarcosNicolau
Copy link
Member Author

Add IDs to interface constants
Make proving system u16

I've added the proving system as a u16. I don't think having an enum in the function is the right approach, since that would require:

  1. Upgrading the contract when we want a new entry in the enum
  2. Enums are defined as u8s, so if we make the proving systems ids uint16 then we can't really use enums.
  3. We could add as many verifiers as we want (different version of sp1 for example) off-chain without having to change anything in the contract
  4. It can be ambigous to call the function via solidity (one would need to import the interface which isn't very confrotable as it would mean cloning the aligned repo and adding it as a dep in foundry)

I admit enums are nicer for ux, but I feel they are not essential here. The IDs are already documented in comments, and the rust sdk explains them clearly.

@MauroToscano MauroToscano merged commit cb3829a into testnet Dec 1, 2025
6 checks passed
@MauroToscano MauroToscano deleted the feat/proving-system-id-for-leafs-1 branch December 1, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants