Skip to content

cli: Simulate for compute units consumed during transfer#1923

Merged
joncinque merged 4 commits intoanza-xyz:masterfrom
joncinque:clisim
Aug 22, 2024
Merged

cli: Simulate for compute units consumed during transfer#1923
joncinque merged 4 commits intoanza-xyz:masterfrom
joncinque:clisim

Conversation

@joncinque
Copy link
Copy Markdown

Problem

As part of the ongoing work to properly size transactions, as in #1810, the CLI still isn't simulating the transaction in most cases to find out the compute units consumed, causing users to pay too much in fees when a compute unit price is set.

Summary of Changes

This PR focuses on the resolve_spend_message case, which can also support the ALL keyword. To simulate the transaction only once and fetch the right fee, the message is modified similarly to this program deployment logic:

agave/cli/src/program.rs

Lines 2949 to 2963 in 51af772

if let UpdateComputeUnitLimitResult::UpdatedInstructionIndex(ix_index) =
simulate_and_update_compute_unit_limit(&rpc_client, &mut message)?
{
for msg in &mut write_messages {
// Write messages are all assumed to be identical except
// the program data being written. But just in case that
// assumption is broken, assert that we are only ever
// changing the instruction data for a compute budget
// instruction.
assert_eq!(msg.program_id(ix_index), Some(&compute_budget::id()));
msg.instructions[ix_index]
.data
.clone_from(&message.instructions[ix_index].data);
}
}

The only actual change to behavior is for solana transfer -- the rest will be done in smaller PRs which look just like the last two commits of this PR.

Fixes #

@joncinque joncinque requested a review from jstarry June 28, 2024 22:43
Comment thread cli/src/wallet.rs
Comment on lines +914 to +918
let compute_unit_limit = if nonce_account.is_some() {
ComputeUnitLimit::Default
} else {
ComputeUnitLimit::Simulated
};
Copy link
Copy Markdown

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

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

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

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

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.

@joncinque joncinque merged commit 0da63a3 into anza-xyz:master Aug 22, 2024
@joncinque joncinque deleted the clisim branch August 22, 2024 09:47
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* cli: Plumb down `ComputeUnitLimit` to resolve tx

* Use simulated compute units in transaction

* wallet: Use simulated compute units for non-nonce transactions

* Modify transfer test
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