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

Add more comments in Stakes::store for its subtlety#14065

Merged
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:stake-store-comments
Dec 11, 2020
Merged

Add more comments in Stakes::store for its subtlety#14065
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:stake-store-comments

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Dec 11, 2020

Problem

Stakes::store() does its elegance without much murmuring regarding the handling of proper stale state prevention for stakes and votes.

Summary of Changes

Make it chatty for peaceful future humanity.

no functional change.

Fixes #

Comment thread runtime/src/stakes.rs
// unconditionally remove existing at first; there is no dependent calculated state for
// votes, not like stakes (stake codepath maintains calculated stake value grouped by
// delegated vote pubkey)
let old = self.vote_accounts.remove(pubkey);
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.

here.

Comment thread runtime/src/stakes.rs
}
None
} else {
// there is no need to remove possibly existing Stakes cache entries with given
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.

@CriesofCarrots Account::default() was hitting here in #14062.

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 ryoqun changed the title Stake store comments Add more comments in Stakes::store for its subtlety Dec 11, 2020
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Dec 11, 2020

@bennofs I hope you glanced this code, considering #14062 finding. Could you check my sanity as a neutral outsider? There should be no hole anymore for Stakes with #14062 (@CriesofCarrots thanks for working on this) :)

Also note that this is possibly reward-able effort. I cannot earn attack bonus; but you can by pointing my overlook.. :)

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 11, 2020

Codecov Report

Merging #14065 (5ca8f7a) into master (4fba7e6) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #14065     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         384      384             
  Lines       94601    94601             
=========================================
- Hits        77718    77712      -6     
- Misses      16883    16889      +6     

@ryoqun ryoqun added the v1.4 label Dec 11, 2020
@ryoqun ryoqun merged commit 7078a6a into solana-labs:master Dec 11, 2020
mergify Bot pushed a commit that referenced this pull request Dec 11, 2020
* Add more comments in Stakes::store for its subtlety

* more comment tweak

(cherry picked from commit 7078a6a)
mergify Bot added a commit that referenced this pull request Dec 11, 2020
* Add more comments in Stakes::store for its subtlety

* more comment tweak

(cherry picked from commit 7078a6a)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
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