Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[zk-token-proof] Add functionality to read proof from accounts instead of instruction data #34750

Merged
merged 13 commits into from
Jan 12, 2024
Merged

[zk-token-proof] Add functionality to read proof from accounts instead of instruction data #34750

merged 13 commits into from
Jan 12, 2024

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jan 11, 2024

Problem

Some of the proofs in the zk-token-proof program is too large to fit in a single transaction.

Summary of Changes

Update the proof program so that users can provide the zkp as either part of the instruction data or as an account data.

still TODO:

  • Add feature gate
  • Update instruction docs

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Jan 11, 2024
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great for the most part already! Just a couple of small suggestions

programs/zk-token-proof/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 71 to 79
*bytemuck::try_from_bytes::<T>(proof_data).map_err(|_| {
ic_msg!(invoke_context, "invalid proof data");
InstructionError::InvalidInstructionData
})?
} else {
*ProofInstruction::proof_data::<T, U>(instruction_data).ok_or_else(|| {
ic_msg!(invoke_context, "invalid proof data");
InstructionError::InvalidInstructionData
})?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid cloning the whole proof data here, how about verifying the proof in each branch, and then have it return the context? That way you're copying a lot less data, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, very good idea!

programs/zk-token-proof/src/lib.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 59 lines in your changes are missing coverage. Please review.

Comparison is base (d3690dd) 81.8% compared to head (550f7e6) 81.8%.
Report is 39 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34750     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         824      824             
  Lines      222692   222742     +50     
=========================================
- Hits       182314   182308      -6     
- Misses      40378    40434     +56     

@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Jan 12, 2024
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly nits, the rest looks great!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are all really good!

sdk/src/feature_set.rs Outdated Show resolved Hide resolved
@@ -760,6 +760,10 @@ pub mod deprecate_executable_meta_update_in_bpf_loader {
solana_sdk::declare_id!("k6uR1J9VtKJnTukBV2Eo15BEy434MBg8bT6hHQgmU8v");
}

pub mod enable_zk_from_account {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit-up-to-you: maybe we can call it enable_zk_proof_from_account to be clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yes that will be clearer.

zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
@samkim-crypto
Copy link
Contributor Author

Thanks for catching some of my sloppy mistakes 🙏

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, glad to see it turned out pretty simply and easy to test. All that code reuse was really worth for a change like this!

@samkim-crypto samkim-crypto merged commit b222fdf into solana-labs:master Jan 12, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants