[zk-token-sdk] add range-proof proof instruction #31788

samkim-crypto · 2023-05-24T03:46:44Z

Problem

The zk-token-proof program does not yet have an instruction that can verify range proof on a Pedersen commitment.

Summary of Changes

Add VerifyRangeProof64 instruction. This instruction can be used to prove that a committed value is in a certain range.

The verification for these instruction was benched in the devserver and CU units were computed assuming that 1 CU should take roughly 33ns (as per #25464 (comment)).

Fixes #

codecov · 2023-05-24T06:41:19Z

Codecov Report

Merging #31788 (8669c33) into master (e396a10) will increase coverage by 0.0%.
The diff coverage is 66.6%.

@@           Coverage Diff           @@
##           master   #31788   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         737      738    +1     
  Lines      205994   206036   +42     
=======================================
+ Hits       168699   168746   +47     
+ Misses      37295    37290    -5

mvines · 2023-05-24T18:38:27Z

programs/zk-token-proof/src/lib.rs

+        ProofInstruction::VerifyRangeProof64 => {
+            if native_programs_consume_cu {
+                invoke_context
+                    .consume_checked(105_066)


105_066 CUs is pretty exact. Where did this come from?
I guess more generally does it make sense to have such precise values (ie, like 2,619 for VerifyPubkeyValidity) instead of a more round number like 105_000? These are basically just rough estimates anyway?

Sorry I forgot to write it in the pr description. So the data verification was benchmarked on my solana devserver and the time was divided the time by 33ns (https://discordapp.com/channels/428295358100013066/977244255212937306/1027500524901253140). I was hoping to include the benchmark code on a subsequent pr. These are approximate, so they don't have to be precise.

Do you think it makes sense to round each instruction costs to a clean number? It probably makes sense to do it in a separate PR for the rest of the instructions.

The very specific numbers to me convey more than a rough approximation, but practically I guess it doesn't matter much. Definitely a separate PR if we want to change them. My main argument for doing so would be for easy of future communication: 100k or even 105k is much easier to discuss than 105,066.

Okay, I will update the numbers in a follow-up PR and perhaps also add bench code in that PR as well.

mvines · 2023-05-24T18:40:52Z

programs/zk-token-proof-tests/tests/process_transaction.rs

    ProofInstruction::VerifyZeroBalance,
    ProofInstruction::VerifyWithdraw,
    ProofInstruction::VerifyCiphertextCiphertextEquality,
    ProofInstruction::VerifyTransfer,
    ProofInstruction::VerifyTransferWithFee,
    ProofInstruction::VerifyPubkeyValidity,
+    ProofInstruction::VerifyRangeProof64,


I'd prefer an instruction name that is more specific to the actual operation it's performing, which I think if I understood the comment at the top of range_proof.rs might look more like:

Suggested change

ProofInstruction::VerifyRangeProof64,

ProofInstruction::VerifyPedersenCommitmentHoldsExactU64Value,

Yes, that is exactly what the range proof is certifying. If we do go this route though, then we would want to change the names for the other instructions as well for consistency, but the compact names for many of the instructions are not clear (#31793 (comment) and also token-2022 specific instructions like Transfer and Withdraw).

What do you think about naming it VerifyRangeProofBitLength64 to be a little more specific and then perhaps moving the proof descriptions on top of the instruction submodules like range_proof.rs directly to zk_token_proof_instruction.rs on top of each instruction?

Unless there's something fundamentally different between BitLength64 and U64 that I'm missing, my preference would be for VerifyRangeProofU64.

Yes, let's make it VerifyRangeProofU64 to make it short and simple!

mvines · 2023-05-24T19:03:54Z

zk-token-sdk/src/zk_token_proof_instruction.rs

+    /// Accounts expected by this instruction:
+    ///
+    ///   * Creating a proof context account
+    ///   0. `[writable]` The proof context account


This is common for all the other instructions, but I think it'd be helpful to also say that the "The proof context account" is expected to be allocated to the expected data size and assigned to the proof program before invoking this instruction

Yes, you are right. I'll include that remark in the instruction description.

…must be pre-allocated

mvines · 2023-05-25T18:44:50Z

zk-token-sdk/src/zk_token_proof_instruction.rs

@@ -132,6 +132,27 @@ pub enum ProofInstruction {
    ///   `PubkeyValidityData`
    ///
    VerifyPubkeyValidity,
+
+    /// Verify a 64-bit range proof.


Regarding the instruction renaming to something like VerifyRangeProofBitLength64: I feel like we could maybe deal with in comments, here, instead actually. Currently one needs to dig through the header of zk-token-sdk/src/instruction/range_proof.rs to figure out exactly what this instruction is doing. But this comment block here is going to be the first place anybody who wants to understand what the instruction is goes to. So if we boost up the docs inline here (the other docs are fine too!), then that would also mostly solve my comment (likewise for the instructions being added in the other PR)

Yeah, I like the idea of flashing this out some.

Great, I will move the instruction descriptions to zk_token_proof_instruction.rs.

CriesofCarrots · 2023-05-25T20:16:50Z

programs/zk-token-proof-tests/tests/process_transaction.rs

    ProofInstruction::VerifyZeroBalance,
    ProofInstruction::VerifyWithdraw,
    ProofInstruction::VerifyCiphertextCiphertextEquality,
    ProofInstruction::VerifyTransfer,
    ProofInstruction::VerifyTransferWithFee,
    ProofInstruction::VerifyPubkeyValidity,
+    ProofInstruction::VerifyRangeProof64,


Unless there's something fundamentally different between BitLength64 and U64 that I'm missing, my preference would be for VerifyRangeProofU64.

CriesofCarrots · 2023-05-25T20:18:19Z

zk-token-sdk/src/zk_token_proof_instruction.rs

@@ -132,6 +132,27 @@ pub enum ProofInstruction {
    ///   `PubkeyValidityData`
    ///
    VerifyPubkeyValidity,
+
+    /// Verify a 64-bit range proof.


Yeah, I like the idea of flashing this out some.

CriesofCarrots · 2023-05-25T20:20:18Z

zk-token-sdk/src/instruction/range_proof.rs

+};
+
+#[cfg(not(target_os = "solana"))]
+const RANGEPROOF64_BIT_LENGTH: usize = 64;


Would it be possible to use u64::BITS instead of adding this new constant?

Oh, I never knew/used u64::BITS before. I think it will work!

CriesofCarrots

r+ nits

zk-token-sdk/src/zk_token_proof_instruction.rs

programs/zk-token-proof-tests/tests/process_transaction.rs

zk-token-sdk/src/instruction/range_proof.rs

Co-authored-by: Tyera <teulberg@gmail.com>

CriesofCarrots

lgtm

samkim-crypto added the work in progress This isn't quite right yet label May 24, 2023

samkim-crypto added 3 commits May 24, 2023 13:42

add VerifyRangeProof instruction

eabe0ad

update proof program processor for VerifyRangeProof64

873cccc

specify target arch for `RANGEPROOF64_BIT_LENGTH

f094ab9

samkim-crypto removed the work in progress This isn't quite right yet label May 24, 2023

samkim-crypto requested review from mvines and CriesofCarrots May 24, 2023 07:25

samkim-crypto added 2 commits May 24, 2023 16:51

rename transcript_new to new_transcript

f56e963

add constructor for VerifyRangeProof64

f177318

mvines reviewed May 24, 2023

View reviewed changes

add remark in the instruction description that context state account …

e80c4b8

…must be pre-allocated

samkim-crypto requested a review from mvines May 25, 2023 01:03

samkim-crypto mentioned this pull request May 25, 2023

[zk-token-sdk] Add sigma/range proof verification instructions #30816

Closed

mvines reviewed May 25, 2023

View reviewed changes

CriesofCarrots reviewed May 25, 2023

View reviewed changes

samkim-crypto added 3 commits May 26, 2023 08:13

add proof description in zk_token_proof_instruction.rs

8f14bcd

rename VerifyRangeProof64 to VerifyRangeProofU64

e5da8d0

use u64::BITS

90115be

samkim-crypto requested review from mvines and CriesofCarrots May 26, 2023 00:57

CriesofCarrots reviewed May 26, 2023

View reviewed changes

samkim-crypto and others added 3 commits May 26, 2023 10:29

Update zk-token-sdk/src/zk_token_proof_instruction.rs

54cd5e2

Co-authored-by: Tyera <teulberg@gmail.com>

Apply suggestions from code review

d79fb62

Co-authored-by: Tyera <teulberg@gmail.com>

fix range proof test

8669c33

samkim-crypto requested a review from CriesofCarrots May 26, 2023 02:50

CriesofCarrots approved these changes May 26, 2023

View reviewed changes

samkim-crypto merged commit ef7ca5e into solana-labs:master May 26, 2023

samkim-crypto mentioned this pull request Jun 14, 2023

[zk-token-proof] Add benches for proof instructions #32071

Merged

samkim-crypto mentioned this pull request Oct 27, 2023

[zk-token-proof] Round compute units to nice numbers #33910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[zk-token-sdk] add range-proof proof instruction #31788

[zk-token-sdk] add range-proof proof instruction #31788

samkim-crypto commented May 24, 2023 •

edited

Loading

codecov bot commented May 24, 2023 •

edited

Loading

mvines May 24, 2023

samkim-crypto May 24, 2023

mvines May 25, 2023

samkim-crypto May 25, 2023

mvines May 24, 2023

samkim-crypto May 24, 2023

CriesofCarrots May 25, 2023

samkim-crypto May 25, 2023

mvines May 24, 2023

samkim-crypto May 24, 2023

mvines May 25, 2023

CriesofCarrots May 25, 2023

samkim-crypto May 25, 2023

CriesofCarrots May 25, 2023

CriesofCarrots May 25, 2023

CriesofCarrots May 25, 2023

samkim-crypto May 25, 2023

CriesofCarrots left a comment

CriesofCarrots left a comment

	ProofInstruction::VerifyRangeProof64,
	ProofInstruction::VerifyPedersenCommitmentHoldsExactU64Value,

[zk-token-sdk] add range-proof proof instruction #31788

[zk-token-sdk] add range-proof proof instruction #31788

Conversation

samkim-crypto commented May 24, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented May 24, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

samkim-crypto commented May 24, 2023 •

edited

Loading

codecov bot commented May 24, 2023 •

edited

Loading