Flexible Aggregator

[illuzen]

Summary

Adds dual-output support and random partitioning to the wormhole multiround flow, plus developer tooling improvements.

Key Changes

Dual Output Random Partitioning

• Each proof can send to two exit accounts with separate amounts
• compute_random_output_assignments() randomly distributes outputs across targets
• Enables enhanced privacy through unpredictable output distribution

New Developer Command

quantus developer build-circuits --circuits-path ../qp-zk-circuits --chain-path ../chain

Builds circuit binaries and copies to CLI/chain directories.

Simplified Verification

• Removed single-proof verify command (use verify-aggregated)
• Added --verify flag to parse-proof for local cryptographic verification

Cleanup

• Removed unused multisend command
• Fixed deprecation warnings in keystore
• Updated dilithium/hdwallet dependencies

Testing

• Multiround flow tested: 2 rounds × 2 proofs, 3 rounds x 3 proofs
• Dual outputs verified on-chain (multiple NativeTransferred events)
• Balance verification passes within tolerance

[n13]

Gemini

Here is a review of the changes:

🛑 Blockers

• Cargo.toml Patches: The [patch] section (lines 21-31) points to local directories (../qp-zk-circuits, ../chain). This will break the build for other developers or CI environments that don't have these repositories checked out in the exact same relative paths. These patches should be removed, and the dependencies should point to git repositories or published crates.

⚠️ Concerns

• Hardcoded Paths in BuildCircuits: In src/cli/mod.rs, the BuildCircuits command defaults to ../qp-zk-circuits and ../chain. While these are just defaults, they imply a specific workspace structure. Consider if this is the intended workflow for all users.
• wormhole.rs Size: src/cli/wormhole.rs is quite large (~137KB). Consider breaking this down into smaller modules (e.g., proofs.rs, commands.rs, utils.rs) to improve maintainability.

✨ Improvements

• Timing in main.rs: Adding execution time to the CLI output is a nice UX touch!
• Wallet Updates: The updates to qp-rusty-crystals-hdwallet and keystore.rs seem to clean up the key derivation logic nicely.

Code-Specific Comments

• src/cli/wormhole.rs:
  • The error handling in generate_proof generally looks good, mapping errors to QuantusError.
  • The use of expect("BytesDigest is always 32 bytes") seems safe given the type guarantees.

Action Items:

1. Remove the [patch] section from Cargo.toml.
2. Verify that the updated qp-rusty-crystals-* dependencies are available/published.

Great work on the integration! Just need to fix the dependency paths to make it merge-ready.

[n13]

Opus Pro 4.6

Here's my review of PR #62. This is a substantial PR that touches the wormhole module, wallet layer, and developer tooling. I'll break it down by severity.

---

PR #62 Review

What This PR Does

• Splits wormhole Generate into Transfer + Generate (decoupling the transfer submission from proof generation)
• Adds Multiround command for multi-round wormhole flow testing with random output partitioning
• Adds ParseProof command for debugging proof files
• Adds BuildCircuits developer command to build and distribute circuit binaries
• Switches to pre-built circuit binaries (new_from_files / from_prebuilt_with_paths) instead of runtime circuit construction
• Dual output support (output_amount_1/output_amount_2, exit_account_1/exit_account_2)
• Updates HD wallet API to new derive_key_from_mnemonic / SensitiveBytes32
• Adds command timing to main.rs

---

Critical Issues

1. [patch] sections should NOT be merged

The Cargo.toml adds patch sections pointing to local filesystem paths:

[patch."https://github.com/Quantus-Network/qp-zk-circuits"]
qp-wormhole-circuit = { path = "../qp-zk-circuits/wormhole/circuit" }
...
[patch.crates-io]
qp-dilithium-crypto = { path = "../chain/primitives/dilithium-crypto" }

This will break builds for anyone who doesn't have sibling repos at those exact paths. This looks like development-only config that should be removed before merge.

2. Balance verification math appears incorrect in run_multiround

The amount parameter is documented as "Amount per transfer" but used as the total amount partitioned across proofs:

let partition_amounts = random_partition(amount, num_proofs, min_per_proof);

So amount is the total sent. But the final verification does:

let total_sent = (num_proofs as u128) * amount;  // Bug: should just be `amount`
let final_round_amount = calculate_round_amount(amount, rounds);
let total_received = (num_proofs as u128) * final_round_amount;  // Bug: same issue

This multiplies by num_proofs again, inflating both expected sent and received amounts. The verification will still "pass" (the ratio stays the same) but the logged numbers will be wrong and the tolerance calculation will be off.

---

DRY Violations (High Priority)

3. generate_proof() and generate_proof_with_dual_output() are ~80% identical

Both functions:

• Parse secrets, block hashes, funding accounts the same way
• Generate unspendable accounts identically
• Build storage keys identically
• Fetch storage proofs identically
• Load provers from pre-built bins identically

The only difference is how PublicCircuitInputs is constructed (single output vs dual output). This should be extracted into a shared helper that builds the PrivateCircuitInputs and storage proof, with only the public inputs differing.

4. verify_aggregated_proof() and verify_aggregated_and_get_events() overlap significantly

Both submit unsigned aggregated proof transactions and watch for status. check_proof_verification_events() also duplicates some event-processing logic. Consider having verify_aggregated_proof call verify_aggregated_and_get_events and discard the events.

5. include_bytes! vs runtime file loading inconsistency

• generate_proof and aggregate_proofs load circuit data at runtime via new_from_files()
• verify_aggregated_and_get_events and parse_proof_file embed circuit data at compile time via include_bytes!("../../generated-bins/...")

This is inconsistent and means the binary must be compiled with generated-bins/ present even if you don't use those commands. Pick one approach.

---

Medium Issues

6. Verify (single proof) command was silently removed

The old code had WormholeCommands::Verify for single proof on-chain verification. The new code only has VerifyAggregated. Was this intentional? If single-proof verification is no longer supported, it should be documented.

7. run_multiround is ~500 lines

This function is very long and handles wallet loading, secret derivation, transfers, proof generation, aggregation, on-chain verification, balance tracking, and cleanup. Breaking it into clearly named phase functions would improve readability and testability.

8. CLI arg description mismatch

/// Amount per transfer in planck (default: 10 DEV)
#[arg(short, long, default_value = "10000000000000")]
amount: u128,

The help says "per transfer" but the code uses it as a total that gets partitioned. This will confuse users.

9. parse_exit_account used for non-exit-accounts

At line ~2001 in the new code, parse_exit_account is called to parse a funding account. Consider renaming to something generic like parse_account_id to avoid confusion.

---

Minor / Nits

10. #[allow(dead_code)] on TransferInfo -- the amount field IS used (current_transfers.iter().map(|t| t.amount)), so this allow may not be needed.

11. parse_transfer_events matches events only by to address. If multiple transfers to the same address appear in one block, it will pick the first one, which could be wrong.

12. No tests for the new functions: format_balance, random_partition, compute_random_output_assignments, calculate_round_amount, derive_wormhole_secret. These are pure functions that would be easy to unit test.

13. The WormholeVerifier import was removed from the top of wormhole.rs but is still used via local use statements in several functions. Fine for correctness, but a bit inconsistent.

---

Summary

Severity	Count	Key Items
Critical	2	Local [patch] in Cargo.toml; balance verification math bug
High (DRY)	3	Duplicate proof generation, duplicate verification, include_bytes inconsistency
Medium	4	Removed Verify command, long function, arg description mismatch, function naming
Minor	4	Dead code allow, event matching, missing tests, import style

The main blockers are the [patch] sections in Cargo.toml (will break CI/other devs) and the substantial code duplication in the proof generation / verification paths. The balance verification bug should also be fixed before merge.

[n13]

I've now read through all the diffs thoroughly. Here's my review of PR #62: Flexible Aggregator.

---

PR #62 Review: Flexible Aggregator

Overview

This is a substantial PR (~5,700 diff lines across 10 files) that introduces dual-output proof support, random output partitioning, a multi-round automated test flow, a developer build-circuits command, and migrates ZK circuit dependencies from git branches to versioned crates. The README gets a comprehensive command reference, and the wallet layer updates to a new HD derivation API.

---

Positive Highlights

• Well-structured multi-round flow -- the run_multiround function is broken into focused helpers (derive_round_secrets, execute_initial_transfers, generate_round_proofs, etc.). Nice decomposition.
• Binary hash verification via AggregationConfig::verify_binary_hashes() is a great safety net to catch stale circuit binaries before expensive proof operations.
• Dry-run mode for multiround is useful for debugging HD derivation paths and partition logic without on-chain transactions.
• Good test coverage retained -- unit tests for helpers, edge cases, fee constraints, and determinism.
• Dependency stabilization -- moving from git branch deps to versioned crates is a significant improvement for reproducibility.
• Excellent README -- the new command reference is thorough and well-organized.

---

Issues & Suggestions

1. DRY Violation: verify_aggregated_proof vs verify_aggregated_and_get_events

These two functions both submit an unsigned verify_aggregated_proof tx, watch for InBestBlock / InFinalizedBlock / Error, and inspect events. The core submission + event-watching loop is duplicated. Consider extracting a shared helper that takes a callback for event processing, or returns a generic result struct that both callers can consume.

2. DRY Violation: InBestBlock / InFinalizedBlock arms are copy-pasted

Within verify_aggregated_proof, the InBestBlock and InFinalizedBlock match arms are nearly identical (same event checking, same error handling). Same pattern in verify_aggregated_and_get_events. Each function has ~40 lines duplicated between the two arms.

3. DRY Violation: verify_binary_hashes repeated pattern

The three if let Some(ref expected) / hash_file / mismatches.push blocks are structurally identical. A small loop would be cleaner:

let checks = [
    ("aggregated_common.bin", &stored_hashes.aggregated_common),
    ("aggregated_verifier.bin", &stored_hashes.aggregated_verifier),
    ("prover.bin", &stored_hashes.prover),
];
for (filename, expected_hash) in checks {
    if let Some(ref expected) = expected_hash {
        if let Some(actual) = hash_file(filename) {
            if expected != &actual {
                mismatches.push(format!("{}...", filename));
            }
        }
    }
}

4. validate_aggregation_params appears to be dead code

The original aggregate_proofs called validate_aggregation_params(proof_files.len(), depth, branching_factor). The new version validates directly against agg_config.num_leaf_proofs. Unless validate_aggregation_params is pub and used externally, it should be removed.

5. Missing tests for new core logic

random_partition and compute_random_output_assignments are non-trivial functions with edge cases (e.g., n=0, n=1, total < min_total, more targets than proof output capacity). Neither has unit tests. Given that these directly affect fund distribution, test coverage here would be valuable.

6. Potential panic in random_partition

parts[idx] = (parts[idx] as i128 + diff) as u128;

If diff is negative and |diff| > parts[idx], this wraps to a very large u128. It's unlikely in practice, but a .max(0) guard or saturating_add approach would be safer for production code handling real funds.

7. File size concern: wormhole.rs is becoming very large

The modified file is approaching ~3,500+ lines. The multiround logic, proof generation, aggregation, and verification are all in one file. Consider splitting into submodules (e.g., wormhole/multiround.rs, wormhole/aggregate.rs, wormhole/verify.rs) to improve navigability.

8. TransferInfo.block_hash initialized as H256::default() then overwritten

In parse_transfer_events, the block hash is set to a zero hash and the caller is responsible for overwriting it. This is error-prone -- a future caller might forget. Consider taking block_hash as a parameter to parse_transfer_events instead.

9. AggregationConfig duplicates upstream struct

The PR acknowledges this in comments ("Must match the CircuitBinsConfig struct in qp-wormhole-aggregator/src/config.rs"). If the upstream struct changes, this will silently desync. Consider re-exporting the config struct from the aggregator crate, or at minimum adding a compile-time or test-time assertion that the deserialized format matches.

10. Breaking changes not gated

The Generate, Verify, and multisend commands are removed. Users upgrading will lose these subcommands. If there are any downstream scripts or docs referencing them, they'll break. The PR description mentions this is intentional, but consider whether a deprecation cycle (warn + redirect) would be warranted for a production CLI.

---

Minor / Nits

• Unused import in original generate_proof: WormholeVerifier was imported in the old code's aggregate_proofs but the new version loads from pre-built bins. If the import is still there, it can be cleaned up.
• log_print! and log_verbose! both called for public inputs count in parse_proof_file (lines 3423-3424 of the diff): log_print! already outputs to the user, the log_verbose! on the next line is redundant.
• format_balance truncates to 2 decimal places. For debugging transfers of very precise amounts, consider showing more precision or making it configurable.

---

Verdict

This is a well-executed feature PR with solid architectural choices (config-driven aggregation, pre-built binaries, HD-derived secrets). The main concerns are around DRY violations in the verification paths, missing tests for the new partitioning logic, and the growing file size of wormhole.rs. None of these are blockers, but addressing them would meaningfully improve maintainability.