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

[token-client] Add interface to add additional compute budget for transactions #6121

Merged

Conversation

samkim-crypto
Copy link
Contributor

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

Problem

With the new activation of the feature 8pgXCMNXC8qyEFypuwpXyRxLXZdpM4Qo72gJ6k87A6wL - Native program should consume compute units activated, the token-2022 tests seem to break downstream ci in the monorepo due to the token-2022 confidential transfer with fee instruction taking too many compute units.

Summary of Changes

  • Update the token-client by adding process_ixs_with_additional_compute_budget, which adds an extra compute budget instruction to the transaction
  • Use the new function to allocate additional compute budget for transfer with fee

@samkim-crypto samkim-crypto added WIP Work in progress and removed WIP Work in progress labels Jan 12, 2024
joncinque
joncinque previously approved these changes 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.

Just one nit, then all good!

pub async fn process_ixs_with_additional_compute_budget<S: Signers>(
&self,
token_instructions: &[Instruction],
additional_compute_budget: Option<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to make this an Option rather than a normal u32? If someone is using this function, we should probably force them to specify the additional compute budget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah looking at it now, I guess it is kind of weird. I wanted this function to be a more general version of process_ixs which process_ixs can invoke, but the name is misleading. I'll fix the syntax to take in a normal u32.

@mergify mergify bot dismissed joncinque’s stale review January 12, 2024 13:23

Pull request has been modified.

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.

Sorry, just one more question. the other change looks good!

Comment on lines +2650 to +2651
// additional compute budget required for `VerifyTransferWithFee`
const TRANSFER_WITH_FEE_COMPUTE_BUDGET: u32 = 500_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't catch this on the first review, but what about the other version of transfer with fee: confidential_transfer_transfer_with_fee_and_split_proofs and the parallel version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it seems like those don't seem to get caught on the CU limit. The split proofs (range proof, ciphertext validity proof, etc.) have smaller CUs since they are essentially chunks of the VerifyTransferWithFee. Some of those exceed 200k limit, but the CU averaging seems to allow them to pass.

The VerifyTransferWithFee is the culmination of all those split proofs and therefore, have huge CU requirement for which even the CU averaging is not sufficient to bring it down to 200k.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be safe to allocate additional budget for those lighter instructions as well, but perhaps it could be done in a follow-up PR to unblock solana-labs/solana#34695 before the feature freeze.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, as long as the other ones work, then all good!

@samkim-crypto samkim-crypto merged commit 23916b2 into solana-labs:master Jan 12, 2024
31 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