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

Fix - Bank::compute_active_feature_set() and Bank::apply_feature_activations() #34124

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Nov 16, 2023

Problem

A feature activation on devnet diverged because it was activated before the epoch boundary on part of the cluster (version dependent). This happened because of the backport #34003 which depends on Bank::compute_active_feature_set() being without side effects (as the name suggests).

Summary of Changes

Move the actual activation of pending features from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

…ure_set() into Bank::apply_feature_activations().
@Lichtso Lichtso added the v1.17 PRs that should be backported to v1.17 label Nov 16, 2023
@steviez
Copy link
Contributor

steviez commented Nov 16, 2023

I'm inclined to think this unit test will now fail:

fn test_compute_active_feature_set() {

Aside from Bank::apply_feature_activations(), this unit test looks to be the only other caller of Bank::compute_active_feature_set() so that unit test should be the only other things that needs to be updated

@pgarg66
Copy link
Contributor

pgarg66 commented Nov 16, 2023

I'm inclined to think this unit test will now fail:

fn test_compute_active_feature_set() {

Aside from Bank::apply_feature_activations(), this unit test looks to be the only other caller of Bank::compute_active_feature_set() so that unit test should be the only other things that needs to be updated

You're right. That test is failing.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #34124 (0942241) into master (b4c652e) will decrease coverage by 0.1%.
Report is 8 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34124     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         816      816             
  Lines      219753   219759      +6     
=========================================
+ Hits       180001   180004      +3     
- Misses      39752    39755      +3     

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Lichtso Lichtso merged commit 6b85450 into master Nov 17, 2023
32 checks passed
@Lichtso Lichtso deleted the fix/compute_active_feature_set branch November 17, 2023 08:54
mergify bot pushed a commit that referenced this pull request Nov 17, 2023
…ctivations()` (#34124)

* Moves modification of feature accounts from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

* Renames allow_new_activations and newly_activated to include_pending and pending.

* Fix test_compute_active_feature_set.

(cherry picked from commit 6b85450)
Lichtso added a commit that referenced this pull request Nov 17, 2023
…ature_activations()` (backport of #34124) (#34136)

Fix - `Bank::compute_active_feature_set()` and `Bank::apply_feature_activations()` (#34124)

* Moves modification of feature accounts from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

* Renames allow_new_activations and newly_activated to include_pending and pending.

* Fix test_compute_active_feature_set.

(cherry picked from commit 6b85450)

Co-authored-by: Alexander Meißner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants