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

v2.1: Accounting migrating builtin programs default Compute Unit Limit with feature status (backport of #3975) #4091

Open
wants to merge 3 commits into
base: v2.1
Choose a base branch
from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Dec 12, 2024

Problem

When a builtin program migrated into sbpf, it should no longer be considered as 'builtin', therefore its default Compute Budget Limit should change from 3K to 200K CU, per #3799. It is currently not supported by Compute Budget Instruction processing.

Summary of Changes

  • Add fixed-length (currently 3 entries) vector MigrationBuiltinFeatureCounter to transaction static meta;
  • Counts migrating builtin features gates during try_from()
  • Check feature_set to resolve if builtin has migrated during calculate_default_compute_unit_limit() if Compute Unit Limit was not requested.

Fixes #


This is an automatic backport of pull request #3975 done by [Mergify](https://mergify.com).

@mergify mergify bot requested a review from a team as a code owner December 12, 2024 21:46
@mergify mergify bot added the conflicts label Dec 12, 2024
Copy link
Author

mergify bot commented Dec 12, 2024

Cherry-pick of 9379fbc has failed:

On branch mergify/bp/v2.1/pr-3975
Your branch is up to date with 'origin/v2.1'.

You are currently cherry-picking commit 9379fbcba4.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   Cargo.lock
	modified:   builtins-default-costs/Cargo.toml

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   builtins-default-costs/src/lib.rs
	deleted by us:   compute-budget-instruction/Cargo.toml
	deleted by us:   compute-budget-instruction/src/builtin_programs_filter.rs
	both modified:   runtime-transaction/src/compute_budget_instruction_details.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@tao-stones
Copy link

tao-stones commented Dec 12, 2024

Did first round of conflicts fix. Need to rebase after #3931 is merged to 2.1

@tao-stones tao-stones force-pushed the mergify/bp/v2.1/pr-3975 branch 2 times, most recently from a643774 to 4321b8f Compare December 13, 2024 17:12
@t-nelson
Copy link

bit much for a backport innit?

@tao-stones
Copy link

bit much for a backport innit?

Ya it is. It is currently an accumulation of #3931 and the actual BP. Once #3931 is backported, I'll rebase this one then invite reviwers.

@tao-stones tao-stones force-pushed the mergify/bp/v2.1/pr-3975 branch from 4321b8f to ad676fb Compare January 9, 2025 22:10
@tao-stones
Copy link

running on ds2 since slot 313127967, epoch 724

tao-stones and others added 3 commits January 11, 2025 11:05
… feature status (#3975)

* Accounting migrating builtin programs default Compute Unit Limit with its feature gate status

* Declare Non/migrating buiiltins in const array, eleminates heap allocation (Vec<>) per transaction

* updates for review commients

add explicit positional information to migrating builtin feature obj

update developer notes, added static_assertion to validate no new items are added, add enum type

* use enum to separately define migrating and not-migrating builtins

* rename for clarity

(cherry picked from commit 9379fbc)
@tao-stones tao-stones force-pushed the mergify/bp/v2.1/pr-3975 branch from ad676fb to f77bd52 Compare January 11, 2025 17:06
@t-nelson
Copy link

Screenshot 2025-01-11 at 10 26 01 AM

why? stop 😭

if you need to bp another pr, bp another pr. stop piling stuff into what's already unreviewable

@tao-stones
Copy link

Screenshot 2025-01-11 at 10 26 01 AM why? stop 😭

if you need to bp another pr, bp another pr. stop piling stuff into what's already unreviewable

Ah, I'm sorry. You are right that #3768 should be bp-ed separately, if it's restrictively needed. I'll fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants