Stake: Support merging fully-activated stake accounts by t-nelson · Pull Request #13712 · solana-labs/solana

t-nelson · 2020-11-19T21:55:18Z

Problem

Only deactivated stake accounts can be merged. This poses a dilemma for automated staking tools in that they can only adjust a delegation downward without managing an ever-growing pool of accounts. It also exposes an unexpected asymmetry with stake-split, which can be performed on delegated stake accounts

Summary of Changes

Minor refactor and cosmetics
Support merging two stake account if they are fully-active and their delegations are compatible

t-nelson · 2020-11-19T21:56:40Z

WIP while I write tests. Works as expected on a local-cluster though!

In the mean time, it'd be great to get a gut-check from @rwalker-com that this isn't utterly stupid for some obscure reason 🙏

rwalker-apple · 2020-11-19T23:03:50Z

+
+    fn can_merge_with(&self, source: &Self) -> bool {
+        self.voter_pubkey == source.voter_pubkey
+            && (self.warmup_cooldown_rate - source.warmup_cooldown_rate).abs() < f64::EPSILON


you also want to verify that they were activated at the same time or are fully activated?

Hey! Thanks for taking a peek! 🙂

I wasn't planning on handling the transient states. The fully-activated check is down here https://github.com/solana-labs/solana/pull/13712/files#diff-ab7997cf6770a6fae7b258e433b5dd8c33afa30431eab212552bc7f2b7b38db1R1164

t-nelson · 2020-11-21T06:39:51Z

I think this one's ready for final review now

codecov · 2020-11-21T07:50:26Z

Codecov Report

Merging #13712 (4b9413d) into master (f0f99ff) will increase coverage by 0.0%.
The diff coverage is 99.4%.

@@           Coverage Diff            @@
##           master   #13712    +/-   ##
========================================
  Coverage    82.1%    82.2%            
========================================
  Files         381      381            
  Lines       91992    92340   +348     
========================================
+ Hits        75613    75951   +338     
- Misses      16379    16389    +10

t-nelson · 2020-11-21T18:21:08Z

I'll try to rethink the organization of the helper functions in light of the rest of the comments 🤔

CriesofCarrots

Thanks for the extensive tests. I agree with @ryoqun that some expanded comments would help. Otherwise, looking good to me!
I'll take another pass when you make whatever helper fn changes you land on.

t-nelson

Ok, last round I think 😅

ryoqun · 2020-11-23T06:42:23Z

Codecov Report

Merging #13712 (9c9f2b5) into master (1c7dd0a) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Cool, '100.0%'. I sense you have a good stamina to attain very strong test coverage.

ryoqun

LGTM with nits.

Happily, I don't have anything anymore other than small renaming. What a solid work done from you. Also thank for enduring my persistent and long review window :)

CriesofCarrots

lgtm w/ a couple nits as well!

t-nelson · 2020-11-23T18:17:27Z

Codecov Report

Merging #13712 (9c9f2b5) into master (1c7dd0a) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Cool, '100.0%'. I sense you have a good stamina to attain very strong test coverage.

☝️ strong motivation 😆

Pull request has been modified.

t-nelson

Nits

t-nelson · 2020-11-23T18:49:54Z

-    #[error("stake account with activated stake cannot be merged")]
-    MergeActivatedStake,
+    #[error("stake account with transient stake cannot be merged")]
+    MergeTransientStake,


More accurate error name/message

t-nelson · 2020-11-23T18:50:41Z

            .0
    }

+    // returned tuple is (effective, activating, deactivating) stake


Commented stake_activating_and_deactivating() return tuple comment

t-nelson · 2020-11-23T18:52:36Z

+                    true,
+                ) {
+                    /*
+                    (e, a, d): e - effective, a - activating, d - deactivating */


Heading comment for tuple patterns in get_if_mergeable() match.

Wonky use of multi-line comment to trick cargo fmt

t-nelson · 2020-11-23T18:53:08Z

+        }
+    }
+
+    fn active_stake(&self) -> Option<&Stake> {


More descriptive name for active stake accessor

…3770) * stake: De-replicode mergable info extraction (cherry picked from commit dc7f897) * stake: Cosmetic - rename variable (cherry picked from commit bb2772d) * stake: Allow compatible, fully-active stake accounts to be merged (cherry picked from commit 8e73187) * stake: Remove disused test helper function (cherry picked from commit 6b9a019) * stake: Disallow stakes merging with themselves (cherry picked from commit 488ce98) Co-authored-by: Trent Nelson <trent@solana.com>

t-nelson added the noCI Suppress CI on this Pull Request label Nov 19, 2020

t-nelson commented Nov 19, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

rwalker-apple reviewed Nov 19, 2020

View reviewed changes

CriesofCarrots reviewed Nov 20, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

t-nelson commented Nov 20, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 20, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

t-nelson force-pushed the stake-merge-active branch from 1cf8fa7 to 2763b3c Compare November 21, 2020 05:34

t-nelson marked this pull request as ready for review November 21, 2020 05:37

t-nelson added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels Nov 21, 2020

solana-grimes removed the CI Pull Request is ready to enter CI label Nov 21, 2020

t-nelson requested review from CriesofCarrots and ryoqun November 21, 2020 05:37

t-nelson force-pushed the stake-merge-active branch from 2763b3c to 58862d3 Compare November 21, 2020 06:32

ryoqun reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

CriesofCarrots reviewed Nov 21, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs Outdated

t-nelson force-pushed the stake-merge-active branch 3 times, most recently from 078513a to bdabd0a Compare November 22, 2020 08:11

t-nelson commented Nov 22, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 22, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 22, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

ryoqun reviewed Nov 22, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

ryoqun reviewed Nov 22, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

t-nelson force-pushed the stake-merge-active branch 2 times, most recently from 5754317 to 9c9f2b5 Compare November 23, 2020 04:58

t-nelson commented Nov 23, 2020

View reviewed changes

ryoqun reviewed Nov 23, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun previously approved these changes Nov 23, 2020

View reviewed changes

CriesofCarrots previously approved these changes Nov 23, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs

t-nelson added 5 commits November 23, 2020 11:46

stake: De-replicode mergable info extraction

1651b9c

stake: Cosmetic - rename variable

29375a8

stake: Allow compatible, fully-active stake accounts to be merged

81748d0

stake: Remove disused test helper function

cbab47d

stake: Disallow stakes merging with themselves

4b9413d

t-nelson force-pushed the stake-merge-active branch from 9c9f2b5 to 4b9413d Compare November 23, 2020 18:49

t-nelson commented Nov 23, 2020

View reviewed changes

t-nelson added the v1.4 label Nov 23, 2020

t-nelson merged commit 488ce98 into solana-labs:master Nov 23, 2020

t-nelson deleted the stake-merge-active branch November 23, 2020 21:32

mergify Bot mentioned this pull request Nov 23, 2020

Stake: Support merging fully-activated stake accounts (bp #13712) #13770

Merged

t-nelson mentioned this pull request Nov 28, 2020

Don't reset credits_observed due to stale voters #13836

Merged

2 tasks

Conversation

t-nelson commented Nov 19, 2020

Problem

Summary of Changes

Uh oh!

t-nelson commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rwalker-apple Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

t-nelson Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Nov 21, 2020

Uh oh!

codecov Bot commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Nov 21, 2020

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Nov 23, 2020

Codecov Report

Uh oh!

ryoqun left a comment

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Nov 23, 2020

Codecov Report

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

t-nelson Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

t-nelson Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

t-nelson Nov 23, 2020

Choose a reason for hiding this comment

t-nelson commented Nov 19, 2020 •

edited

Loading

codecov Bot commented Nov 21, 2020 •

edited

Loading