Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Clean up before credits_auto_rewind#22839

Merged
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:credits-auto-rewind-cleanup
Feb 1, 2022
Merged

Clean up before credits_auto_rewind#22839
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:credits-auto-rewind-cleanup

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Jan 31, 2022

Problem

it's desired to clean some code upfront before the staking reward code tweak (#22546 ) in order to minimize the risk and cognitive load when reviewing such a critical code change.

Summary of Changes

Do the cleanup as a preparatory pr; there should be no functionality change in this pr in any way.

Fixes #

// Drive credits_observed forward unconditionally when rewards are disabled
// or when this is the stake's activation epoch
if point_value.rewards == 0
|| (fix_activating_credits_observed && stake.delegation.activation_epoch == rewarded_epoch)
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jan 31, 2022

Choose a reason for hiding this comment

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

I'm intentionally removing the sole use of this obsolete feature guard ( fix_activating_credits_observed #19309 ) because i believe this has been activated across board already (mainnet-beta, testnet, devnet). CC: @joncinque

On the other hand, i'm trying to reuse its plumbing for another my feature (ref: #22546 ). so, I'm keeping the plumbing intentionally as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, it's been active for awhile, so feel free to remove it

@ryoqun ryoqun added the CI Pull Request is ready to enter CI label Jan 31, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jan 31, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 31, 2022

Codecov Report

Merging #22839 (6ea8189) into master (13e631d) will decrease coverage by 0.0%.
The diff coverage is 78.2%.

@@            Coverage Diff            @@
##           master   #22839     +/-   ##
=========================================
- Coverage    81.2%    81.2%   -0.1%     
=========================================
  Files         560      560             
  Lines      151332   151340      +8     
=========================================
- Hits       122980   122978      -2     
- Misses      28352    28362     +10     

Comment thread programs/stake/src/stake_state.rs Outdated
if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() {
inflation_point_calc_tracer(&SkippedReason::DisabledInflation.into());
}
forced_credits_update_with_skipped_reward |= true;
Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots Jan 31, 2022

Choose a reason for hiding this comment

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

Any (upcoming) reason this needs to be |= instead of just =? Seems more confusing than helpful in the case of a mutable bool, and the two cases won't ever both be evaluated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well, there is no reason. changed to =: 6ea8189

Comment thread programs/stake/src/stake_state.rs Outdated
if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() {
inflation_point_calc_tracer(&SkippedReason::JustActivated.into());
}
forced_credits_update_with_skipped_reward |= true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ryoqun ryoqun requested a review from CriesofCarrots February 1, 2022 02:27
Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

lgtm!

@ryoqun ryoqun merged commit 545c97f into solana-labs:master Feb 1, 2022
mergify Bot pushed a commit that referenced this pull request Feb 1, 2022
* Clean up before credits_auto_rewind

* Use `=` intead of `|=` for mutable bool

(cherry picked from commit 545c97f)
mergify Bot pushed a commit that referenced this pull request Feb 1, 2022
* Clean up before credits_auto_rewind

* Use `=` intead of `|=` for mutable bool

(cherry picked from commit 545c97f)
mergify Bot added a commit that referenced this pull request Feb 1, 2022
* Clean up before credits_auto_rewind

* Use `=` intead of `|=` for mutable bool

(cherry picked from commit 545c97f)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
mergify Bot added a commit that referenced this pull request Feb 1, 2022
* Clean up before credits_auto_rewind

* Use `=` intead of `|=` for mutable bool

(cherry picked from commit 545c97f)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 24, 2022
* Clean up before credits_auto_rewind

* Use `=` intead of `|=` for mutable bool
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants