Add builtins crate #3866
Add builtins crate #3866
Conversation
builtins/src/lib.rs
Outdated
| enable_feature_id: Some(feature_set::zk_elgamal_proof_program_enabled::id()), | ||
| program_id: solana_sdk_ids::zk_elgamal_proof_program::id(), | ||
| entrypoint: solana_zk_elgamal_proof_program::Entrypoint::vm, | ||
| native_cu_cost: 0, // TODO, these are per-instruction. |
There was a problem hiding this comment.
@tao-stones the two ZK builtins are going to be tricky since they are metered per-instruction.
There was a problem hiding this comment.
Yea, I don't know what to do with ZK builtins, they are per instruction and they consume much more CUs than other regular builtins. We could consider them as "not builtins" so to allocate 200K by default (Assume users of ZK will set cu-limit sensibly); or we can speed up SIMD-198 to make all remaining builtins per instruction as well (tho that does not address the issue that original builtins consumes hundreds of CUs, while ZK takes thousands of CUs)
There was a problem hiding this comment.
Well, you originally didn't have them in the list, so should they not be included here at all? Or is setting them to zero okay?
There was a problem hiding this comment.
There was no ZK when the list was created :) I think ZK builtins should be included in the list with "MAX(per instruction cost)"; then:
- before SIMD-170 (eg, current behavior): cost mode reserve enough CUs for all its instructions, maybe a bit over reserving for some, but that is not new. VM's CU Meter always give default 200K for it.
- after SIMD-170, both cost model and CU meter don't care about
native_cu_costanymore, they will give 3K CU for it, if user didn't request CU Limit, therefore fails TX.
There was a problem hiding this comment.
I don't really follow what you're suggesting we do in your comment above. But in any case, this PR should definitely not be changing the static list of builtins without a feature gate.
There was a problem hiding this comment.
Not disagree that the static list shouldn't be changed without feature gate. But technically can still do that before #3799 feature gate is activated.
There was a problem hiding this comment.
Yes we could but I don't think a crate refactoring PR is the right time do that. If we want to update the list before SIMD-0170 is activated we would need to backport the change to v2.0 and v2.1 and update the list of builtin id's included in the SIMD-0170 proposal details.
buffalojoec
force-pushed
the
joec-builtins-crate
branch
2 times, most recently
from
d3b5f51 to
5cfd990
Compare
buffalojoec
force-pushed
the
joec-builtins-crate
branch
from
5cfd990 to
335f411
Compare
|
@jstarry @tao-stones @LucasSte I've popped the last commit (where I modify the CU default cost list) from this PR. I think moving the list of builtins to its own crate is useful for a few reasons. It gives us, and other developers, the ability to access this list, the builtin entrypoint functions, and their respective feature gates (both for enabling and migrating) without having to pull in the whole runtime. In another follow-up PR, I'll address the cost model problem more directly and see what you guys think. |
buffalojoec
force-pushed
the
joec-builtins-crate
branch
from
335f411 to
e545cdf
Compare
|
cc @yihau |
Problem
The new changes surrounding allocation of minimum required compute units for builtin programs in the cost model have realized a need to track the status of a builtin's migration to BPF. However, currently we are keeping these migration feature gates in two separate lists.
Summary of Changes
Introduce a
solana-builtinscrate, which can be the single source of truth for the list of builtins, including their migration feature ID and their CU designation.