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

cleanup feature code after mainnet-beta activation of DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V #34089

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

tao-stones
Copy link
Contributor

Problem

feature DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V #27839 has been activated everywhere.

Summary of Changes

cleanup feature code

Fixes #

@tao-stones
Copy link
Contributor Author

solana feature status DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V
Feature                                      | Status                  | Activation Slot | Description
DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V | pending until epoch 533 | NA              | cap transaction accounts data size up to a limit #27839

solana feature status DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V -ud
Feature                                      | Status                  | Activation Slot | Description
DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V | active since epoch 557  | 240624000       | cap transaction accounts data size up to a limit #27839

solana feature status DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V -ut
Feature                                      | Status                  | Activation Slot | Description
DdLwVYuvDz26JohmgSbA7mjpJFgX5zP2dkp8qsF2C33V | active since epoch 516  | 217388260       | cap transaction accounts data size up to a limit #27839

@brooksprumo
Copy link
Contributor

Over on Discord it was proposed that we don't immediately clean up feature gate code1, and instead wait until the next version rolls out on mnb. This would allow master to always replay a ledger created by stable, which is a common use-case for the VM/runtime team.

I see this feature gate has already moved to the "awaiting cleanup" section2 on the feature gate activation schedule as well.

It seems reasonable to wait to clean up featurization if it helps another team. It also seems that this feature gate is quite localized and thus won't dramatically reduce complexity by removing the featurization code (which would be a reason to justify removing the code now vs later).

Wdyt about waiting?

Footnotes

  1. https://discord.com/channels/428295358100013066/910937142182682656/1165996903163498556

  2. https://github.com/solana-labs/solana/wiki/Feature-Gate-Activation-Schedule#activated-awaiting-cleanup

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #34089 (09bb723) into master (39f2866) will increase coverage by 0.0%.
Report is 9 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34089   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         819      819           
  Lines      220912   220904    -8     
=======================================
+ Hits       180865   180911   +46     
+ Misses      40047    39993   -54     

@tao-stones
Copy link
Contributor Author

Thanks for pointing it out, totally agree. Just want to put out the PR avoid another content switch. Change it to Draft until Feature Activation Schedule rolls this into "do clean up" stage.

@tao-stones tao-stones marked this pull request as draft November 15, 2023 18:35
@brooksprumo
Copy link
Contributor

Just want to put out the PR avoid another content switch.

I just did the same thing, so I totally understand.

Change it to Draft until Feature Activation Schedule rolls this into "do clean up" stage.

Sounds good. I'm going to remove myself as a reviewer for now. Once the clean up stage is open, please do re-tag me to review. Thanks!

@brooksprumo brooksprumo removed their request for review November 17, 2023 14:44
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 4, 2023
@github-actions github-actions bot closed this Dec 12, 2023
@tao-stones tao-stones reopened this Dec 12, 2023
@tao-stones tao-stones removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 12, 2023
@tao-stones tao-stones marked this pull request as ready for review December 12, 2023 22:44
@brooksprumo
Copy link
Contributor

Over on Discord it was proposed that we don't immediately clean up feature gate code, and instead wait until the next version rolls out on mnb. This would allow master to always replay a ledger created by stable, which is a common use-case for the VM/runtime team.

Welp, looks like this was reversed, lol.
https://discord.com/channels/428295358100013066/910937142182682656/1184177692745142313

I'll re-review this PR. It also looks like there are file conflicts that need to be resolved now.

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. Please re-request a review after conflicts are resolved.

@tao-stones
Copy link
Contributor Author

tao-stones commented Dec 13, 2023

Over on Discord it was proposed that we don't immediately clean up feature gate code, and instead wait until the next version rolls out on mnb. This would allow master to always replay a ledger created by stable, which is a common use-case for the VM/runtime team.

Welp, looks like this was reversed, lol.

Yeah, the flood gate is now open 😜 (... and all the rebases)

@tao-stones tao-stones force-pushed the cleanup-feature-27839 branch from 667a01d to 2c229ca Compare December 13, 2023 04:05
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 2dfcdce into solana-labs:master Dec 13, 2023
34 checks passed
@tao-stones tao-stones deleted the cleanup-feature-27839 branch December 13, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants