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.

Sign up for GitHub

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
merged 3 commits into from
Jan 12, 2024
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a574a4
add `process_ixs_with_additional_compute_budget`
samkim-crypto Jan 12, 2024
23cb9c4
add extra compute budget for transfer with fee
samkim-crypto Jan 12, 2024
34936d9
update `process_ixs_with_additional_compute_budget` to take in a regu…
samkim-crypto Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions token/client/src/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ where
async fn construct_tx<S: Signers>(
&self,
token_instructions: &[Instruction],
additional_compute_budget: Option<u32>,
signing_keypairs: &S,
) -> TokenResult<Transaction> {
let mut instructions = vec![];
Expand All @@ -532,6 +533,14 @@ where

instructions.extend_from_slice(token_instructions);

if let Some(additional_compute_budget) = additional_compute_budget {
instructions.push(
solana_sdk::compute_budget::ComputeBudgetInstruction::set_compute_unit_limit(
additional_compute_budget,
),
);
}

let (message, blockhash) =
if let (Some(nonce_account), Some(nonce_authority), Some(nonce_blockhash)) = (
self.nonce_account,
Expand Down Expand Up @@ -581,7 +590,7 @@ where
signing_keypairs: &S,
) -> TokenResult<T::SimulationOutput> {
let transaction = self
.construct_tx(token_instructions, signing_keypairs)
.construct_tx(token_instructions, None, signing_keypairs)
.await?;

self.client
Expand All @@ -596,7 +605,27 @@ where
signing_keypairs: &S,
) -> TokenResult<T::Output> {
let transaction = self
.construct_tx(token_instructions, signing_keypairs)
.construct_tx(token_instructions, None, signing_keypairs)
.await?;

self.client
.send_transaction(&transaction)
.await
.map_err(TokenError::Client)
}

pub async fn process_ixs_with_additional_compute_budget<S: Signers>(
&self,
token_instructions: &[Instruction],
additional_compute_budget: u32,
signing_keypairs: &S,
) -> TokenResult<T::Output> {
let transaction = self
.construct_tx(
token_instructions,
Some(additional_compute_budget),
signing_keypairs,
)
.await?;

self.client
Expand Down Expand Up @@ -2618,7 +2647,10 @@ where
.new_decryptable_available_balance(transfer_amount, source_aes_key)
.map_err(|_| TokenError::AccountDecryption)?;

self.process_ixs(
// additional compute budget required for `VerifyTransferWithFee`
const TRANSFER_WITH_FEE_COMPUTE_BUDGET: u32 = 500_000;
Comment on lines +2650 to +2651
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!


self.process_ixs_with_additional_compute_budget(
&confidential_transfer::instruction::transfer_with_fee(
&self.program_id,
source_account,
Expand All @@ -2629,6 +2661,7 @@ where
&multisig_signers,
proof_location,
)?,
TRANSFER_WITH_FEE_COMPUTE_BUDGET,
signing_keypairs,
)
.await
Expand Down