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 CUs to close account instruction #33042

Merged
merged 1 commit into from
Aug 30, 2023
Merged

[zk-token-proof] Add CUs to close account instruction #33042

merged 1 commit into from
Aug 30, 2023

Conversation

samkim-crypto
Copy link
Contributor

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

Problem

The close context state account instruction does not incure a fee. If the feature #30620 is activated, then any native instruction that does not incure a fee will fail.

Summary of Changes

Added compute units to close account. The instruction itself performs two things:

  • Check if the owner of an account (the context account) is the signer for the transaction
  • Frees the account

I modeled the compute units similar to the spl-token close account instruction. The token close account instruction performs similar steps as the close context state account instruction here. Although the token instruction is executed as BPF, it only really performs pubkey comparison and then frees the account, so the cost should be quite similar. The token close instruction incurs about 3_300 compute units, so this amount was assigned to the close context state account instruction.

Fixes #

@@ -132,6 +132,11 @@ declare_process_instruction!(process_instruction, 0, |invoke_context| {

match instruction {
ProofInstruction::CloseContextState => {
if native_programs_consume_cu {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the if native_programs_consume_cu { check at all? Can't we just always consume the CUs?

Copy link
Contributor Author

@samkim-crypto samkim-crypto Aug 30, 2023

Choose a reason for hiding this comment

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

Yes, we don't need this check and can just remove it. It was added in long time ago and it was never really removed. The newer instructions that were added in don't have this feature gate check.

I was going to remove this check (hence the title...), but if I remove it here, then some of the CI tests fail due to some proof instructions exceeding compute unit limit. So I thought I would just add the compute units for close instruction in this PR to get that backported quickly and then remove the feature gate in a separate PR. As long as the proof program activation is coordinated with #30620, then I thought this would be okay.

@samkim-crypto samkim-crypto changed the title [zk-token-proof] Remove feature gate on proof instruction native compute units and add CUs to close account instruction [zk-token-proof] Add CUs to close account instruction Aug 30, 2023
@samkim-crypto samkim-crypto added the v1.16 PRs that should be backported to v1.16 label Aug 30, 2023
@samkim-crypto
Copy link
Contributor Author

@taozhu-chicago sorry for taking so long to address this 🙏 . This PR adds compute units for an instruction, CloseContextState, that was not assigned compute units. Can you just sanity check the changes?

I also found out why the test is actually not failing even though this instruction does not consume compute units. The program test disables the native compute unit feature here.

Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm - thanks for adding defaults to this instruction. A nit can be done in separate PR.

@@ -134,6 +134,11 @@ declare_process_instruction!(process_instruction, 0, |invoke_context| {

match instruction {
ProofInstruction::CloseContextState => {
if native_programs_consume_cu {
invoke_context
.consume_checked(3_300)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: perhaps can define those hardcoded number as const, like this, so the default cost can be referenced elsewhere like here.

@samkim-crypto samkim-crypto merged commit 79c02c8 into solana-labs:master Aug 30, 2023
mergify bot pushed a commit that referenced this pull request Aug 30, 2023
add compute units for close context state account

(cherry picked from commit 79c02c8)
samkim-crypto added a commit that referenced this pull request Aug 30, 2023
…t of #33042) (#33075)

[zk-token-proof] Add CUs to close account instruction (#33042)

add compute units for close context state account

(cherry picked from commit 79c02c8)

Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
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