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

add compute budget instruction to set loaded accounts data size limit #30377

Conversation

tao-stones
Copy link
Contributor

Problem

Need instruction for users to set per transaction accounts data size limit

Summary of Changes

  • add compute budget instruction, with 64MB as Max (current hard coded limit)
  • feature gated
  • updated call sites

Feature Gate Issue: #30366

@tao-stones tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label Feb 16, 2023
@tao-stones tao-stones force-pushed the feature-add-instruction-set-tx-data-size branch from 7217688 to db2716d Compare February 16, 2023 22:19
Copy link
Contributor Author

@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.

Soliciting inputs:

sdk/src/compute_budget.rs Outdated Show resolved Hide resolved
runtime/src/transaction_priority_details.rs Show resolved Hide resolved
program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved
@tao-stones tao-stones force-pushed the feature-add-instruction-set-tx-data-size branch 2 times, most recently from 9537944 to 364ee9f Compare February 17, 2023 17:02
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

After working on various issues related to setting maximum allocation limits [1], would it beneficial to be more explicit with the names to indicate these limits/instructions are for loading account data (not allocating)?

Or/also, do you anticipate a similar instruction/limit for requesting/setting an allocation limit?

[1] #27375

@tao-stones tao-stones force-pushed the feature-add-instruction-set-tx-data-size branch from 364ee9f to e6b2efc Compare February 22, 2023 22:07
@tao-stones tao-stones force-pushed the feature-add-instruction-set-tx-data-size branch from 0889e9b to aee61cb Compare February 22, 2023 22:44
@tao-stones
Copy link
Contributor Author

would it beneficial to be more explicit with the names to indicate these limits/instructions are for loading account data (not allocating)?

Good call, renamed to be explicit

Or/also, do you anticipate a similar instruction/limit for requesting/setting an allocation limit?

afaik there is no plan for it yet. It is possible to add another instruction to allow request allocation limit when needed.

@Lichtso Lichtso changed the title add compute budget instruction to set accounts data size limit add compute budget instruction to set loaded accounts data size limit Feb 23, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I can't find where the load limit is used. Is that already implemented, or will that come in a future PR?

docs/src/developing/programming-model/runtime.md Outdated Show resolved Hide resolved
program-runtime/src/compute_budget.rs Show resolved Hide resolved
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I can't find where the load limit is used. Is that already implemented, or will that come in a future PR?

@tao-stones
Copy link
Contributor Author

I can't find where the load limit is used. Is that already implemented, or will that come in a future PR?

currently the limit is hardcoded 64M at here. This PR is to introduce the request ix, after it's landed, will have a follow up PR to replace the hardcoded 64MB with the ix. The overarching goal is to charge payer for the bytes requested to loaded. If user doesn't specify the loaded data limit, then 64MB is used, and charged accordingly. To reduce cost, user can use this instruction to request lesser (and more accurate) bytes per tx.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@tao-stones tao-stones merged commit 66ea750 into solana-labs:master Feb 24, 2023
@@ -41,6 +41,8 @@ pub enum ComputeBudgetInstruction {
/// Set a compute unit price in "micro-lamports" to pay a higher transaction
/// fee for higher transaction prioritization.
SetComputeUnitPrice(u64),
/// Set a specific transaction-wide account data size limit, in bytes, is allowed to load.
SetLoadedAccountsDataSizeLimit(u32),
Copy link
Contributor

Choose a reason for hiding this comment

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

😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sad because no TLV here? I bounced around the idea with multiple parties, as I wasn't fully convinced that mixed use of TLV and whatever-we-have will be satisfying. The consensus is mainly on "less programs, more instructions per program". So i decided to keep this program consistent in style, leave TLV to next new program (say ApplicationFee :D )

Copy link
Contributor

Choose a reason for hiding this comment

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

TLV means what?

Copy link
Contributor

Choose a reason for hiding this comment

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

TLV means what?

i had proposed that we stop adding new compute budget instructions for each requestable parameter and instead add one final compute budget instruction that takes as data a TLV structure similar to what token22 uses for extensions. idea here being that the instructions add both space and compute overhead to transactions and their process. also, in a future transaction format version, the TLV structure could be made a first-class citizen, eliminating the need for compute budget instructions and the compute budget program at all

nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Mar 12, 2023
…solana-labs#30377)

* add compute budget instruction to set accounts data size limit

* changes names to explicitly for loaded accounts data size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants