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] Enable proof program to be invoked as an inner call #33045

Merged
merged 1 commit into from
Aug 30, 2023
Merged

[zk-token-proof] Enable proof program to be invoked as an inner call #33045

merged 1 commit into from
Aug 30, 2023

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Aug 29, 2023

Problem

Currently, the ZK Token proof program contains a check that prevents other programs from invoking the proof program as an inner call. In the earlier version of the program, we did not vision the proof program from being invoked as an inner call, so this check made sense. However, with the way the program is set up, invocation of the proof program instructions can be useful in certain applications like in solana-labs/solana-program-library#3984. Additionally, invocation to CloseContextState instruction could allow the following application:

Parallel execution of Token22 transfers:
  • Due to the instruction data limit, some of the zkp's for the Token22 confidential transfer instructions must be divided into pieces.
  • The processor for the confidential transfer instruction currently allows "parallel" execution of transactions. Namely, the client can submit two transactions asynchronously
    • tx_0 = [
      system::create_account, // create account for proof_0
      zk_token_proof::create_proof_0, // initialize proof context account
      token_2022::confidential_transfer, // token-2022 instruction
      ]
    • tx_1 = [
      system::create_account, // create account for proof_1
      zk_token_proof::create_proof_1, // initialize proof context account
      token_2022::confidential_transfer, // token-2022 instruction (duplicate of the instruction in tx_0
      ]
  • The token-2022 instruction belonging to the transaction that gets processed first no-ops. The token-2022 instruction belonging to the transaction that gets processed later follows through with the main transfer logic.
  • If CPI is enabled, then we can also allow the token-2022 instruction that gets processed later to invoke the ZK Token proof program CloseContextState instruction to close down the proof accounts, allowing a confidential transfer to require only two asynchronous transactions.

Summary of Changes

Remove the check that prevents the proof program from being invoked as an inner function.

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Aug 29, 2023
@samkim-crypto samkim-crypto requested a review from mvines August 29, 2023 16:30
@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Aug 29, 2023
@mvines mvines requested a review from joncinque August 29, 2023 16:55
@mvines
Copy link
Member

mvines commented Aug 29, 2023

oof, ok. will we need a re-audit due to this? also if there's any way around modifying the proof program at this point, sooooo close to shipping v1.16, we should take it. or potentially defer this change to v1.17+ and deal with it in token22 in the meantime.

@samkim-crypto
Copy link
Contributor Author

Yes, sorry about the late changes. We can just add this to 1.17. The audit with token22 just started yesterday, so we can probably tell them to include this change as well...

But there is this one WIP change #33042 that unfortunately must be backported... Currently, the CloseContextState instruction does not incur any compute units. Once #30620 is activated, I believe any native program instruction that does not incur a compute unit would fail, so the compute costs for CloseContextState instruction should be added. I apologize for not addressing this earlier 🙏 . I was hoping to finish this PR today...

@joncinque
Copy link
Contributor

How about just allowing the close instruction in CPI in 1.16? That's the only part that's needed for the new tx flow, and reduces the potential harm surface, unlike the rest of the proof program which has high compute needs. Otherwise, deferring to 1.17 isn't too bad

@mvines
Copy link
Member

mvines commented Aug 29, 2023

How about just allowing the close instruction in CPI in 1.16? That's the only part that's needed for the new tx flow, and reduces the potential harm surface, unlike the rest of the proof program which has high compute needs. Otherwise, deferring to 1.17 isn't too bad

Yeah that definitely sounds better for v1.16. For v1.17 would we even need to allow the rest of the instructions in CPI?

@joncinque
Copy link
Contributor

For v1.17 would we even need to allow the rest of the instructions in CPI?

I don't think so, but I'll defer to Sam on that one

@samkim-crypto
Copy link
Contributor Author

Okay, sounds good 🙏. I think we actually don't need CPI for verification instructions since they can always be included in the same transaction. CPI would only be useful for asynchronous execution of instructions, so just the close instruction would suffice. I will update the changes.

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 good to me!

@samkim-crypto samkim-crypto added the v1.16 PRs that should be backported to v1.16 label Aug 30, 2023
@samkim-crypto samkim-crypto merged commit 01dbe49 into solana-labs:master Aug 30, 2023
mergify bot pushed a commit that referenced this pull request Aug 30, 2023
…33045)

enable close context state account instruction to be invoked as an inner call

(cherry picked from commit 01dbe49)
samkim-crypto added a commit that referenced this pull request Aug 30, 2023
…r call (backport of #33045) (#33071)

[zk-token-proof] Enable proof program to be invoked as an inner call (#33045)

enable close context state account instruction to be invoked as an inner call

(cherry picked from commit 01dbe49)

Co-authored-by: samkim-crypto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants