stake: Allow stakes with unmatched credits observed to merge #18985

joncinque · 2021-07-30T04:51:41Z

Problem

It isn't possible to merge active / activating stakes with unmatched credits observed, but mathematically speaking, there's no reason to not allow it.

Summary of Changes

Allow it! This is essentially just calculating the stake-weighted average. If there's any remainder while doing the calculation, we take the ceiling, since reward payout takes current_credits_observed - stake.credits_observed. This means you could lose out on some fractional value when doing an active stake merge. The main idea is to avoid the opposite issue, of getting more rewards than you should due to a merge.

Fixes #18411 Fixes #18942

codecov · 2021-07-30T06:47:39Z

Codecov Report

Merging #18985 (df80ea3) into master (03353d5) will decrease coverage by 0.1%.
The diff coverage is 99.0%.

❗ Current head df80ea3 differs from pull request most recent head 3dd56ee. Consider uploading reports for the commit 3dd56ee to get more accurate results

@@            Coverage Diff            @@
##           master   #18985     +/-   ##
=========================================
- Coverage    82.7%    82.6%   -0.2%     
=========================================
  Files         449      449             
  Lines      127997   127274    -723     
=========================================
- Hits       105968   105180    -788     
- Misses      22029    22094     +65

t-nelson

Looking good! I just have a couple tinfoil hat suggestions

programs/stake/src/stake_state.rs

t-nelson · 2021-07-30T05:13:01Z

programs/stake/src/stake_state.rs

+            (merge_credits_observed as u128).checked_mul(merge_lamports as u128)?;
+        let total_weighted_credits = stake_weighted_credits.checked_add(merge_weighted_credits)?;
+        let effective_credits_observed = total_weighted_credits.checked_div(total_stake)?;
+        if total_weighted_credits.checked_rem(total_stake)? > 0 {


We'd get the same effect by adding half of total_stake to total_weighted_credits before dividing, no?

... but still do we have to ceil if total_stake isn't divisible by half? (note: i'm weak at math.. haven't dove any proof thinking)...

this reminds of @behzadnouri 's concise math technique (#16457 (comment)). but we're doing financial math here instead of engineering math.

so better of being verbose and rigorous, instead of succinctness?

In this case, we're not trying to round up or down, we're always trying to ceiling the value if it's at all above. This is all to make sure that the resulting stake does not get too much credit. If we ever round down, then your merged stake gets one more credit than it truly deserves.

Simple example: if I merge a stake of 100 with credits_observed = 1001 into a stake of 200 with credits_observed = 1000, with rounding that would produce a stake of 300 with credits_observed = 1000, which would get you more rewards than if the stakes had stayed separated. Does that make sense?

this reminds of @behzadnouri 's concise math technique (#16457 (comment)). but we're doing financial math here instead of engineering math.

@ryoqun are you suggesting to add a comment that explains the math with concise variable names?

@joncinque I think @t-nelson 's suggestion always does ceiling. so i think we're at the same page of ceiling.

this reminds of @behzadnouri 's concise math technique (#16457 (comment)). but we're doing financial math here instead of engineering math.

@ryoqun are you suggesting to add a comment that explains the math with concise variable names?

nope. it's just my random thought... xD no need to explain this ceiling part as a comment

Ah I see, yes, to do the ceiling in one go, we'd have to add total_stake - 1 before dividing by total_stake. I'm fine with either

Right, my main goal was to eliminate the second division and branching logic (Lazy Trent sees a branch and would rather eliminate it than write a test)

good point, branches suck, will remove 😄

programs/stake/src/stake_state.rs

CriesofCarrots

Not much to add to Trent's comments; indeed looking good!

sdk/src/feature_set.rs

programs/stake/src/stake_state.rs

ryoqun · 2021-08-02T15:46:45Z

programs/stake/src/stake_state.rs

@@ -1023,6 +1057,34 @@ impl MergeKind {
    }
 }

+/// Calculate the effective credits observed for two stakes whenn merging
+///
+/// When merging two activating or active stakes, the credits observed of the


According to this definition,

solana/programs/stake/src/stake_state.rs

Lines 888 to 899 in 5e27789

match stake.delegation.stake_activating_and_deactivating(

clock.epoch,

Some(stake_history),

true,

) {

/*

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

(0, 0, 0) => Ok(Self::Inactive(meta, stake_keyed_account.lamports()?)),

(0, _, _) => Ok(Self::ActivationEpoch(meta, stake)),

(_, 0, 0) => Ok(Self::FullyActive(meta, stake)),

_ => {

let err = StakeError::MergeTransientStake;

we shouldn't be merging activating stakes.

so, When merging two just-delegated or fully-active stakes, ... would be more accurate.

or When merging two ActivationEpoch or FullyActive stakes, ... (yeah, we lack good terminology here)

Ah I see, I always thought ActivationEpoch = activating, but as you point out, a stake can be activating and also be in a transient state with some active and some activating. I worry that just-delegated doesn't make it clear that the stake is only activating. What about purely-activating?

hmm, purely-activating sounds to me to indicate the transient activating stake account.

maybe, just using enum variant should be ok. it's just a source code comment after all?

@t-nelson do you have some word on this? (iirc, ActivationEpoch came from you?).

programs/stake/src/stake_state.rs

ryoqun · 2021-08-02T16:52:01Z

programs/stake/src/stake_state.rs

+        }
+    }
+
+    proptest! {


this proptest! beast is new to me... seems you broght it here from the spl land. xD

Yeah, it's a great way to test maxes and mins on your calculations! https://altsysrq.github.io/proptest-book/intro.html is a good guide 😁

These docs look way nicer than the ones on docs.rs 🎉

programs/stake/src/stake_state.rs

ryoqun

@joncinque well done pr. I left some cosmetic review comments only. thanks for showing your courage to do the surgery of our heart of inflation without hesitation. xD

ryoqun · 2021-08-02T17:17:18Z

programs/stake/src/stake_state.rs

@@ -957,6 +958,7 @@ impl MergeKind {
        }
    }

+    // Remove this when the `stake_merge_with_unmatched_credits_observed` feature is removed


Pull request has been modified.

t-nelson

lgtm! I'll demote the ceil simplification to a nit. I'd like to see it, but won't block

t-nelson · 2021-08-03T20:17:30Z

programs/stake/src/stake_state.rs

+            (merge_credits_observed as u128).checked_mul(merge_lamports as u128)?;
+        let total_weighted_credits = stake_weighted_credits.checked_add(merge_weighted_credits)?;
+        let effective_credits_observed = total_weighted_credits.checked_div(total_stake)?;
+        if total_weighted_credits.checked_rem(total_stake)? > 0 {


Right, my main goal was to eliminate the second division and branching logic (Lazy Trent sees a branch and would rather eliminate it than write a test)

t-nelson · 2021-08-03T20:18:29Z

programs/stake/src/stake_state.rs

+        }
+    }
+
+    proptest! {


These docs look way nicer than the ones on docs.rs 🎉

Pull request has been modified.

* stake: Allow stakes with unmatched credits observed to merge * Address feedback * Remove branch by doing a ceiling in one calc (cherry picked from commit 2b33c0c) # Conflicts: # Cargo.lock # programs/stake/Cargo.toml # sdk/src/feature_set.rs

…#18985) (#19055) * stake: Allow stakes with unmatched credits observed to merge (#18985) * stake: Allow stakes with unmatched credits observed to merge * Address feedback * Remove branch by doing a ceiling in one calc (cherry picked from commit 2b33c0c) # Conflicts: # Cargo.lock # programs/stake/Cargo.toml # sdk/src/feature_set.rs * Fix merge conflicts * Fix clippy lint * Add back whitespace Co-authored-by: Jon Cinque <jon.cinque@gmail.com>

joncinque requested review from ryoqun, t-nelson and CriesofCarrots July 30, 2021 04:51

t-nelson reviewed Jul 30, 2021

View reviewed changes

CriesofCarrots reviewed Jul 30, 2021

View reviewed changes

sdk/src/feature_set.rs Outdated Show resolved Hide resolved

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

ryoqun reviewed Aug 2, 2021

View reviewed changes

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

ryoqun reviewed Aug 2, 2021

View reviewed changes

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

ryoqun reviewed Aug 2, 2021

View reviewed changes

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

ryoqun previously approved these changes Aug 2, 2021

View reviewed changes

ryoqun reviewed Aug 2, 2021

View reviewed changes

t-nelson previously approved these changes Aug 3, 2021

View reviewed changes

joncinque force-pushed the st-act-merg branch from 49773e9 to 3dd56ee Compare August 3, 2021 20:43

joncinque added the v1.7 label Aug 3, 2021

joncinque added 3 commits August 3, 2021 23:00

stake: Allow stakes with unmatched credits observed to merge

f0527c1

Address feedback

ff2ae59

Remove branch by doing a ceiling in one calc

7e44feb

joncinque force-pushed the st-act-merg branch from 3dd56ee to 7e44feb Compare August 4, 2021 03:00

joncinque merged commit 2b33c0c into solana-labs:master Aug 4, 2021

joncinque deleted the st-act-merg branch August 4, 2021 14:43

mergify bot mentioned this pull request Aug 4, 2021

stake: Allow stakes with unmatched credits observed to merge (backport #18985) #19055

Merged

joncinque mentioned this pull request Aug 18, 2021

stake-pool: UpdateValidatorListBalance must be the sole instruction in the transaction solana-labs/solana-program-library#2288

Closed

brooksprumo mentioned this pull request Aug 23, 2021

backport 19361 v17 #19380

Closed

joncinque mentioned this pull request Nov 16, 2023

stake: Remove feature stake_merge_with_unmatched_credits_observed #34114

Merged

stake: Allow stakes with unmatched credits observed to merge #18985

stake: Allow stakes with unmatched credits observed to merge #18985

joncinque commented Jul 30, 2021

codecov bot commented Jul 30, 2021 •

edited

t-nelson left a comment

t-nelson Jul 30, 2021

ryoqun Aug 2, 2021 •

edited

joncinque Aug 3, 2021

ryoqun Aug 3, 2021

joncinque Aug 3, 2021

t-nelson Aug 3, 2021

joncinque Aug 3, 2021

CriesofCarrots left a comment

ryoqun Aug 2, 2021

joncinque Aug 3, 2021

ryoqun Aug 3, 2021

ryoqun Aug 2, 2021

joncinque Aug 3, 2021

t-nelson Aug 3, 2021

ryoqun left a comment

ryoqun Aug 2, 2021

t-nelson left a comment

t-nelson Aug 3, 2021

t-nelson Aug 3, 2021

	match stake.delegation.stake_activating_and_deactivating(
	clock.epoch,
	Some(stake_history),
	true,
	) {
	/*
	(e, a, d): e - effective, a - activating, d - deactivating */
	(0, 0, 0) => Ok(Self::Inactive(meta, stake_keyed_account.lamports()?)),
	(0, _, _) => Ok(Self::ActivationEpoch(meta, stake)),
	(_, 0, 0) => Ok(Self::FullyActive(meta, stake)),
	_ => {
	let err = StakeError::MergeTransientStake;

stake: Allow stakes with unmatched credits observed to merge #18985

stake: Allow stakes with unmatched credits observed to merge #18985

Conversation

joncinque commented Jul 30, 2021

Problem

Summary of Changes

codecov bot commented Jul 30, 2021 • edited

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Aug 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2021 •

edited

ryoqun Aug 2, 2021 •

edited