Skip to content

Use Poseidon2 hash function instead of BLAKE3 during the benchmark proving phase#3152

Open
Fumuran wants to merge 6 commits into
nextfrom
andrew-make-benchmarks-use-poseidon2
Open

Use Poseidon2 hash function instead of BLAKE3 during the benchmark proving phase#3152
Fumuran wants to merge 6 commits into
nextfrom
andrew-make-benchmarks-use-poseidon2

Conversation

@Fumuran

@Fumuran Fumuran commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Overview

Makes Poseidon2 the default proving hash function and improves the transaction benchmarks.

Changes

  • Changed the default LocalTransactionProver hash function from BLAKE3 to Poseidon2.
  • Added ECDSA variants for every signature-authenticated benchmark (single P2ID, two P2ID, create P2ID), in both the cycle-counting and time-counting suites. CLAIM and B2AGG authenticate the bridge account via network auth (no signature), so they keep a single variant.
  • Restructured the time-counting Criterion IDs to encode the signing scheme and proving hash function, e.g. Execute and prove transaction/poseidon2-falcon/single-p2id-note and .../poseidon2/claim-note-l1.

Notes

  • The default change is repo-wide: every LocalTransactionProver::default() caller now proves with Poseidon2.

Benchmark results

Here are the results of the "execute and prove" benchmarks:

Execute and prove transaction/poseidon2-falcon/single-p2id-note
                        time:   [3.4903 s 3.5027 s 3.5163 s]
                        change: [+155.24% +157.66% +159.77%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2-ecdsa/single-p2id-note
                        time:   [892.26 ms 899.61 ms 908.16 ms]
                        change: [+140.83% +144.16% +147.77%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2-falcon/two-p2id-notes
                        time:   [3.5030 s 3.5223 s 3.5446 s]
                        change: [+153.78% +156.21% +158.56%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2-ecdsa/two-p2id-notes
                        time:   [893.59 ms 899.67 ms 906.27 ms]
                        change: [+137.36% +140.44% +143.70%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2/claim-note-l1
                        time:   [1.7647 s 1.7746 s 1.7847 s]
                        change: [+151.39% +155.13% +157.97%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2/claim-note-l2
                        time:   [1.7594 s 1.7695 s 1.7819 s]
                        change: [+149.81% +152.18% +154.59%] (p = 0.00 < 0.05)
                        Performance has regressed.

Execute and prove transaction/poseidon2/b2agg-note
                        time:   [7.0210 s 7.0370 s 7.0538 s]
                        change: [+153.10% +154.96% +156.50%] (p = 0.00 < 0.05)
                        Performance has regressed.

So after switching from Poseidon2 to BLAKE3 average "execute and prove" time increased by ~140%.

Closes: #3129

@Fumuran Fumuran added the pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority label Jun 26, 2026
@Fumuran Fumuran marked this pull request as ready for review June 26, 2026 12:28
@Fumuran Fumuran requested a review from bobbinth June 26, 2026 12:28
@bobbinth

Copy link
Copy Markdown
Contributor
Execute and prove transaction/two P2ID notes
                        time:   [3.3751 s 3.3844 s 3.3952 s]
                        change: [+144.61% +146.35% +147.98%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 30 measurements (6.67%)

Is this using Falcon or ECDSA. If Falcon, let's switch this to ECDSA.

@Al-Kindi-0 Al-Kindi-0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The time-counting proving benchmark only has auth coverage for single-P2ID consume. consume two P2ID notes is still Falcon-only and unlabeled, maybe worth it to add an ECDSA variant too.
Separately, the cycle-counting / bench-tx.json path still has create single P2ID note as Falcon-only and unlabeled.

Comment thread crates/miden-tx/src/prover/mod.rs Outdated
}

/// Creates a new [LocalTransactionProver] instance with the [`Poseidon2`] hash function.
pub fn with_poseidon2() -> Self {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have to make this benchmark choice part of the public prover API?

The benchmark can already choose Poseidon2 with LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)), while Default keeps meaning the normal prover choice.

A new with_poseidon2() helper gives one hash function special API weight without a clear policy for Blake3 or future defaults.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can't use LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)) out of the box: HashFunction is part of the miden-core, but miden-core is not imported as a dependency in that crate.

So choosing between new dependency and verbose LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)) call vs no new dependency and convenience constructor I chose the latter.

In general it seems to me that it will be better to create such constructors for the other hash functions, which will allow us to avoid importing miden-core every time. I can create another constructor for Blake3 so the Poseidon2 won't be special anymore.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably change the default local prover to Poseidon2. Having Blake3 is actually misleading here as it makes the numbers look better than what they actually are.

So, I would update LocalTransactionProver::new() to instantiate the prover with Poseidon2 instead of introducing this new constructor.

This will probably affect proving times downstream cc @igamigo and @WiktorStarczewski.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For now I'll explicitly implement Default for LocalTransactionProver to use Poseidon2, but in general we should update the default implementation of the ProvingOptions to use implicit implementation which is generated by #[derive(Default)] (we can't use it now because it is located in the miden-prover in the miden-vm repo). I'll create a tiny PR in the VM to update this, so after the next release we should be able to remove this explicit implementation and roll-back to the implicit one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine because AFAICT, both client and node use LocalTransactionProver::default() to instantiate the prover. But would be good to double check to make sure we use Poseidon2 as the hash function everywhere. cc @Mirko-von-Leipzig @igamigo @WiktorStarczewski

const BENCH_EXECUTE_TX_CONSUME_CLAIM_L2: &str = "CLAIM note (L2 to Miden)";
const BENCH_EXECUTE_TX_CONSUME_B2AGG: &str = "B2AGG note (bridge-out)";

const BENCH_GROUP_EXECUTE_AND_PROVE: &str = "Execute and prove transaction";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could the prover hash be part of the Criterion ID itself (ideally programmatically) ?

The README says these proving numbers use Poseidon2, but copied output still looks like Execute and prove transaction/CLAIM note (L1 to Miden). A clearer label sequence would keep the group stable and put the variable implementation in the ID, for example:

Execute and prove transaction/poseidon2-falcon/single-p2id-note
Execute and prove transaction/poseidon2-ecdsa/single-p2id-note
Execute and prove transaction/poseidon2-falcon/two-p2id-notes

If Blake3 stays available for comparison, the matching IDs could be blake3-falcon/... and blake3-ecdsa/.... Then the terminal output and Criterion directories say exactly what was measured.

const BENCH_EXECUTE_AND_PROVE_TX_CONSUME_B2AGG: &str =
"Execute and prove transaction which consumes B2AGG note (bridge-out)";
"single P2ID note (ECDSA signing)";
const BENCH_EXECUTE_AND_PROVE_TX_CONSUME_TWO_P2ID: &str = "two P2ID notes";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to the above, could this label include the signing scheme too?

claude and others added 3 commits June 28, 2026 19:33
Addresses PR #3152 review feedback:
- Add ECDSA variants for every signature-authenticated benchmark (two P2ID,
  create P2ID) in both the cycle-counting and time-counting suites; single P2ID
  already had both. CLAIM/B2AGG authenticate the bridge account via network auth
  (no signature), so they keep a single variant.
- Build the time-counting Criterion IDs programmatically so they encode the
  signing scheme and the proving hash function, e.g.
  'Execute and prove transaction/poseidon2-falcon/single-p2id-note' and
  'Execute and prove transaction/poseidon2/claim-note-l1'.
- Regenerate bench-tx.json, refresh README examples, update CHANGELOG and the
  trace-contract test expectations.
Addresses PR #3152 review: instead of a Poseidon2-specific convenience
constructor, change the prover default itself. Remove the with_poseidon2()
and with_blake3() constructors and give LocalTransactionProver a manual
Default impl that uses Poseidon2; the benchmark now proves via
LocalTransactionProver::default().
@Fumuran Fumuran requested review from Al-Kindi-0 and huitseeker June 28, 2026 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benchmarks: use Poseidon2 instead of BLAKE3 in prover

5 participants