Skip to content

Simplify poseidon usage#571

Merged
illuzen merged 5 commits into
mainfrom
illuzen/simpler-poseidon
Jun 1, 2026
Merged

Simplify poseidon usage#571
illuzen merged 5 commits into
mainfrom
illuzen/simpler-poseidon

Conversation

@illuzen
Copy link
Copy Markdown
Contributor

@illuzen illuzen commented May 27, 2026

shouldn't change any hashes, but simplifies Header definition

depends on the new qp-poseidon PR Quantus-Network/qp-poseidon#74


Note

High Risk
Changes block header typing and Poseidon dependency wiring across runtime and consensus-critical qp-header; a wrong hash would fork the chain despite the stability test.

Overview
This PR drops the qp-poseidon crate from the workspace and standardizes on qp-poseidon-core 2.0.2 (workspace Cargo.toml / lockfile), removing that dependency from node, runtime, wormhole, mining-rewards, dilithium-crypto, and related crates.

qp_header::Header is simplified from Header<Number, BlockHashHasher, StateHasher> to Header<Number, Hashing>: header fields and block hash are H256, Poseidon block hashing is implemented via qp_poseidon_core inside qp-header, and the lone type parameter is the state trie / extrinsics root hasher (still BlakeTwo256 at runtime). Runtime and mocks now use Header<BlockNumber, BlakeTwo256> instead of PoseidonHasher + Blake2.

pallet-wormhole no longer depends on ToFelts / qp_poseidon for config bounds (native balance, asset id, transfer count, wormhole account id). Tests use BlakeTwo256 for TestExternalities instead of PoseidonHasher.

A consensus regression guard replaces the old dual-path hash comparison: poseidon_header_hash_is_stable asserts a fixed devnet header hash (b7dbfd39…).

Reviewed by Cursor Bugbot for commit a3cdcab. Bugbot is set up for automated code reviews on this repo. Configure here.

@n13
Copy link
Copy Markdown
Collaborator

n13 commented Jun 1, 2026

Review: Safe to merge ✅

This is a clean refactor, and I verified the consensus-critical claim ("shouldn't change any hashes") empirically rather than trusting it — both the block-header hash and the ZK-tree hashing are byte-identical before and after.

The real risk, and how it was verified

The danger isn't the type signature change — it's that qp-header and pallet-zk-tree move from qp-poseidon-core 1.4.02.0.2, while the proving circuit (qp-zk-circuits-common) stays pinned on 1.4.0. If 2.0.2 hashed even slightly differently, it would fork the chain and silently break every wormhole proof. Confirmed safe three ways:

  1. Header hash, empirically. Running the old poseidon_header_hash_matches_old_path test on main (which uses 1.4.0) prints:

    Old hash: 0xb7dbfd398bcef7d1c91821eb105c52e87dab78e822779bfd0dc7ab132d659a55
    

    This is exactly the value pinned in the new 2.0.2 poseidon_header_hash_is_stable test → identical.

  2. Source diff of the two crate versions. serialization.rs differs only in test code (.expect(&format!()).unwrap_or_else(|| panic!())); lib.rs (the Poseidon permutation + hash_to_bytes) is byte-identical, and both pull identical p3-* 0.3.0 + qp-poseidon-constants 1.1.0. So bytes_to_felts_compact / u64_to_felts / hash_to_bytes (used by pallet-zk-tree) are unchanged → on-chain tree roots still match what the 1.4.0 circuit expects.

  3. The original hash() never used the PoseidonHasher type param for computation — it always called qp_poseidon_core directly. Dropping it can't change the result.

Also clean: no dangling qp_poseidon:: / PoseidonHasher references remain, CI is green across the board, and the wormhole mock's TestExternalities<PoseidonHasher><BlakeTwo256> change is actually a correctness fix (now matches Header::Hashing = BlakeTwo256).

One suggestion (non-blocking)

The new poseidon_header_hash_is_stable test is a great regression guard. The zk-tree hashing has no equivalentpallets/zk-tree/src/tests.rs only checks structural properties (root changes on insert, proofs self-verify), which would pass under any Poseidon version and wouldn't catch a future divergence from the 1.4.0 circuit. Since leaf/node hashing is equally consensus- and bridge-critical, consider adding a pinned-value test there (e.g. assert a known hash_leaf output and a small known tree root). The happy-path proof test (test_verify_aggregated_proof_succeeds_with_valid_state) doesn't cover this gap because it hardcodes the block hash rather than building the tree on-chain.

Minor notes

  • The clippy fixes in scheduler/benchmarking.rs and reversible-transfers are behavior-preserving but slightly out of scope for a "simplify poseidon" PR — fine to leave, just worth noting for a clean changelog.

Nice, tidy cleanup that meaningfully reduces the header's type complexity while provably preserving consensus. 🎉

Copy link
Copy Markdown
Collaborator

@n13 n13 left a comment

Choose a reason for hiding this comment

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

GTG

@illuzen illuzen merged commit a735879 into main Jun 1, 2026
5 checks passed
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