Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allocate builtin instructions budget with its actual cost #170
base: main
Are you sure you want to change the base?
Allocate builtin instructions budget with its actual cost #170
Changes from 1 commit
2406ddb
46d0702
22594b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to take this approach of using static costs per builtin program, and alternative is to just fix the implementation of CU usage in execution.
If our cost-model says that builtin program Z always uses X CUs, then that should be what is actually used by the execution, regadless of what it does internally, including CPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledging that several alternatives exist, this proposal focuses on the specific issue: "If the VM must consume builtin.default_compute_units, then it should allocate exactly that amount for the builtin, rather than a fixed 200,000 units." This approach addresses block overpacking in the short term, while longer-term solutions are still being explored.
To clarify, it's the cost model being told that a builtin program Z always uses X CUs.
Yea, that'd be ideal. I am not 100% how and when this will happen tho. Happy to discuss more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(agave implementation specific)
I believe we'd need to modify the
declare_process_instruction
macro so that we can somehow have a call that doesn't consume CUs and one that does - either through some flag or separate call.Calls from inside our builtins would then need to call the correct one that does not consume.
Would also need to make sure that actual user-code cannot call into the non-consuming version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apfitzge you can just provide zero and it becomes a no-op.
https://github.com/anza-xyz/agave/blob/a72f981370c3f566fc1becf024f3178da041547a/program-runtime/src/invoke_context.rs#L71-L76
A good standard would be to follow the ZK proof program's instructions (shared previously by @tao-stones). We would just need to sort out how to represent the constants to get some kind of builtin-program dictionary of CUs per instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then I think then it'd always consume 0, since it is a macro defining a function, not a function itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, sorry, I don't think I was specific enough to describe what I meant. Yeah, the generated code consumes zero if you do that, but the salient bit from the ZK program I was intending to share was where each instruction does a manual consumption.
https://github.com/anza-xyz/agave/blob/e207c6e0eaf8e1657fbfaff07da05ca6a7928349/programs/zk-token-proof/src/lib.rs#L205-L208
Globally, default CU consumption is zero, but each instruction consumes its own individual value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's already plans underway to move the builtins to bpf.
at that point it'd effectively be the same as this alternative since builtins would be treated the same as other user programs - (?) possible exception for compute-budget which is a configuration not an instruction (imo).
Thus we will need some way to address the over-reserving described here.
There have been discussions on ramping down the default ix CUs over time, what is the issue with such an approach now if it is eventually the path we have to take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if moving builtins to bpf solves the particular issue (mentioned above) this proposal aims in immediate term, cause still need to allocate "proper" budget for builtin, or any instruction in that scenario.
But lower default cu/ix will help - lowered default per ix will ease over reserving. What would the new "default" be is arbitrary. In extreme case, if it is lowered to
0
, we'd be in the ideal world that every transaction will have to set (in other words, configure) its cu limit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but I think there's simplicity in saying "all programs are treated the same" vs "builtins are treated differently AND these specific builtin instructions are treated even different from that"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. to be at "all programs are treated the same" in terms of allocating budget for VM, it needs to shift to 100% request-based mode. More tools need to be built to help devs to request cu limits easily and accurately, also lower the default cu/ix when < 100% transaction requests cu limit. This proposal serves stopgap before all that land.