Switch to new plonky2

[illuzen]

Summary

Adds wormhole pallet with aggregated proof verification, dual-output support, and uses the new separated qp-plonky2-verifier crate for smaller runtime size.

Key Changes

Wormhole Pallet

• transfer_native / transfer_asset - Send to unspendable wormhole accounts
• verify_aggregated_proof - Verify aggregated ZK proofs with multiple exit accounts
• Dual output support: each proof can send to two exit accounts
• Poseidon storage hasher for transfer proof keys
• Nullifier tracking to prevent double-spending

Separated Verifier

• Uses qp-plonky2-verifier instead of full qp-plonky2
• Significantly reduces runtime binary size
• Separate circuit binaries for leaf and aggregated proofs

Runtime Changes

• Added pallet-wormhole and pallet-multisig
• Removed pallet-vesting and pallet-merkle-airdrop
• Updated to latest qp-rusty-crystals-dilithium

Other

• Removed single-proof verification (aggregated only)
• Better error logging for proof verification failures
• QUIC miner support

Testing

• Aggregated proofs verified on-chain with multiple exit accounts
• Dual outputs emit separate NativeTransferred events per account
• Nullifier replay protection tested

[n13]

Gemini Review

Summary of Changes

• Wormhole Pallet:
  • Removed verify_wormhole_proof: Single proof verification is replaced by verify_aggregated_proof.
  • Updated verify_aggregated_proof: Now handles batch verification with improved error logging and security checks.
  • Fee Logic: Fees are calculated based on the total exit amount of the batch.
• Key Generation:
  • SensitiveBytes32: Adopted for handling secrets (seeds, entropy) in generate_node_key and node/src/command.rs, improving memory security.
  • Standardized Derivation: node/src/command.rs now uses a standard derivation path (m/44'/{CHAIN_ID}/{index}'/0/0) and cleaner logic for mnemonic/seed handling.
• Dependencies: Updated qp-plonky2 crates to a more modular structure (core, field, verifier) and bumped versions for dilithium and hdwallet.

Code Review

1. Security Improvements

• Block Hash Validation: The addition of ensure!(block_hash != default_hash, Error::<T>::BlockNotFound); in verify_aggregated_proof is a critical fix. It prevents a malicious prover from using a future block number (which would return a default zero hash) to bypass checks.
• Sensitive Data Handling: Switching to SensitiveBytes32 ensures that sensitive key material (entropy, seeds) is handled more securely, likely with automatic zeroization.

2. Logic & Functionality

• Aggregated Proofs: The logic correctly iterates over aggregated_inputs.account_data to process multiple transfers.
  • Dummy Account Skip: The check exit_account_bytes == [0u8; 32] correctly skips dummy slots used for padding in the circuit.
• Key Derivation: The refactor in node/src/command.rs simplifies the logic by separating the "no derivation" (raw seed) and "derivation" (HD wallet) paths, making the CLI behavior more predictable.

3. Breaking Changes

• verify_wormhole_proof Removal: This is a breaking change for any off-chain workers or clients that were submitting single proofs. They must now submit aggregated proofs via verify_aggregated_proof.

4. Observations / Questions

• Asset Support: The diff for verify_aggregated_proof was partially truncated, so I couldn't fully verify the asset handling logic (handling asset_id vs native tokens) within the loop. Since transfer_asset exists, ensure that verify_aggregated_proof correctly parses and handles asset_id from the aggregated inputs for each transfer in the batch.
• Fee Calculation: Fees are calculated on the total_exit_amount. Ensure this aligns with the economic model (i.e., the fee rate applies to the aggregate volume rather than per-transaction, which is generally fine and more efficient).

Conclusion

The PR looks solid and improves the codebase's security and maintainability. The transition to aggregated proofs is well-handled with the added safety checks.

Status: Approved (assuming asset handling in verify_aggregated_proof is verified).

[n13]

Opus 4.6 Max thinking review of PR #367.

---

PR #367: Switch to new plonky2 - Code Review

Overview

This PR removes single-proof verification in favor of aggregated-only proofs, migrates from HDLattice to derive_key_from_mnemonic, upgrades qp-rusty-crystals-dilithium to 2.1.0, and adds SensitiveBytes32 handling. Generally well-scoped, but there are a few things worth flagging.

---

Critical / High

1. Breaking call_index renumbering

Removing verify_wormhole_proof (index 0) and shifting transfer_native from 1 to 0, transfer_asset from 2 to 1, and verify_aggregated_proof from 3 to 2 will break any in-flight or cached unsigned transactions that encode the old call indices. If this pallet is already deployed on a live testnet, this requires a runtime upgrade coordinated with all miners/submitters. Worth confirming this is intentional and there are no pending extrinsics in the wild.

2. Removal of MinimumTransferAmount check

The minimum transfer check is removed entirely (both from the pallet config and the aggregated proof handler). Without it, a valid proof for dust amounts (e.g., 1 unit) can trigger on-chain minting + event emission + storage writes. Is this intentional? If the circuit enforces a minimum on the prover side, that's fine, but it's worth documenting that the on-chain guard is gone.

(Nik here...): Are dust amounts going to be an issue for the chain? If our fees are volume based I assume they are?!

---

Medium

3. DRY violation in command.rs -- duplicated derivation block

The derive_key_from_mnemonic + early-return logic is copy-pasted almost verbatim across the words branch and the else (new mnemonic) branch:

// In the `words` branch:
let keypair = derive_key_from_mnemonic(&words_phrase, None, &path).map_err(|e| { ... })?;
let dilithium_pair = DilithiumPair::from_seed(&keypair.secret.to_bytes()).map_err(|e| { ... })?;
let account_id = AccountId32::from(dilithium_pair.public());
return Ok(QuantusKeyDetails { ... });

// In the `else` (new mnemonic) branch -- near-identical block:
let keypair = derive_key_from_mnemonic(&new_words, None, &path).map_err(|e| { ... })?;
let dilithium_pair = DilithiumPair::from_seed(&keypair.secret.to_bytes()).map_err(|e| { ... })?;
let account_id = AccountId32::from(dilithium_pair.public());
return Ok(QuantusKeyDetails { ... });

This could be collapsed -- e.g., compute seed_for_pair or the mnemonic string first, then run the derivation logic once at the end.

4. .expect() replacing error propagation in pair.rs

-let signature = keypair.sign(message, None, None);
+let signature = keypair.sign(message, None, None).expect("Signing should not fail");

This turns a recoverable error into a panic. If this is in the runtime signing path, a panic would halt block production. If signing truly cannot fail, a comment explaining why would be reassuring. Otherwise, propagating the error is safer.

5. Event semantic change in wormhole pallet

Previously, ProofVerified was emitted per exit account:

-// Emit event for each exit account
-Self::deposit_event(Event::ProofVerified { exit_amount: *exit_balance });

Now it's emitted once with the total aggregated amount:

+// Emit event for each exit account
+Self::deposit_event(Event::ProofVerified { exit_amount: total_exit_amount });

The comment says "for each exit account" but it's actually emitting once for the entire batch. Any indexers or UIs tracking per-account exit events will break. The comment should also be updated to reflect the new behavior.

---

Low / Nits

6. Debug log left in production path

log::debug!(
    "Fee calculation done: miner_fee={:?}, burn_amount={:?}",
    miner_fee,
    burn_amount
);

This is fine for debugging but should ideally have a target: specified (e.g., target: "wormhole") for consistent log filtering, matching the pattern used elsewhere in the pallet.

7. Block number future check removal is safe but the comment could be tighter

The added comment is good:

+// If we don't check this a malicious prover can set the block_hash to 0
+// and block_number in the future and this check will pass

But it describes the reason the default_hash check is needed, not why the explicit future check was removed. The old TODO: is this check necessary? is answered by the fact that block_hash(future_number) returns the default hash, so the default_hash check subsumes it. A one-liner like "block_hash returns default for future/non-existent blocks, so the check below covers both cases" would be clearer.

8. QUANTUS_DILITHIUM_CHAIN_ID vs hardcoded 189189

The derivation path in command.rs now uses the constant:

let path = format!("m/44'/{QUANTUS_DILITHIUM_CHAIN_ID}/{index}'/0/0", index = wallet_index);

But in pair.rs, the path is hardcoded:

const DEFAULT_PATH: &str = "m/44'/189189'/0'/0'/0'";

These should both use the constant for consistency (and to avoid divergence if the chain ID ever changes).

---

Summary

Severity	Issue	Action
High	Call index renumbering breaks wire format	Confirm intentional for upgrade
High	No more MinimumTransferAmount	Confirm circuit enforces minimum
Medium	Duplicated derivation block in command.rs	Extract shared logic
Medium	.expect() replacing error propagation	Propagate or justify
Medium	Event emitted once, not per-account	Fix comment, confirm with indexers
Low	Debug log missing target:	Add target
Low	Hardcoded 189189 in pair.rs	Use QUANTUS_DILITHIUM_CHAIN_ID

Overall the direction is solid -- removing the single-proof path simplifies the pallet, the error logging improvements are welcome, and the SensitiveBytes32 migration is a nice security improvement. The main things to address are the DRY violation in command.rs and confirming the breaking changes are coordinated.