Small code cleanup and typo fixes by ryoqun · Pull Request #13325 · solana-labs/solana

ryoqun · 2020-10-31T08:47:03Z

Problem

There is some outdated code, typo, confusing variable names, spotted by me or audit report information suggestions.

Summary of Changes

Clarify them all. There should be absolutely no functional changes.

These code are extremely important. Let's squeeze out every part of noise and make it beautifully-coded.

Context #13233

mergify · 2020-10-31T08:52:04Z

automerge label removed due to a CI failure

mvines · 2020-10-31T19:21:36Z

If you rebase this PR, it should get past cargo audit

ryoqun · 2020-11-01T05:45:17Z

-            // if stakers do not claim before the epoch goes away they lose the
-            //  credits...


claim-redeem is past thing.

ryoqun · 2020-11-01T05:45:41Z

 pub const INITIAL_LOCKOUT: usize = 2;

 // Maximum number of credits history to keep around
-//  smaller numbers makes


incomplete comment

pub isn't needed (at least for now)

ryoqun · 2020-11-01T05:47:26Z

    ) -> (u128, u64) {
-        if self.credits_observed >= vote_state.credits() {
+        // if there is no newer credits since observed, return no point
+        if new_vote_state.credits() <= self.credits_observed {


Added comment, and swapped the condition operands to match with the comment and read fluently (IMO).

ryoqun · 2020-11-01T05:49:09Z

+        let mut points = 0;
+        let mut new_credits_observed = self.credits_observed;


this is subtle, but let's match with the rhythm here: we're using (point, credits) tuple across this fn

also, the intention of point (without new_) and new_credits_observed (with new_) is that point is isolated and independent for each inflation distribution. So, it's stateless. But credits_observed is continuously updated thing in stake accounts. So, it's state-full. So, only credits_observed can be said there is newer version and (current/older) version.

ryoqun · 2020-11-01T05:55:39Z

+        let mut new_credits_observed = self.credits_observed;
+
+        for (epoch, final_epoch_credits, initial_epoch_credits) in
+            new_vote_state.epoch_credits().iter().copied()


copied() is to remove *s prefixed credits and prev_credits.

ryoqun · 2020-11-01T05:58:59Z

+            new_credits_observed = new_credits_observed.max(final_epoch_credits);
+
+            // finally calculate points for this epoch
+            points += stake * earned_credits;


the main dish of this fn is put at the end, partly preparing for the next actual functional change pr.

yeah, I know this is against https://github.com/solana-labs/solana/pull/13325/files#r515579041 (credits => points here) and the general rule of putting looping-state-related code (maintaining max of new_credits_observed while looping) at the end to avoid intermixed state inside loop body.

ryoqun · 2020-11-01T06:01:41Z

                0
            };
-
-            points += u128::from(self.delegation.stake(*epoch, stake_history))


factor out u128::froms and stake calculation. let's make important definition (point calculation formula) shine by its own: https://github.com/solana-labs/solana/pull/13325/files#r515579738

ryoqun · 2020-11-01T06:20:20Z

+        let mut points = 0;
+        let mut new_credits_observed = self.credits_observed;
+
+        for (epoch, final_epoch_credits, initial_epoch_credits) in


the naming of credits and prev_credits here was a bit unhelpful in this context, imo.

what's important here is that they're together forming an interval because we're calculating overlaps between it and self.credit_observed.

So, I prefixed both of them with prefixes. (they used symmetrically in the logic but the one has a prefix and the other not; that's another point of confution, imo). final_ and initial_ is chosen over last_/first_ and end_/start_:

these epoch_credit interval's bounds are rather more rigid, not arbitrary, like they generally must not overlap with each other and properly ordered, etc. For example, this is not like lockout_intervals. So let's use somewhat formal-sounding wording here.

it's less odd to see (final, initial) order than (end, start) because the final wording has more semantic significance than initial; usually end and start have equal balance. Otherwise, we have to change ABI to make first, last to happen...

ryoqun · 2020-11-01T06:23:37Z

@mvines Could you review this? Now enginner talks about artistic aesthetic. xD These should be fairly safu. but I just want some sanity check since I've added another cleaning: 76a7f8f

codecov · 2020-11-01T06:56:52Z

Codecov Report

Merging #13325 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #13325     +/-   ##
=========================================
- Coverage    82.2%    82.2%   -0.1%     
=========================================
  Files         375      375             
  Lines       87307    87310      +3     
=========================================
+ Hits        71789    71791      +2     
- Misses      15518    15519      +1

mvines

Nice readability improvement!

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c)

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun · 2020-11-27T02:52:06Z

-        if self.credits_observed >= vote_state.credits() {
+        // if there is no newer credits since observed, return no point
+        if new_vote_state.credits() <= self.credits_observed {
            return (0, 0);


;) https://github.com/solana-labs/solana/pull/13836/files#r531357042

ryoqun added automerge Merge this Pull Request automatically once CI passes v1.3 labels Oct 31, 2020

mergify Bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 31, 2020

ryoqun added 2 commits November 1, 2020 14:40

Small code cleanup and typo fixes

0a7b49e

Clean up calculate_points_and_credits

76a7f8f

ryoqun force-pushed the small-cleanup-type-inflation branch from 4a366d4 to 76a7f8f Compare November 1, 2020 05:44

ryoqun commented Nov 1, 2020

View reviewed changes

ryoqun requested a review from mvines November 1, 2020 06:23

mvines approved these changes Nov 1, 2020

View reviewed changes

ryoqun merged commit 0e4509c into solana-labs:master Nov 2, 2020

mergify Bot pushed a commit that referenced this pull request Nov 2, 2020

Small code cleanup and typo fixes (#13325)

49a559d

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c)

mergify Bot mentioned this pull request Nov 2, 2020

Small code cleanup and typo fixes (bp #13325) #13340

Merged

mergify Bot pushed a commit that referenced this pull request Nov 2, 2020

Small code cleanup and typo fixes (#13325)

fcd76ec

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c)

mergify Bot mentioned this pull request Nov 2, 2020

Small code cleanup and typo fixes (bp #13325) #13341

Merged

mergify Bot added a commit that referenced this pull request Nov 2, 2020

Small code cleanup and typo fixes (#13325) (#13340)

958c43f

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

mergify Bot added a commit that referenced this pull request Nov 2, 2020

Small code cleanup and typo fixes (#13325) (#13341)

0bf45cb

* Small code cleanup and typo fixes * Clean up calculate_points_and_credits (cherry picked from commit 0e4509c) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun mentioned this pull request Nov 7, 2020

Clean up Delegation::stake_and_activating #13460

Merged

ryoqun mentioned this pull request Nov 27, 2020

Don't reset credits_observed due to stale voters #13836

Merged

2 tasks

ryoqun commented Nov 27, 2020

View reviewed changes

		// if stakers do not claim before the epoch goes away they lose the
		// credits...

		let mut points = 0;
		let mut new_credits_observed = self.credits_observed;

Conversation

ryoqun commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

mergify Bot commented Oct 31, 2020

Uh oh!

mvines commented Oct 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 1, 2020

Uh oh!

codecov Bot commented Nov 1, 2020

Codecov Report

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryoqun commented Oct 31, 2020 •

edited

Loading