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

Remove partitioned reward test CLI and Config #3949

Closed

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Dec 5, 2024

Problem

Partitioned epoch rewards feature has been activated. The test cli and config are no longer needed. Clean it up to make the code simpler.

Summary of Changes

  • remove partitioned reward test code

Fixes #

@HaoranYi HaoranYi marked this pull request as draft December 5, 2024 16:17
@HaoranYi HaoranYi changed the title remove partitioned reward test code Remove partitioned reward test CLI and Config Dec 5, 2024
@HaoranYi HaoranYi force-pushed the remove_partitioned_reward_test_code branch 4 times, most recently from 4d7ce6a to 8e49e58 Compare December 5, 2024 18:59
@HaoranYi HaoranYi force-pushed the remove_partitioned_reward_test_code branch from 8e49e58 to d45aaf6 Compare December 19, 2024 18:22
@HaoranYi HaoranYi marked this pull request as ready for review December 19, 2024 18:22
@brooksprumo
Copy link

brooksprumo commented Dec 19, 2024

IMO it would be nice to wait until 2.3 before removing the featurization code. Otherwise, if a private cluster is running and hasn't activated P.E.R. yet, when a node upgrades to master, it'll diverge. By waiting one additional version, then we know builds of edge (2.2) always work with builds of stable (2.0).

For this PR, is this strictly the testing/experimental code though?

@HaoranYi
Copy link
Author

This pr didn't remove featurization code it only removes test harness code and test cli and a few hacks.

@HaoranYi
Copy link
Author

HaoranYi commented Dec 19, 2024

For this PR, is this strictly the testing/experimental code though?

Yes.
I think the cli maybe obsolete. If we run the validator with the cli now on mainnet, we may get hash mismatch because of the hack we put in to explicitly exclude rewards sysvar ...
I will double check...

@CriesofCarrots
Copy link

CriesofCarrots commented Dec 19, 2024

IMO it would be nice to wait until 2.3 before removing the featurization code. Otherwise, if a private cluster is running and hasn't activated P.E.R. yet, when a node upgrades to master, it'll diverge. By waiting one additional version, then we know builds of edge (2.2) always work with builds of stable (2.0).

For this PR, is this strictly the testing/experimental code though?

@brooksprumo Is this a new policy? cc @willhickey
I believe the documented process is to remove in master when the last level (MNB) has been activated, so remove in edge when activation happened on stable. (I'm asking because I've been working on removing the featurization code: https://github.com/CriesofCarrots/solana/tree/cleanup-per)

I think adding an additional minor-version delay is arbitrary. Consider the situation when v2.1 becomes stable and we've removed the PER feature in v2.3 (edge). A private cluster running v2.1 but without PER activated will experience the same divergence updating to master. That is to say, there isn't any way to ever remove featurization code and guarantee builds of edge always work with builds of stable. In every case, we must depend on users to emulate MNB activation sets.

@willhickey
Copy link

Is this a new policy?
I think the policy has just been compatibility between adjacent minor versions, so removing v2.0 feature gated code in master is fine. But teams could choose to do cleanup slower than that, if they want

@@ -5676,16 +5661,13 @@ impl Bank {
let measure_total = Measure::start("");

let slot = self.slot();
let ignore = (!self.is_partitioned_rewards_feature_enabled()
Copy link
Author

@HaoranYi HaoranYi Dec 19, 2024

Choose a reason for hiding this comment

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

It looks like the test CLI will work fine even when the feature is active.

@HaoranYi
Copy link
Author

HaoranYi commented Dec 19, 2024

For this PR, is this strictly the testing/experimental code though?

Yes. I think the cli maybe obsolete. If we run the validator with the cli now on mainnet, we may get hash mismatch because of the hack we put in to explicitly exclude rewards sysvar ... I will double check...

ah. It will be fine to run validator with those test CLI.

@HaoranYi
Copy link
Author

It looks like @CriesofCarrots's cleanup_pr branch already covered the changes in this PR.

Let's close this one.

@HaoranYi HaoranYi closed this Dec 19, 2024
@brooksprumo
Copy link

@brooksprumo Is this a new policy?

and

I think adding an additional minor-version delay is arbitrary.

No, not a new policy, and yes, it is arbitrary.

Mostly I got bit by this on a pop-net cluster when TVC activated. I upgraded one node that was running the same minor version (2.2/master in this case), and then it stopped working because TVC's featurization was removed promptly after it was activated on mnb.

@CriesofCarrots
Copy link

It looks like @CriesofCarrots's cleanup_pr branch already covered the changes in this PR.

Let's close this one.

Actually, I hadn't done any of the CLI or accountsDB cleanup, so I was going to approve this. Okay if I cherry-pick your commit instead, @HaoranYi ?

@HaoranYi
Copy link
Author

It looks like @CriesofCarrots's cleanup_pr branch already covered the changes in this PR.
Let's close this one.

Actually, I hadn't done any of the CLI or accountsDB cleanup, so I was going to approve this. Okay if I cherry-pick your commit instead, @HaoranYi ?

Yes, please cherry pick it to your branch.

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.

4 participants