feat: zk proof payload signing and verification [skip-line-limit]#1296
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ECDSA-signed ZK proof plumbing: test-mode signer support is wired into the builder and provider cache; ZK actors are initialized with a concrete backend and a PrivateKeySigner; proofs are signed on creation; verification enforces signed payloads and can emit SignedProofFailed with recovered signer attribution. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Builder
participant ActorSetup as ZK Actor Setup
participant ProofReq as ProofRequestActor
participant ZKBackend as ZK Backend
participant EventBus as Event Bus
participant ProofVerif as ProofVerificationActor
Builder->>ActorSetup: setup_zk_actors(bus, backend, signer)
ActorSetup->>ProofReq: ProofRequestActor::setup(bus, signer)
ProofReq->>ZKBackend: request proof computation
ZKBackend->>ProofReq: ComputeResponse (proof)
alt signer present
ProofReq->>ProofReq: build ProofPayload
ProofReq->>ProofReq: SignedProofPayload::sign(payload, signer)
ProofReq->>EventBus: publish EncryptionKeyCreated { signed_payload: Some(...) }
else
ProofReq->>EventBus: publish EncryptionKeyCreated { signed_payload: None }
end
EventBus->>ProofVerif: EncryptionKeyReceived { signed_payload: ? }
alt signed_payload present
ProofVerif->>ProofVerif: recover_signer(), store PendingVerification
end
ProofVerif->>ZKBackend: request verification
ZKBackend->>ProofVerif: ZkVerificationResponse
alt verification failed and pending signed payload+signer
ProofVerif->>EventBus: SignedProofFailed { e3_id, faulting_node, signed_payload }
else verification succeeded
ProofVerif->>EventBus: EncryptionKeyCreated (confirmed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-prover/src/actors/mod.rs (1)
29-31:⚠️ Potential issue | 🟡 MinorOutdated doc example — missing the new
signerparameter.The example still shows a 2-argument call to
setup_zk_actors, but the function now requires 3 arguments.📝 Proposed fix
-//! // Setup all actors with proper separation of concerns -//! setup_zk_actors(&bus, Some(&backend)); +//! // Setup all actors with proper separation of concerns +//! setup_zk_actors(&bus, Some(&backend), None);
🤖 Fix all issues with AI agents
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 29-49: ProofType's discriminants are currently compiler-assigned
making casts like `self.proof_type as u8` fragile; add a stable u8
representation by annotating the enum `ProofType` with `#[repr(u8)]` and assign
explicit u8 discriminant values to each variant (e.g., T0PkBfv = 0,
T1PkGeneration = 1, ...), preserving existing semantic order; update any places
that rely on numeric values to continue using `self.proof_type as u8` (no other
API changes required) so on-chain proof type identifiers remain stable across
edits.
🧹 Nitpick comments (2)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
438-439: Consider logging when signer retrieval fails.
.ok()silently swallows the error. If a wallet is misconfigured, operators won't know why proofs aren't being signed. A brief log helps with diagnosis.📝 Proposed fix
- let signer = provider_cache.ensure_signer().await.ok(); + let signer = match provider_cache.ensure_signer().await { + Ok(s) => Some(s), + Err(e) => { + info!("No signer available for ZK proof signing (proofs will be unsigned): {e}"); + None + } + };crates/zk-prover/src/actors/proof_verification.rs (1)
194-236: Misleading log whenpendingisNone(no matching request) vs. when signed payload is absent.When
self.pending.remove(&pending_key)returnsNone(e.g., duplicate response or race), the code falls through to theelsebranch at line 229, logging "No signed payload available for party". This conflates two distinct situations: (1) no pending entry existed at all, and (2) a pending entry existed but had no signed payload.Consider distinguishing these cases for better operator diagnostics.
📝 Proposed fix sketch
} else { error!( "T0 proof verification FAILED for party {} - rejecting key: {}", msg.key.party_id, msg.error.unwrap_or_else(|| "unknown error".to_string()) ); - // If we have a signed payload, emit SignedProofFailed for fault attribution - if let Some(PendingVerification { - signed_payload: Some(signed), - recovered_signer: Some(signer), - }) = pending - { - warn!( - "Emitting SignedProofFailed for party {} (signer: {signer})", - msg.key.party_id - ); - if let Err(err) = self.bus.publish(SignedProofFailed { - e3_id: msg.e3_id, - faulting_node: signer, - proof_type: signed.payload.proof_type, - signed_payload: signed, - }) { - error!("Failed to publish SignedProofFailed: {err}"); - } - } else { - warn!( - "No signed payload available for party {} - \ - fault cannot be attributed on-chain", - msg.key.party_id - ); - } + match pending { + Some(PendingVerification { + signed_payload: Some(signed), + recovered_signer: Some(signer), + }) => { + warn!( + "Emitting SignedProofFailed for party {} (signer: {signer})", + msg.key.party_id + ); + if let Err(err) = self.bus.publish(SignedProofFailed { + e3_id: msg.e3_id, + faulting_node: signer, + proof_type: signed.payload.proof_type, + signed_payload: signed, + }) { + error!("Failed to publish SignedProofFailed: {err}"); + } + } + Some(_) => { + warn!( + "No signed payload available for party {} - \ + fault cannot be attributed on-chain", + msg.key.party_id + ); + } + None => { + warn!( + "No pending verification found for party {} - \ + possible duplicate response", + msg.key.party_id + ); + } + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 114-132: The current digest() uses abi_encode_packed with two
adjacent dynamic fields (Bytes::copy_from_slice(&self.proof.data) and
Bytes::copy_from_slice(&self.proof.public_signals)), which is collision-prone;
replace the packed encoding with safe encoding by either switching to standard
ABI encoding (use abi_encode instead of abi_encode_packed) or insert an explicit
fixed-size length separator (e.g., U256::from(self.proof.data.len())) between
the two byte arrays before packing, and update the function digest() to use that
new encoding; also correct the doc comment that currently (incorrectly) claims
"length-prefixed byte arrays" to reflect the chosen safe encoding.
🧹 Nitpick comments (1)
crates/events/src/enclave_event/signed_proof.rs (1)
62-66: Nit: merge the twoDecSharesarms.
T5ShareDecryptionmaps to the sameCircuitName::DecSharesas the T2 variants and could be combined into a single match arm for brevity.♻️ Suggested diff
- ProofType::T2SkShareDecryption | ProofType::T2ESmShareDecryption => { - CircuitName::DecShares - } - ProofType::T5ShareDecryption => CircuitName::DecShares, + ProofType::T2SkShareDecryption + | ProofType::T2ESmShareDecryption + | ProofType::T5ShareDecryption => CircuitName::DecShares,
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/tests/tests/integration.rs`:
- Around line 89-90: The unix-only symlink for `bb` (creating `bb_binary`)
leaves non-unix platforms with a broken backend when `find_bb()` returns Some;
update the block that currently does `std::os::unix::fs::symlink(&bb,
&bb_binary)` to add a `#[cfg(not(unix))]` branch that either copies `bb` to
`bb_binary` (e.g., using `std::fs::copy`) or explicitly fails early (e.g.,
`unimplemented!()` or `panic!()`) so `ZkBackend::new` does not get a
non-existent path; change the code around the `bb`/`bb_binary` creation site and
ensure tests that call `find_bb()` handle the alternate branch.
In `@crates/zk-prover/src/actors/mod.rs`:
- Line 30: The doc example calls setup_zk_actors(&bus, &backend) but the real
function signature requires a signer argument; update the example to call
setup_zk_actors(&bus, &backend, signer) (or a clearly named placeholder like
`&signer`/`signer`) and mention what type/trait the signer should implement if
relevant so the example matches the actual function signature (refer to
setup_zk_actors and the signer parameter in the module).
🧹 Nitpick comments (3)
crates/tests/tests/integration.rs (1)
43-69:find_bb()is duplicated fromzk-prover/tests/local_e2e_tests.rs.This is a near-exact copy of the same helper in the zk-prover test crate (as shown in the relevant code snippets). Consider extracting it into a shared test utility (e.g., in
e3-test-helpers) to avoid maintaining two copies.crates/zk-prover/src/actors/proof_request.rs (1)
90-98:resp.proofis cloned twice — minor allocation waste.Line 91 clones
resp.proofintokey.proof, and line 97 clones it again intoProofPayload. Consider cloning once and sharing the reference.♻️ Proposed fix
let mut key = (*pending.key).clone(); - key.proof = Some(resp.proof.clone()); + let proof = resp.proof.clone(); + key.proof = Some(proof.clone()); // Always sign the proof payload — unsigned proofs are not published let payload = ProofPayload { e3_id: pending.e3_id.clone(), proof_type: ProofType::T0PkBfv, - proof: resp.proof.clone(), + proof, };crates/zk-prover/src/actors/proof_verification.rs (1)
182-231: Pending entries accumulate if theZkActornever responds.If
do_send(line 145) delivers the request but theZkActorcrashes or fails to reply, the(e3_id, party_id)entry remains inself.pendingindefinitely. Under sustained failures, this is an unbounded memory growth vector.Consider adding a TTL/timeout mechanism (e.g., periodic sweep of stale entries) or using
ctx.notify_laterfor timeout handling. This is not critical for an initial implementation but worth tracking.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 91-96: The doc comment for the signed payload is stale: it
references abi.encodePacked(...) but the implementation in digest() uses
.abi_encode() (standard ABI encoding). Update the comment to describe encoding
with abi.encode(chainId, e3Id, proofType, proof, publicSignals) (or "standard
ABI encoding") so it matches the digest() implementation and clearly instructs
on-chain verifiers to use Solidity's abi.encode(...); ensure the doc text around
the E3 identifier, proof type, proof, and publicSignals reflects that exact
encoding.
🧹 Nitpick comments (1)
crates/tests/tests/integration.rs (1)
43-69:find_bbrelies on thewhichcommand, which is not available on all systems.This is test-only code so the impact is limited, but
whichisn't POSIX-guaranteed (it's not a shell builtin everywhere). A more portable alternative would be to checkPATHmanually or usecommand -vviash -c. That said, given the#[cfg(unix)]gate and the fallback path insetup_test_zk_backend, this is fine for now.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 91-96: Doc incorrectly states the on-chain digest is
keccak256(abi.encode(...)) without the EIP-191 personal-message prefix; update
the ProofPayload/ProofPayload doc comment to explicitly state that the signer
uses EIP-191 ("\x19Ethereum Signed Message:\n32") wrapping (as used by
sign_message_sync in the sign method) and that on-chain verifiers must apply the
equivalent personal-message hash (e.g., OpenZeppelin's
MessageHashUtils.toEthSignedMessageHash) before ecrecover; reference the
SignedProofPayload/ProofPayload type and the sign_message_sync signing path so
maintainers can locate and align on-chain verification with the Rust signing
semantics.
🧹 Nitpick comments (1)
crates/events/src/enclave_event/signed_proof.rs (1)
52-67:T5ShareDecryptionmaps toCircuitName::DecShares— same as the T2 decryption variants.This may be intentional (both are decryption circuits), but the T2 and T5 arms on lines 62-65 could be combined into a single match arm for clarity if they genuinely use the same circuit. If T5 is expected to eventually use a different circuit, a comment noting the intentional sharing would help future maintainers.
2 participants
Implements the first part of the signed proof fault attribution model: every ZK proof a node broadcasts is now cryptographically signed with the node's private key,that would enable trustless on chain slashing of nodes that submit invalid proofs.
Closes #1297 #1279
Summary by CodeRabbit
New Features
Behavioral Changes
Tests