[zk-token-sdk] Add ciphertext-commitment equality proof instruction #31808
Conversation
Codecov Report
@@ Coverage Diff @@
## master #31808 +/- ##
========================================
Coverage 81.9% 81.9%
========================================
Files 747 752 +5
Lines 206417 206747 +330
========================================
+ Hits 169111 169416 +305
- Misses 37306 37331 +25 |
programs/zk-token-proof/src/lib.rs
Outdated
| @@ -192,5 +192,17 @@ declare_process_instruction!(process_instruction, 0, |invoke_context| { | |||
| ic_msg!(invoke_context, "VerifyPubkeyValidity"); | |||
| process_verify_proof::<PubkeyValidityData, PubkeyValidityProofContext>(invoke_context) | |||
| } | |||
| ProofInstruction::VerifyCiphertextCommitmentEquality => { | |||
| if native_programs_consume_cu { | |||
There was a problem hiding this comment.
Since this is a new instruction, does it need the feature-gate check?
I guess there's an argument to be made for consistency between all the instructions, but I think anything not released before that feature gate was added doesn't need that logic.
There was a problem hiding this comment.
Ah you are right. I think it makes sense to remove it here and also for the other range proof instructions as well.
There was a problem hiding this comment.
So actually, I might be misunderstanding things, but is native_programs_consume_cu fully active?
It seems like it makes sense not have the check for code that are added after the feature gate is fully activated since that is a meaningless check.
But if the feature gate is only created, but not yet fully active, then it seems natural to include the check since this is consuming compute units for native instructions?
There was a problem hiding this comment.
I'll touch base with you about this, and feature gating generally, tomorrow. Getting late here and I want to make sure I'm reasoning properly 😅
There was a problem hiding this comment.
Oh btw, I think tomorrow is unplug day, so please take your time with the reviews! Any delay here is on me.
There was a problem hiding this comment.
I think we can remove the native_programs_consume_cu checks from the proof program entirely. It's totally ok that when the proof program is activated, even if native_programs_consume_cu has not yet activated, that the proof program consume CUs for its instructions
There was a problem hiding this comment.
Okay, you are right. The instructions are expensive, so they should always consume CU units anyway. I will remove it for this instruction here and perhaps remove the others in the benchmark PR.
There was a problem hiding this comment.
It's totally ok that when the proof program is activated, even if native_programs_consume_cu has not yet activated, that the proof program consume CUs for its instructions
Great, that was my reasoning. The corollary that I was mulling too late last night and wanted to confirm is whether any of the proof program has been activated anywhere. If so, the new instructions themselves would need to be feature-gated.
zk-token-sdk/src/instruction/mod.rs
Outdated
| @@ -19,6 +20,9 @@ use { | |||
| }; | |||
| pub use { | |||
| bytemuck::Pod, | |||
| ctxt_comm_equality::{ | |||
There was a problem hiding this comment.
Same comment as on the other PR about spelling things out. Since this is a new module, can we go ahead and spell out ciphertext_commitment_, please?
(My brain wants to read it as context_communication as it is...)
There was a problem hiding this comment.
Okay, will do!
Problem
The zk-token-proof program does not yet have an instruction that can verify whether a ciphertext-commitment proof: a proof certifying that an ElGamal ciphertext and a Pedersen commitment encrypt/encode the same value.
Summary of Changes
Add
VerifyCiphertextCommitmentEqualityProofinstruction. The verification for this instruction was benched on the devserver and CU units were computing assuming that 1 CU should take roughly 33ns (as per #25464 (comment)).Fixes #