Bump plonky2 and zk

[illuzen]

No description provided.

[n13]

Here's my review of PR #509: Bump plonky2 and zk.

---

Summary

This PR does two things:

1. Version bumps — qp-plonky2 from 1.4.0 to 1.4.1, and the qp-wormhole-* / qp-zk-circuits-common family from 1.4.0 to 1.4.2.
2. Build-time diagnostics — adds Poseidon2 hash printing of generated circuit binaries in the pallets/wormhole/build.rs build script, along with two new build dependencies (hex, qp-poseidon-core).

CI Status

• Format check: passed
• Build & Test (macOS + Ubuntu): pending
• Clippy & Doc analysis: pending

I'd hold off merging until CI finishes, especially since the ZK circuit generation is the heaviest part of the build.

---

Feedback

1. Missing PR description

The body is empty. For a version bump of ZK circuit dependencies, it'd be great to know what changed in the upstream crates. Were there bug fixes? Performance improvements? API changes? A one-liner linking to the upstream changelog would be helpful.

2. Misleading commit message

Commit 39f5054 says "skip circuit build unless we are really building", but the actual diff only adds hash-printing diagnostics and a doc comment explaining why circuit generation can't be skipped. The code doesn't skip anything. This will be confusing to anyone reading git log later. The added doc comment is useful though:

//! Note: Circuit generation cannot be skipped for this pallet because the binaries are
//! embedded at compile time via `include_bytes!`.

3. Build dependency cost for diagnostics only

Adding qp-poseidon-core and hex as build-dependencies means they compile for the host target on every build. Both are already available as dev/regular dependencies, but build-deps are separate compilation units. For something that's purely diagnostic output (printed as cargo:warning=), this adds build overhead. Worth considering whether a simpler hash (e.g., a basic checksum or just the file size) would suffice, or whether these diagnostics should be behind a feature flag or env var.

4. cargo:warning= noise

The hash output uses cargo:warning= which prints warnings during every build:

cargo:warning=  common.bin: 12345 bytes, hash: abcdef0123456789
cargo:warning=  verifier.bin: ...

Five extra warning lines on every compile. This is fine for debugging, but could get noisy in CI logs. Consider gating behind an env var (e.g., QP_PRINT_HASHES=1) or printing at a lower verbosity level. Or if this is only intended for the bump verification and not permanent, mention that in the PR.

5. Hash truncation

hex::encode(&hash[..16]) // first 16 bytes for shorter display

Truncating to 16 bytes (128 bits) for display is reasonable, but this is a domain-specific hash (Poseidon2) being used purely for diagnostic fingerprinting. If anyone later tries to compare these hashes against full hashes from another tool, the truncation could cause confusion. Not a blocker, just something to be aware of.

6. The version bump itself looks clean

The Cargo.toml and Cargo.lock changes are straightforward 1:1 version bumps. No new transitive dependencies appear. qp-poseidon and qp-poseidon-core stay at 1.4.0, which is consistent since only plonky2 and wormhole crates were bumped.

---

Verdict

The version bump portion is clean and straightforward. The main asks before merging:

1. Wait for CI (build + clippy) to go green
2. Add a brief PR description noting what changed upstream
3. Fix the misleading commit message on 39f5054 (or squash on merge)
4. Decide whether the hash-printing diagnostics should be permanent or gated