feat: add zk verification to authorization state #259
Conversation
This commit introduces a verifying key as optional attribute of the authorization layer. Every deployed program will be bound to an authorization contract, and will optionally be a ZK program. This commit introduces the skeleton for a ZK verification for such case. It adds a proof binary to a Cosm execute message (the other variants are currently unimplemented for ZK), and binds the authorization state to a ZK key for the shielded program. It uses ark-groth16, an industry standard that will support the output proof of most of the zkVM implementations. As tech debt, we might want a separated library that will decouple such decisions from the concrete implementation, leaving all of the involved implementations (zkVM, verifier, msg encoding) decoupled on this crate.
|
we have a script in /devtools to regenerate the schemas when APIs are changed |
WalkthroughThis pull request integrates zero-knowledge proof support across several components. New dependencies are added to the Cargo.toml files, and JSON schema definitions are updated with properties and object types for ZK verification keys and messages. The authorization logic now conditionally verifies proofs in messages, with corresponding error handling for verification failures. Additionally, builder patterns and test structures are extended to support an optional ZK verifying key, and a new module for ZK functionality is introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth as Authorization
participant Msg as ProcessorMessage
participant zk as zk::verify_proof
Client->>Auth: validate_messages(messages)
Auth->>Msg: Iterate over messages
alt Message is CosmwasmExecuteZkMsg
Auth->>zk: verify_zk_proof(zk_vk, proof, inputs)
zk-->>Auth: return true/false
alt Verification fails
Auth->>Client: Return ZKProofVerificationError
else Verification succeeds
Auth->>Msg: Continue normal validation
end
else
Auth->>Msg: Perform standard validation
end
Auth->>Client: Return overall success
sequenceDiagram
participant Dev as Developer
participant Builder as AuthorizationBuilder
participant AuthInfo as AuthorizationInfo
Dev->>Builder: new()
Dev->>Builder: with_zk_vk(VerifyingKey)
Builder->>AuthInfo: build()
AuthInfo-->>Dev: Returns instance with zk_vk field set
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
contracts/authorization/schema/valence-authorization.json (2)
326-337: Validate minimum values for integer format.Defining
"minimum": 0.0alongside"format": "uint8"is acceptable, but it might be clearer to specify"minimum": 0instead of0.0. Otherwise, this change looks fine for representing a verifying key as an optional array of bytes.
2301-2312: Confirm uniform handling of the ZK key field.This addition of
"zk_vk"mirrors its usage elsewhere. Consider adding a brief comment or description in the schema clarifying that it represents a serialized verifying key, making it easier for users to understand.packages/authorization-utils/src/builders.rs (2)
13-13: Import verification key in a dedicated block if many cryptographic imports grow.The new import is straightforward. If cryptographic-related imports scale up, consider grouping them logically for better discoverability.
86-89: Provide documentation for the ZK key builder method.The
with_zk_vkmethod is clear, but consider adding a short doc comment explaining how to obtain or generate the key. This can reduce user confusion about the expected contents ofVerifyingKey.packages/authorization-utils/src/msg.rs (2)
17-17: Check for feature gating to manage new dependencies.Importing
VerifyingKeyfromzkis straightforward. If some builds don’t require zero-knowledge proofs, you could optionally gate these imports behind a feature flag.
305-305: Preserve proof when resetting just the message.This branch sets only
msgand keepsproofunaltered. Ensure that any re-usage of proofs after changingmsgis tracked and doesn't require a separate setter.packages/authorization-utils/src/zk.rs (2)
5-9: Add schema or doc annotation for VerifyingKey.The
VerifyingKeyalias is plain, but a short doc comment or type-level explanation clarifying expected data format (e.g., uncompressed bytes) could be beneficial for maintainers.
11-15: Validate trait coverage for future ZK expansions.The
ProvingSystemtrait is minimal now. Consider adding deeper constraints (e.g.,Sized) or specialized methods for proving and verifying if the library’s scope grows.contracts/authorization/src/authorization.rs (2)
335-343: Ensure appropriate error handling for ZK proof verification failuresThe current implementation returns a generic
ZKProofVerificationErrorwithout specific context about which message failed verification or why. Consider enhancing the error to include additional context.if let Some(vk) = &self.zk_vk { - for m in messages { + for (index, m) in messages.iter().enumerate() { if !m.verify_zk_proof(vk) { - return Err(ContractError::ZKProofVerificationError); + return Err(ContractError::ZKProofVerificationError { + message_index: index, + }); } } }This change would require updating the
ContractErrorenum inerror.rsto include the message index in theZKProofVerificationErrorvariant.
335-343: Consider adding documentation about the ZK verification logicThe ZK verification logic would benefit from more detailed documentation explaining the purpose and requirements of the verification process.
+// If the authorization has a ZK verifying key, we validate that each message contains a valid +// zero-knowledge proof. This ensures that only messages with valid proofs can be executed +// through this authorization when it's configured for ZK verification. if let Some(vk) = &self.zk_vk { for m in messages { if !m.verify_zk_proof(vk) { return Err(ContractError::ZKProofVerificationError); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(1 hunks)contracts/authorization/schema/valence-authorization.json(5 hunks)contracts/authorization/src/authorization.rs(2 hunks)contracts/authorization/src/error.rs(1 hunks)contracts/processor/schema/valence-processor.json(2 hunks)e2e/examples/polytone.rs(1 hunks)packages/authorization-utils/Cargo.toml(1 hunks)packages/authorization-utils/src/authorization.rs(4 hunks)packages/authorization-utils/src/builders.rs(5 hunks)packages/authorization-utils/src/lib.rs(1 hunks)packages/authorization-utils/src/msg.rs(6 hunks)packages/authorization-utils/src/zk.rs(1 hunks)program-manager/schema/valence-program-manager.json(1 hunks)program-manager/src/tests.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: setup-local-ic
- GitHub Check: build-and-deploy
🔇 Additional comments (22)
packages/authorization-utils/Cargo.toml (1)
9-11: Adding Zero-Knowledge Cryptography DependenciesThe added dependencies from the arkworks ecosystem are appropriate for implementing zero-knowledge proof verification. The ark-bn254 elliptic curve is an industry standard for zkSNARKs, and ark-groth16 provides the Groth16 proving system mentioned in the PR objectives.
packages/authorization-utils/src/lib.rs (1)
8-8: New ZK Module DeclarationThe new
zkmodule declaration properly exposes the zero-knowledge proof functionality to be consumed by other parts of the codebase. This aligns with the PR objectives to add ZK verification to the authorization state.contracts/authorization/src/error.rs (1)
44-46: ZK Proof Verification Error IntegrationThis new error variant correctly handles the case where a zero-knowledge proof fails verification against its verifying key. The error message is clear and informative.
e2e/examples/polytone.rs (1)
620-620: ZK Field Added to Authorization StructureThe added
zk_vk: Nonefield correctly extends the authorization structure to support the optional zero-knowledge verifying key. This change is consistent with the PR's goal of making ZK verification an optional feature in the authorization layer.Cargo.toml (1)
41-43: Appropriate zero-knowledge cryptography dependencies addedThe addition of
ark-bn254,ark-groth16, andark-serializedependencies provides the necessary components for implementing the zero-knowledge verification functionality mentioned in the PR objectives. These are widely-used libraries for ZK proof systems.program-manager/src/tests.rs (1)
255-255: Proper test initialization for new ZK fieldThe new
zk_vk: Nonefield correctly initializes the test AuthorizationInfo struct with a null verification key, maintaining consistency with the schema changes. This ensures that existing tests continue to work while supporting the optional ZK verification key.program-manager/schema/valence-program-manager.json (1)
293-303:✅ Verification successful
Appropriate schema definition for ZK verification key
The schema properly defines the
zk_vkproperty as an optional array of uint8 integers, which is suitable for representing serialized cryptographic keys. This aligns with the PR objective of adding zero-knowledge verification capabilities to the authorization state.
🏁 Script executed:
#!/bin/bash # Description: Verify this schema change is consistent with the AuthorizationInfo struct definition # Check if the AuthorizationInfo struct in authorization-utils has the zk_vk field rg "struct AuthorizationInfo" -A 20 "packages/authorization-utils"Length of output: 1856
Schema for zk_vk Verified and Aligned with AuthorizationInfo Struct
The verification confirms that the
AuthorizationInfostruct inpackages/authorization-utils/src/authorization.rsincludes the fieldzk_vkasOption<VerifyingKey>, which aligns well with the JSON schema definition inprogram-manager/schema/valence-program-manager.json. The schema appropriately represents an optional array of uint8 integers for serialized cryptographic keys, fulfilling the PR objective for adding zero-knowledge verification capabilities.contracts/processor/schema/valence-processor.json (2)
1081-1105: New ZK message type properly defined with required fieldsThe added
cosmwasm_execute_zk_msgtype extends theProcessorMessageschema with zero-knowledge proof support. The schema correctly defines the two required properties:msgandproof, both leveraging the existingBinarytype definition.
2144-2168: Consistent schema definition in responses sectionGood to see that the
cosmwasm_execute_zk_msgtype is consistently defined in the responses section with the same structure as in the first occurrence. This consistency is crucial for proper message handling throughout the system.packages/authorization-utils/src/authorization.rs (3)
5-8: Import organization is clean and appropriateThe imports are well-organized, with the specific addition of
VerifyingKeyfrom thezkmodule, which is necessary for the new ZK verification functionality.
23-24: Well-documented ZK verification key fieldThe comment clearly explains the purpose of the
zk_vkfield: "A ZK verifying key for subroutines that expects a ZK proof." This helps other developers understand when and why this field should be used.
45-45: ZK field properly added to Authorization structThe
zk_vkfield is appropriately added to theAuthorizationstruct, maintaining consistency with theAuthorizationInfostruct.contracts/authorization/schema/valence-authorization.json (2)
1707-1731: Confirm naming consistency for zero-knowledge execute message.The new
"cosmwasm_execute_zk_msg"object nicely separates standard execute messages from those containing proofs. Ensure similar naming is used throughout the codebase for consistent references in JSON schemas and Rust enums.
3673-3697: Maintain consistent design for ZK message definitions.This block for
"cosmwasm_execute_zk_msg"is essentially duplicated from another section. It correctly enforces the required structure for proofs. Keep it synchronized with any future schema changes or naming refinements.packages/authorization-utils/src/builders.rs (3)
24-25: Ensure optional verifying key is suitable for partial usage.Storing
zk_vkasOption<VerifyingKey>is flexible. Verify that call sites handleNonecorrectly, especially when zero-knowledge verification is required.
47-47: Initialize optional ZK key consistently.Setting
zk_vktoNoneby default aligns with it being an optional field. This approach cleanly prevents accidental usage of an uninitialized key.
100-100: Confirm full coverage inbuildmethod.Including
zk_vkinAuthorizationInfoensures the field is used consistently. Ensure any relevant validations, such as proper key length checks, occur upstream if needed.packages/authorization-utils/src/msg.rs (3)
269-269: ZK variant nicely extends the existing message types.Adding
CosmwasmExecuteZkMsg { msg, proof }is a natural extension. Confirm that any tooling (schema generators, etc.) accounts for the new fields in test coverage.
295-295: Consistently return the message field for further processing.The
get_msgbranch forCosmwasmExecuteZkMsgis aligned with other variants. No concerns here.
319-323: Enabling WASM execution with ZK proofs is consistent.Allowing the
CosmwasmExecuteZkMsgvariant to createWasmMsg::Executehelps unify both standard and proof-based messages under a single model. Good approach.packages/authorization-utils/src/zk.rs (1)
1-4: Leverage stable version references for cryptography imports.Imports from
ark_bn254andark_groth16are well-known libraries. Periodically track version updates or security patches to maintain cryptographic integrity.contracts/authorization/src/authorization.rs (1)
377-377: LGTM: CosmwasmExecuteZkMsg is properly integrated with existing message typesThe addition of
CosmwasmExecuteZkMsgto the match arm is appropriate for handling ZK-enabled Cosmwasm execute messages.
| @@ -59,6 +65,7 @@ impl AuthorizationInfo { | |||
| subroutine: self.subroutine, | |||
| priority: self.priority.unwrap_or_default(), | |||
| state: AuthorizationState::Enabled, | |||
| zk_vk: None, | |||
There was a problem hiding this comment.
Initialization needs update to include ZK verification key
The current implementation initializes zk_vk to None regardless of the value provided in AuthorizationInfo. This means that even if a verifying key is provided, it will be discarded during conversion.
Apply this fix:
- zk_vk: None,
+ zk_vk: self.zk_vk,This ensures that any ZK verification key provided in AuthorizationInfo is properly transferred to the Authorization instance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zk_vk: None, | |
| zk_vk: self.zk_vk, |
| pub fn verify_zk_proof(&self, vk: &VerifyingKey) -> bool { | ||
| match self { | ||
| Self::CosmwasmExecuteZkMsg { msg, proof } => { | ||
| zk::verify_proof(vk, proof.as_slice(), msg.as_slice()) | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust proof verification checks or error logs.
verify_zk_proof succinctly checks the proof. If possible, consider logging or returning a detailed error instead of a boolean, providing clearer diagnostics for proof failures.
| pub fn verify_proof(vk: &VerifyingKey, proof: &[u8], inputs: &[u8]) -> bool { | ||
| let pvk: PreparedVerifyingKey<Bn254> = | ||
| match CanonicalDeserialize::deserialize_uncompressed(vk.as_slice()) { | ||
| Ok(k) => k, | ||
| _ => return false, | ||
| }; | ||
|
|
||
| // TODO derive the field elements from the inputs, depending on the zkVM backend | ||
| let _ = inputs; | ||
| let inputs = &[]; | ||
| let inputs = match Groth16::<Bn254>::prepare_inputs(&pvk, inputs) { | ||
| Ok(i) => i, | ||
| _ => return false, | ||
| }; | ||
|
|
||
| let proof = match ark_groth16::Proof::deserialize_compressed(proof) { | ||
| Ok(p) => p, | ||
| _ => return false, | ||
| }; | ||
|
|
||
| Groth16::<Bn254>::verify_proof_with_prepared_inputs(&pvk, &proof, &inputs).is_ok() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct proof inputs once multi-field manipulations occur.
Currently, inputs is a placeholder array. If future changes deserialize real public inputs, ensure the field arithmetic is correct and consider robust error handling instead of returning false on parse failure.
- let inputs = &[];
+ // Example: parse the public inputs from `inputs` here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn verify_proof(vk: &VerifyingKey, proof: &[u8], inputs: &[u8]) -> bool { | |
| let pvk: PreparedVerifyingKey<Bn254> = | |
| match CanonicalDeserialize::deserialize_uncompressed(vk.as_slice()) { | |
| Ok(k) => k, | |
| _ => return false, | |
| }; | |
| // TODO derive the field elements from the inputs, depending on the zkVM backend | |
| let _ = inputs; | |
| let inputs = &[]; | |
| let inputs = match Groth16::<Bn254>::prepare_inputs(&pvk, inputs) { | |
| Ok(i) => i, | |
| _ => return false, | |
| }; | |
| let proof = match ark_groth16::Proof::deserialize_compressed(proof) { | |
| Ok(p) => p, | |
| _ => return false, | |
| }; | |
| Groth16::<Bn254>::verify_proof_with_prepared_inputs(&pvk, &proof, &inputs).is_ok() | |
| } | |
| pub fn verify_proof(vk: &VerifyingKey, proof: &[u8], inputs: &[u8]) -> bool { | |
| let pvk: PreparedVerifyingKey<Bn254> = | |
| match CanonicalDeserialize::deserialize_uncompressed(vk.as_slice()) { | |
| Ok(k) => k, | |
| _ => return false, | |
| }; | |
| // TODO derive the field elements from the inputs, depending on the zkVM backend | |
| let _ = inputs; | |
| // Example: parse the public inputs from `inputs` here | |
| let inputs = match Groth16::<Bn254>::prepare_inputs(&pvk, inputs) { | |
| Ok(i) => i, | |
| _ => return false, | |
| }; | |
| let proof = match ark_groth16::Proof::deserialize_compressed(proof) { | |
| Ok(p) => p, | |
| _ => return false, | |
| }; | |
| Groth16::<Bn254>::verify_proof_with_prepared_inputs(&pvk, &proof, &inputs).is_ok() | |
| } |
| fn validate_messages(&self, messages: &[ProcessorMessage]) -> Result<(), ContractError> { | ||
| // if the contract has a verifying key, then only processor messages that | ||
| // contains proofs are allowed | ||
| if let Some(vk) = &self.zk_vk { | ||
| for m in messages { | ||
| if !m.verify_zk_proof(vk) { | ||
| return Err(ContractError::ZKProofVerificationError); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate ZK implementation with comprehensive testing
The ZK verification logic is a critical security component. Ensure comprehensive testing covers various scenarios including:
- Messages with valid proofs
- Messages with invalid proofs
- Mixed message types when a verifying key is present
- Behavior when no verifying key is present
🏁 Script executed:
#!/bin/bash
# Check if there are tests covering the ZK verification functionality
echo "Searching for ZK verification tests..."
rg -i "test.*zk|zk.*test" --type rustLength of output: 126
Action Required: Add comprehensive tests for ZK verification logic
The current implementation in contracts/authorization/src/authorization.rs (lines 334–343) enforces ZK proof verification when a verifying key is set. However, our repository search did not reveal any tests specifically covering this ZK logic. Please ensure that you add or update tests to validate all critical scenarios including:
- Messages with valid proofs.
- Messages with invalid proofs (triggering
ContractError::ZKProofVerificationError). - Mixed message types when a verifying key is present.
- Correct behavior when no verifying key is provided.
These tests are essential to guarantee the security integrity of the ZK verification path.
This commit introduces a verifying key as optional attribute of the authorization layer.
Every deployed program will be bound to an authorization contract, and will optionally be a ZK program.
This commit introduces the skeleton for a ZK verification for such case. It adds a proof binary to a Cosm execute message (the other variants are currently unimplemented for ZK), and binds the authorization state to a ZK key for the shielded program.
It uses ark-groth16, an industry standard that will support the output proof of most of the zkVM implementations. As tech debt, we might want a separated library that will decouple such decisions from the concrete implementation, leaving all of the involved implementations (zkVM, verifier, msg encoding) decoupled on this crate.
Summary by CodeRabbit
New Features
Bug Fixes