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.

Sign up for GitHub

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

Jump to bottom

SIMD-0207: Raise block limit to 50M #4112

Merged
merged 2 commits into from
Dec 16, 2024
Merged

SIMD-0207: Raise block limit to 50M #4112

merged 2 commits into from
Dec 16, 2024

Conversation

apfitzge
Copy link

Problem

Summary of Changes

  • Bank initialization and on epoch boundaries will set limits from feature-set
    • override for u64::MAX in tests/benches

Fixes #

@apfitzge apfitzge self-assigned this Dec 13, 2024
apfitzge
apfitzge commented Dec 13, 2024
View reviewed changes
runtime/src/bank.rs Outdated

// For tests and benches, if the limit is set to maximum then skip
// setting feature-specific limits.
#[cfg(feature = "dev-context-only-utils")]
Copy link
Author

Choose a reason for hiding this comment

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

code comment explains reason for this. Adding comment on review to draw attention.

apfitzge
apfitzge commented Dec 13, 2024
View reviewed changes
runtime/src/bank.rs Outdated
@@ -6829,6 +6829,9 @@ impl Bank {
Arc::new(reserved_keys)
};

// Update the cost-tracker's block limits.
self.update_block_limits();
Copy link
Author

Choose a reason for hiding this comment

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

Whenever we create a completely new bank (from snapshot or genesis) this will set the limits according to the feature-set.

This is also called when we move to a new epoch via new_from_parent.

@apfitzge apfitzge mentioned this pull request Dec 13, 2024
Closed
brooksprumo
brooksprumo reviewed Dec 13, 2024
View reviewed changes
Copy link

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

It looks like with the current implementation, if a test/etc sets the block limit to a value other than default/50m/max, it'll be reset back to default or 50m on the next epoch boundary. Is that the desired behavior?

runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank/tests.rs Outdated Show resolved Hide resolved
runtime/src/bank/tests.rs Outdated Show resolved Hide resolved
@apfitzge
Copy link
Author

It looks like with the current implementation, if a test/etc sets the block limit to a value other than default/50m/max, it'll be reset back to default or 50m on the next epoch boundary. Is that the desired behavior?

Not really, but was trying to keep code simple with the switch in one place.
I'm thinking now it might be better if we have 2 separate update paths:

  • one in feature activations, only applying on new feature activations
  • one in finish_init, which applies OLD features to make sure initial bank has correct costs.

We could then get rid of the special-casing for tests/benches as well. @brooksprumo wdyt?

@brooksprumo
Copy link

I'm thinking now it might be better if we have 2 separate update paths:

  • one in feature activations, only applying on new feature activations
  • one in finish_init, which applies OLD features to make sure initial bank has correct costs.

We could then get rid of the special-casing for tests/benches as well. @brooksprumo wdyt?

This makes sense to me.

@apfitzge
Copy link
Author

@brooksprumo. I made the change discussed above; added some more tests that explicitly test from genesis (same code path for from snapshots).

@brooksprumo brooksprumo self-requested a review December 13, 2024 21:07
brooksprumo
brooksprumo approved these changes Dec 13, 2024
View reviewed changes
Copy link

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

:shipit:

apfitzge
apfitzge commented Dec 13, 2024
View reviewed changes
runtime/src/bank.rs
Comment on lines +5304 to +5318
// Cost-Tracker is not serialized in snapshot or any configs.
// We must apply previously activated features related to limits here
// so that the initial bank state is consistent with the feature set.
// Cost-tracker limits are propagated through children banks.
if self
.feature_set
.is_active(&feature_set::raise_block_limits_to_50m::id())
{
let (account_cost_limit, block_cost_limit, vote_cost_limit) = simd_0207_block_limits();
self.write_cost_tracker().unwrap().set_limits(
account_cost_limit,
block_cost_limit,
vote_cost_limit,
);
}
Copy link
Author

Choose a reason for hiding this comment

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

@tao-stones @bw-solana this block of code here + additional tests are difference from the original PR.
You can look at just the 2nd commit; the first commit was just cherry-picked directly from previous PR.

bw-solana
bw-solana approved these changes Dec 13, 2024
View reviewed changes
Copy link

@bw-solana bw-solana 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 approved these changes Dec 16, 2024
View reviewed changes
Copy link

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

lgtm

@apfitzge apfitzge added the v2.1 Backport to v2.1 branch label Dec 16, 2024
@apfitzge apfitzge merged commit 9e59baa into anza-xyz:master Dec 16, 2024
51 checks passed
@mergify Mergify
Copy link

mergify bot commented Dec 16, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Dec 16, 2024
@apfitzge @mergify
SIMD-0207: Raise block limit to 50M (#4112)
057244e 
* SIMD-0207: Raise block limit to 50M (#4026)

* Fix loading from snapshots/genesis

(cherry picked from commit 9e59baa)

# Conflicts:
#	cost-model/src/block_cost_limits.rs
#	runtime/src/bank.rs
#	sdk/feature-set/src/lib.rs
@mergify mergify bot mentioned this pull request Dec 16, 2024
Open
@bw-solana
Copy link

@apfitzge - Do we have a feature gate tracker for this one? I don't see it on the schedule: https://github.com/anza-xyz/agave/wiki/Feature-Gate-Tracker-Schedule

@apfitzge
Copy link
Author

#4042

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tao-stones tao-stones tao-stones approved these changes

@bw-solana bw-solana bw-solana approved these changes

@brooksprumo brooksprumo brooksprumo approved these changes

Assignees

@apfitzge apfitzge

Labels
v2.1 Backport to v2.1 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apfitzge @brooksprumo @bw-solana @tao-stones