Skip to content

cli: Simulate for compute units consumed during transfer #1923

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

Merged
joncinque merged 4 commits into from
Aug 22, 2024
Merged

cli: Simulate for compute units consumed during transfer #1923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b52ec20
cli: Plumb down `ComputeUnitLimit` to resolve tx
joncinque Jun 27, 2024
17a97df
Use simulated compute units in transaction
joncinque Jun 27, 2024
6632162
wallet: Use simulated compute units for non-nonce transactions
joncinque Jun 27, 2024
d1717b7
Modify transfer test
joncinque Jun 28, 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
1 change: 1 addition & 0 deletions clap-utils/src/compute_budget.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub fn compute_unit_limit_arg<'a, 'b>() -> Arg<'a, 'b> {
.help(COMPUTE_UNIT_LIMIT_ARG.help)
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum ComputeUnitLimit {
/// Do not include a compute unit limit instruction, which will give the
/// transaction a compute unit limit of:
Expand Down
4 changes: 3 additions & 1 deletion cli/src/cluster_query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,6 +1520,7 @@ pub fn process_ping(
let to = config.signers[0].pubkey();
lamports = lamports.saturating_add(1);

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let ixs = vec![system_instruction::transfer(
&config.signers[0].pubkey(),
Expand All @@ -1528,7 +1529,7 @@ pub fn process_ping(
)]
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});
Message::new(&ixs, Some(&config.signers[0].pubkey()))
};
Expand All @@ -1538,6 +1539,7 @@ pub fn process_ping(
SpendAmount::Some(lamports),
&blockhash,
&config.signers[0].pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
4 changes: 3 additions & 1 deletion cli/src/feature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ use {
console::style,
serde::{Deserialize, Serialize},
solana_clap_utils::{
fee_payer::*, hidden_unless_forced, input_parsers::*, input_validators::*, keypair::*,
compute_budget::ComputeUnitLimit, fee_payer::*, hidden_unless_forced, input_parsers::*,
input_validators::*, keypair::*,
},
solana_cli_output::{cli_version::CliVersion, QuietDisplay, VerboseDisplay},
solana_remote_wallet::remote_wallet::RemoteWalletManager,
Expand Down Expand Up @@ -972,6 +973,7 @@ fn process_activate(
SpendAmount::Some(rent),
&blockhash,
&fee_payer.pubkey(),
ComputeUnitLimit::Default,
|lamports| {
Message::new(
&feature::activate_with_lamports(&feature_id, &fee_payer.pubkey(), lamports),
Expand Down
6 changes: 4 additions & 2 deletions cli/src/nonce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ pub fn process_create_nonce_account(

let nonce_authority = nonce_authority.unwrap_or_else(|| config.signers[0].pubkey());

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let ixs = if let Some(seed) = seed.clone() {
create_nonce_account_with_seed(
Expand All @@ -475,7 +476,7 @@ pub fn process_create_nonce_account(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
})
} else {
create_nonce_account(
Expand All @@ -487,7 +488,7 @@ pub fn process_create_nonce_account(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
})
};
Message::new(&ixs, Some(&config.signers[0].pubkey()))
Expand All @@ -501,6 +502,7 @@ pub fn process_create_nonce_account(
amount,
&latest_blockhash,
&config.signers[0].pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
51 changes: 40 additions & 11 deletions cli/src/spend_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ use {
crate::{
checks::{check_account_for_balance_with_commitment, get_fee_for_messages},
cli::CliError,
compute_budget::{simulate_and_update_compute_unit_limit, UpdateComputeUnitLimitResult},
},
clap::ArgMatches,
solana_clap_utils::{input_parsers::lamports_of_sol, offline::SIGN_ONLY_ARG},
solana_clap_utils::{
compute_budget::ComputeUnitLimit, input_parsers::lamports_of_sol, offline::SIGN_ONLY_ARG,
},
solana_rpc_client::rpc_client::RpcClient,
solana_sdk::{
commitment_config::CommitmentConfig, hash::Hash, message::Message,
Expand Down Expand Up @@ -52,6 +55,7 @@ pub fn resolve_spend_tx_and_check_account_balance<F>(
amount: SpendAmount,
blockhash: &Hash,
from_pubkey: &Pubkey,
compute_unit_limit: ComputeUnitLimit,
build_message: F,
commitment: CommitmentConfig,
) -> Result<(Message, u64), CliError>
Expand All @@ -65,6 +69,7 @@ where
blockhash,
from_pubkey,
from_pubkey,
compute_unit_limit,
build_message,
commitment,
)
Expand All @@ -77,6 +82,7 @@ pub fn resolve_spend_tx_and_check_account_balances<F>(
blockhash: &Hash,
from_pubkey: &Pubkey,
fee_pubkey: &Pubkey,
compute_unit_limit: ComputeUnitLimit,
build_message: F,
commitment: CommitmentConfig,
) -> Result<(Message, u64), CliError>
Expand All @@ -92,6 +98,7 @@ where
from_pubkey,
fee_pubkey,
0,
compute_unit_limit,
build_message,
)?;
Ok((message, spend))
Expand All @@ -113,6 +120,7 @@ where
from_pubkey,
fee_pubkey,
from_rent_exempt_minimum,
compute_unit_limit,
build_message,
)?;
if from_pubkey == fee_pubkey {
Expand Down Expand Up @@ -150,41 +158,57 @@ fn resolve_spend_message<F>(
from_pubkey: &Pubkey,
fee_pubkey: &Pubkey,
from_rent_exempt_minimum: u64,
compute_unit_limit: ComputeUnitLimit,
build_message: F,
) -> Result<(Message, SpendAndFee), CliError>
where
F: Fn(u64) -> Message,
{
let fee = match blockhash {
let (fee, compute_unit_info) = match blockhash {
Some(blockhash) => {
let mut dummy_message = build_message(0);
dummy_message.recent_blockhash = *blockhash;
get_fee_for_messages(rpc_client, &[&dummy_message])?
let compute_unit_info = if compute_unit_limit == ComputeUnitLimit::Simulated {
// Simulate for correct compute units
if let UpdateComputeUnitLimitResult::UpdatedInstructionIndex(ix_index) =
simulate_and_update_compute_unit_limit(rpc_client, &mut dummy_message)?
{
Some((ix_index, dummy_message.instructions[ix_index].data.clone()))
} else {
None
}
} else {
None
};
(
get_fee_for_messages(rpc_client, &[&dummy_message])?,
compute_unit_info,
)
}
None => 0, // Offline, cannot calculate fee
None => (0, None), // Offline, cannot calculate fee
};

match amount {
SpendAmount::Some(lamports) => Ok((
let (mut message, spend_and_fee) = match amount {
SpendAmount::Some(lamports) => (
build_message(lamports),
SpendAndFee {
spend: lamports,
fee,
},
)),
),
SpendAmount::All => {
let lamports = if from_pubkey == fee_pubkey {
from_balance.saturating_sub(fee)
} else {
from_balance
};
Ok((
(
build_message(lamports),
SpendAndFee {
spend: lamports,
fee,
},
))
)
}
SpendAmount::RentExempt => {
let mut lamports = if from_pubkey == fee_pubkey {
Expand All @@ -193,13 +217,18 @@ where
from_balance
};
lamports = lamports.saturating_sub(from_rent_exempt_minimum);
Ok((
(
build_message(lamports),
SpendAndFee {
spend: lamports,
fee,
},
))
)
}
};
// After build message, update with correct compute units
if let Some((ix_index, ix_data)) = compute_unit_info {
message.instructions[ix_index].data = ix_data;
}
Ok((message, spend_and_fee))
}
8 changes: 6 additions & 2 deletions cli/src/stake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1437,6 +1437,7 @@ pub fn process_create_stake_account(
let fee_payer = config.signers[fee_payer];
let nonce_authority = config.signers[nonce_authority];

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let authorized = Authorized {
staker: staker.unwrap_or(from.pubkey()),
Expand Down Expand Up @@ -1480,7 +1481,7 @@ pub fn process_create_stake_account(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});
if let Some(nonce_account) = &nonce_account {
Message::new_with_nonce(
Expand All @@ -1503,6 +1504,7 @@ pub fn process_create_stake_account(
&recent_blockhash,
&from.pubkey(),
&fee_payer.pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down Expand Up @@ -1869,6 +1871,7 @@ pub fn process_withdraw_stake(
let fee_payer = config.signers[fee_payer];
let nonce_authority = config.signers[nonce_authority];

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let ixs = vec![stake_instruction::withdraw(
&stake_account_address,
Expand All @@ -1880,7 +1883,7 @@ pub fn process_withdraw_stake(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});

if let Some(nonce_account) = &nonce_account {
Expand All @@ -1902,6 +1905,7 @@ pub fn process_withdraw_stake(
&recent_blockhash,
&stake_account_address,
&fee_payer.pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
6 changes: 4 additions & 2 deletions cli/src/validator_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ pub fn process_set_validator_info(
vec![config.signers[0]]
};

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let keys = keys.clone();
if balance == 0 {
Expand All @@ -364,7 +365,7 @@ pub fn process_set_validator_info(
)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});
instructions.extend_from_slice(&[config_instruction::store(
&info_pubkey,
Expand All @@ -387,7 +388,7 @@ pub fn process_set_validator_info(
)]
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});
Message::new(&instructions, Some(&config.signers[0].pubkey()))
}
Expand All @@ -401,6 +402,7 @@ pub fn process_set_validator_info(
SpendAmount::Some(lamports),
&latest_blockhash,
&config.signers[0].pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
8 changes: 6 additions & 2 deletions cli/src/vote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,7 @@ pub fn process_create_vote_account(
.map_or(false, |feature| feature.activated_at.is_some());
let space = VoteStateVersions::vote_state_size_of(is_feature_active) as u64;

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let vote_init = VoteInit {
node_pubkey: identity_pubkey,
Expand Down Expand Up @@ -855,7 +856,7 @@ pub fn process_create_vote_account(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});

if let Some(nonce_account) = &nonce_account {
Expand All @@ -879,6 +880,7 @@ pub fn process_create_vote_account(
&recent_blockhash,
&config.signers[0].pubkey(),
&fee_payer.pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down Expand Up @@ -1322,6 +1324,7 @@ pub fn process_withdraw_from_vote_account(
let fee_payer = config.signers[fee_payer];
let nonce_authority = config.signers[nonce_authority];

let compute_unit_limit = ComputeUnitLimit::Default;
let build_message = |lamports| {
let ixs = vec![withdraw(
vote_account_pubkey,
Expand All @@ -1332,7 +1335,7 @@ pub fn process_withdraw_from_vote_account(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
});

if let Some(nonce_account) = &nonce_account {
Expand All @@ -1354,6 +1357,7 @@ pub fn process_withdraw_from_vote_account(
&recent_blockhash,
vote_account_pubkey,
&fee_payer.pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
10 changes: 8 additions & 2 deletions cli/src/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,11 @@ pub fn process_transfer(
None
};

let compute_unit_limit = if nonce_account.is_some() {
ComputeUnitLimit::Default
} else {
ComputeUnitLimit::Simulated
};
Comment on lines +914 to +918
Copy link
Copy Markdown

@jstarry jstarry Jun 29, 2024

Choose a reason for hiding this comment

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

Why don't you simulate nonce transactions?

Copy link
Copy Markdown
Author

@joncinque joncinque Jul 15, 2024

Choose a reason for hiding this comment

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

(Sorry for the late response, I was out for a bit)

We took this approach with the token CLI in solana-labs/solana-program-library#6550 to handle offline signers. Since offline signers can't access the network, we can't simulate the transaction before signing, so they'll need to use the default (or eventually static) compute unit limit.

When broadcasting the signed transaction, we need to create exactly the same transaction as the offline signers, so we use the presence of a nonce account as a guess that there are offline signers, since we can't tell from a Signer if it's a pre-signer.

While offline signers and nonces aren't directly related, I couldn't come up with another easy way of knowing if a transaction has pre-signers. Suggestions are very welcome!

Copy link
Copy Markdown

@jstarry jstarry Aug 21, 2024

Choose a reason for hiding this comment

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

Sorry for the delay.. So the situation you describing is broadcasting an already signed transaction where all signers are pre-signers and we almost certainly used a nonce for creating the transaction? And the assumption is that if you're using a nonce it also generally means that this is probably an offline transaction in which you are using pre-signers?

Since the CLI knows whether pre-signers are being used and whether a blockhash was supplied explicitly, we can use those signals to decide whether to simulate or fall back to default. What do you think?

Copy link
Copy Markdown
Author

@joncinque joncinque Aug 21, 2024

Choose a reason for hiding this comment

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

Yeah pretty much, the situation is more specifically if any of the signers was a pre-signer, in which case there's a high probability that it was signed offline.

The main reason that I resisted that approach is that the --signer or --blockhash args are parsed by helpers like signer_from_path and BlockhashQuery, and the top-level normally never references them directly. But you're right, those are probably clearer than just the presence of a nonce account.

If we do go with that approach, we already have the BlockhashQuery and can disable the simulation if it's None or FeeCalculator. For the pre-signers, we need to add another field to all of the command parsers, like contains_presigners, and just check if the --signer arg is present. What do you think?

Copy link
Copy Markdown

@jstarry jstarry Aug 22, 2024

Choose a reason for hiding this comment

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

Hmm after thinking more I think the nonce account approach is a good enough signal for now. The offline signer / supplied blockhash signals are technically not perfect either. Makes me feel all of that could be cleaned up in a separate change but for now this approach is fine.

let build_message = |lamports| {
let ixs = if let Some((base_pubkey, seed, program_id, from_pubkey)) = with_seed.as_ref() {
vec![system_instruction::transfer_with_seed(
Expand All @@ -924,14 +929,14 @@ pub fn process_transfer(
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
})
} else {
vec![system_instruction::transfer(&from_pubkey, to, lamports)]
.with_memo(memo)
.with_compute_unit_config(&ComputeUnitConfig {
compute_unit_price,
compute_unit_limit: ComputeUnitLimit::Default,
compute_unit_limit,
})
};

Expand All @@ -954,6 +959,7 @@ pub fn process_transfer(
&recent_blockhash,
&from_pubkey,
&fee_payer.pubkey(),
compute_unit_limit,
build_message,
config.commitment,
)?;
Expand Down
Loading