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

Remove unused StakeLockout::lockout#10719

Merged
ryoqun merged 8 commits intosolana-labs:masterfrom
ryoqun:remove-stake-lockout
Jun 23, 2020
Merged

Remove unused StakeLockout::lockout#10719
ryoqun merged 8 commits intosolana-labs:masterfrom
ryoqun:remove-stake-lockout

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Jun 19, 2020

Problem

StakeLockout::lockout isn't used anymore and it does unnecessary computation to mange the field and uses extra memory.

Also, the naming is outdated as a result.

Summary of Changes

Remove the field and tidy up the namings of types and fields.

This PR should contain no functional change.

Part of #10718

@ryoqun ryoqun requested a review from carllin June 19, 2020 14:15
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Jun 19, 2020

Odd.. Why the test fails? I felt StakeLockout::lockout seems to be not used..

Comment thread core/src/consensus.rs Outdated
for slot in slot_with_ancestors {
let entry = &mut stake_lockouts.entry(slot).or_default();
entry.lockout += vote.lockout();
stake_lockouts.insert(slot, StakeLockout::default());
Copy link
Copy Markdown
Contributor

@carllin carllin Jun 19, 2020

Choose a reason for hiding this comment

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

I think the test failures are because this is replacing the slot entry every time (resets stake_lockouts.stake, but it could already exist from previous calls to update_ancestor_stakes 😃 )

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 20, 2020

Codecov Report

Merging #10719 into master will decrease coverage by 0.0%.
The diff coverage is 95.4%.

@@            Coverage Diff            @@
##           master   #10719     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         300      300             
  Lines       70509    70482     -27     
=========================================
- Hits        57660    57622     -38     
- Misses      12849    12860     +11     

Comment thread core/src/consensus.rs
Comment thread core/src/consensus.rs
pub stake_lockouts: HashMap<Slot, StakeLockout>,
pub total_staked: u64,
pub voted_stakes: VotedStakes,
pub total_stake: Stake,
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.

@carllin I've ended up renaming bunch of interrelated fields/types while addressing https://github.com/solana-labs/solana/pull/10719/files#r443351567. How does this look for you, overall?

Also, I've chosen adjuctive noun naming convension because I think this is more natural than total_staked/vote_staked, etc. And the precedent exists: rooted_stake in the existing code (not stake_rooted) and Active Stake: XXX in solana validator (not Active Staked).

Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jun 22, 2020

Choose a reason for hiding this comment

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

Ah, also, I've added Voted intentionally because just Stakes sounded too general.

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.

@ryoqun fine by me!

@ryoqun ryoqun marked this pull request as ready for review June 22, 2020 09:14
@ryoqun ryoqun merged commit 44f5452 into solana-labs:master Jun 23, 2020
danpaul000 pushed a commit to danpaul000/solana that referenced this pull request Jul 8, 2020
* Remove unused StakeLockout::lockout

* Revert...

* Really revert to the original behavior...

* Use consistent naming after StakeLockout removal

* Furhter clean up

* Missed type aliases...

* More...

* Even more...
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.

2 participants