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

Cargo.lock

@@ -289,6 +289,21 @@ dependencies = [
  "shlex",
 ]
 
+[[package]]
+name = "bit-set"
+version = "0.5.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6e11e16035ea35e4e5997b393eacbf6f63983188f7a2ad25bfb13465f5ad59de"
+dependencies = [
+ "bit-vec",
+]
+
+[[package]]
+name = "bit-vec"
+version = "0.6.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb"
+
 [[package]]
 name = "bitflags"
 version = "1.2.1"
@@ -3265,6 +3280,26 @@ dependencies = [
  "unicode-xid 0.2.0",
 ]
 
+[[package]]
+name = "proptest"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1e0d9cc07f18492d879586c92b485def06bc850da3118075cd45d50e9c95b0e5"
+dependencies = [
+ "bit-set",
+ "bitflags",
+ "byteorder",
+ "lazy_static",
+ "num-traits",
+ "quick-error 2.0.1",
+ "rand 0.8.3",
+ "rand_chacha 0.3.0",
+ "rand_xorshift 0.3.0",
+ "regex-syntax",
+ "rusty-fork",
+ "tempfile",
+]
+
 [[package]]
 name = "prost"
 version = "0.8.0"
@@ -3325,6 +3360,18 @@ dependencies = [
  "percent-encoding 2.1.0",
 ]
 
+[[package]]
+name = "quick-error"
+version = "1.2.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0"
+
+[[package]]
+name = "quick-error"
+version = "2.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3"
+
 [[package]]
 name = "quote"
 version = "0.6.13"
@@ -3377,7 +3424,7 @@ dependencies = [
  "rand_jitter",
  "rand_os",
  "rand_pcg 0.1.2",
- "rand_xorshift",
+ "rand_xorshift 0.1.1",
  "winapi 0.3.8",
 ]
 
@@ -3559,6 +3606,15 @@ dependencies = [
  "rand_core 0.3.1",
 ]
 
+[[package]]
+name = "rand_xorshift"
+version = "0.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d25bf25ec5ae4a3f1b92f929810509a2f53d7dca2f50b794ff57e3face536c8f"
+dependencies = [
+ "rand_core 0.6.3",
+]
+
 [[package]]
 name = "raptorq"
 version = "1.6.4"
@@ -3826,6 +3882,18 @@ version = "1.0.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "61b3909d758bb75c79f23d4736fac9433868679d3ad2ea7a61e3c25cfda9a088"
 
+[[package]]
+name = "rusty-fork"
+version = "0.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "cb3dcc6e454c328bb824492db107ab7c0ae8fcffe4ad210136ef014458c1bc4f"
+dependencies = [
+ "fnv",
+ "quick-error 1.2.3",
+ "tempfile",
+ "wait-timeout",
+]
+
 [[package]]
 name = "ryu"
 version = "1.0.4"
@@ -5668,6 +5736,7 @@ dependencies = [
  "log 0.4.14",
  "num-derive",
  "num-traits",
+ "proptest",
  "rustc_version 0.4.0",
  "serde",
  "serde_derive",

programs/stake/Cargo.toml

@@ -25,6 +25,7 @@ solana-config-program = { path = "../config", version = "=1.8.0" }
 thiserror = "1.0"
 
 [dev-dependencies]
+proptest = "1.0"
 solana-logger = { path = "../../logger", version = "=1.8.0" }
 
 [build-dependencies]

programs/stake/src/stake_state.rs

@@ -8,6 +8,7 @@ use {
         account::{AccountSharedData, ReadableAccount, WritableAccount},
         account_utils::{State, StateMut},
         clock::{Clock, Epoch},
+        feature_set::stake_merge_with_unmatched_credits_observed,
         ic_msg,
         instruction::{checked_add, InstructionError},
         keyed_account::KeyedAccount,
@@ -957,6 +958,7 @@ impl MergeKind {
         }
     }
 
+    // Remove this when the `stake_merge_with_unmatched_credits_observed` feature is removed
     fn active_stakes_can_merge(
         invoke_context: &dyn InvokeContext,
         stake: &Stake,
@@ -988,7 +990,19 @@ impl MergeKind {
         Self::metas_can_merge(invoke_context, self.meta(), source.meta(), clock)?;
         self.active_stake()
             .zip(source.active_stake())
-            .map(|(stake, source)| Self::active_stakes_can_merge(invoke_context, stake, source))
+            .map(|(stake, source)| {
+                if invoke_context
+                    .is_feature_active(&stake_merge_with_unmatched_credits_observed::id())
+                {
+                    Self::active_delegations_can_merge(
+                        invoke_context,
+                        &stake.delegation,
+                        &source.delegation,
+                    )
+                } else {
+                    Self::active_stakes_can_merge(invoke_context, stake, source)
+                }
+            })
             .unwrap_or(Ok(()))?;
         let merged_state = match (self, source) {
             (Self::Inactive(_, _), Self::Inactive(_, _)) => None,
@@ -1005,7 +1019,12 @@ impl MergeKind {
                     source_meta.rent_exempt_reserve,
                     source_stake.delegation.stake,
                 )?;
-                stake.delegation.stake = checked_add(stake.delegation.stake, source_lamports)?;
+                merge_delegation_stake_and_credits_observed(
+                    invoke_context,
+                    &mut stake,
+                    source_lamports,
+                    source_stake.credits_observed,
+                )?;
                 Some(StakeState::Stake(meta, stake))
             }
             (Self::FullyActive(meta, mut stake), Self::FullyActive(_, source_stake)) => {
@@ -1013,8 +1032,12 @@ impl MergeKind {
                 // protect against the magic activation loophole. It will
                 // instead be moved into the destination account as extra,
                 // withdrawable `lamports`
-                stake.delegation.stake =
+                merge_delegation_stake_and_credits_observed(
-                    checked_add(stake.delegation.stake, source_stake.delegation.stake)?;
+                    invoke_context,
+                    &mut stake,
+                    source_stake.delegation.stake,
+                    source_stake.credits_observed,
+                )?;
                 Some(StakeState::Stake(meta, stake))
             }
             _ => return Err(StakeError::MergeMismatch.into()),
@@ -1023,6 +1046,69 @@ impl MergeKind {
     }
 }
 
+fn merge_delegation_stake_and_credits_observed(
+    invoke_context: &dyn InvokeContext,
+    stake: &mut Stake,
+    absorbed_lamports: u64,
+    absorbed_credits_observed: u64,
+) -> Result<(), InstructionError> {
+    if invoke_context.is_feature_active(&stake_merge_with_unmatched_credits_observed::id()) {
+        stake.credits_observed =
+            stake_weighted_credits_observed(stake, absorbed_lamports, absorbed_credits_observed)
+                .ok_or(InstructionError::ArithmeticOverflow)?;
+    }
+    stake.delegation.stake = checked_add(stake.delegation.stake, absorbed_lamports)?;
+    Ok(())
+}
+
+/// Calculate the effective credits observed for two stakes when merging
+///
+/// When merging two `ActivationEpoch` or `FullyActive` stakes, the credits
+/// observed of the merged stake is the weighted average of the two stakes'
+/// credits observed.
+///
+/// This is because we can derive the effective credits_observed by reversing the staking
+/// rewards equation, _while keeping the rewards unchanged after merge (i.e. strong
+/// requirement)_, like below:
+///
+/// a(N) => account, r => rewards, s => stake, c => credits:
+/// assume:
+///   a3 = merge(a1, a2)
+/// then:
+///   a3.s = a1.s + a2.s
+///
+/// Next, given:
+///   aN.r = aN.c * aN.s (for every N)
+/// finally:
+///        a3.r = a1.r + a2.r
+/// a3.c * a3.s = a1.c * a1.s + a2.c * a2.s
+///        a3.c = (a1.c * a1.s + a2.c * a2.s) / (a1.s + a2.s)     // QED
+///
+/// (For this discussion, we omitted irrelevant variables, including distance
+///  calculation against vote_account and point indirection.)
+fn stake_weighted_credits_observed(
+    stake: &Stake,
+    absorbed_lamports: u64,
+    absorbed_credits_observed: u64,
+) -> Option<u64> {
+    if stake.credits_observed == absorbed_credits_observed {
+        Some(stake.credits_observed)
+    } else {
+        let total_stake = u128::from(stake.delegation.stake.checked_add(absorbed_lamports)?);
+        let stake_weighted_credits =
+            u128::from(stake.credits_observed).checked_mul(u128::from(stake.delegation.stake))?;
+        let absorbed_weighted_credits =
+            u128::from(absorbed_credits_observed).checked_mul(u128::from(absorbed_lamports))?;
+        // Discard fractional credits as a merge side-effect friction by taking
+        // the ceiling, done by adding `denominator - 1` to the numerator.
+        let total_weighted_credits = stake_weighted_credits
+            .checked_add(absorbed_weighted_credits)?
+            .checked_add(total_stake)?
+            .checked_sub(1)?;
+        u64::try_from(total_weighted_credits.checked_div(total_stake)?).ok()
+    }
+}
+
 // utility function, used by runtime
 // returns a tuple of (stakers_reward,voters_reward)
 pub fn redeem_rewards(
@@ -1281,6 +1367,7 @@ fn do_create_account(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use proptest::prelude::*;
     use solana_sdk::{
         account::{AccountSharedData, WritableAccount},
         clock::UnixTimestamp,
@@ -6649,4 +6736,187 @@ mod tests {
         let delegation = new_state.delegation().unwrap();
         assert_eq!(delegation.stake, 2 * stake.delegation.stake);
     }
+
+    #[test]
+    fn test_active_stake_merge() {
+        let delegation_a = 4_242_424_242u64;
+        let delegation_b = 6_200_000_000u64;
+        let credits_a = 124_521_000u64;
+        let rent_exempt_reserve = 227_000_000u64;
+        let meta = Meta {
+            rent_exempt_reserve,
+            ..Meta::default()
+        };
+        let stake_a = Stake {
+            delegation: Delegation {
+                stake: delegation_a,
+                ..Delegation::default()
+            },
+            credits_observed: credits_a,
+        };
+        let stake_b = Stake {
+            delegation: Delegation {
+                stake: delegation_b,
+                ..Delegation::default()
+            },
+            credits_observed: credits_a,
+        };
+
+        let invoke_context = MockInvokeContext::new(vec![]);
+
+        // activating stake merge, match credits observed
+        let activation_epoch_a = MergeKind::ActivationEpoch(meta, stake_a);
+        let activation_epoch_b = MergeKind::ActivationEpoch(meta, stake_b);
+        let new_stake = activation_epoch_a
+            .merge(&invoke_context, activation_epoch_b, None)
+            .unwrap()
+            .unwrap()
+            .stake()
+            .unwrap();
+        assert_eq!(new_stake.credits_observed, credits_a);
+        assert_eq!(
+            new_stake.delegation.stake,
+            delegation_a + delegation_b + rent_exempt_reserve
+        );
+
+        // active stake merge, match credits observed
+        let fully_active_a = MergeKind::FullyActive(meta, stake_a);
+        let fully_active_b = MergeKind::FullyActive(meta, stake_b);
+        let new_stake = fully_active_a
+            .merge(&invoke_context, fully_active_b, None)
+            .unwrap()
+            .unwrap()
+            .stake()
+            .unwrap();
+        assert_eq!(new_stake.credits_observed, credits_a);
+        assert_eq!(new_stake.delegation.stake, delegation_a + delegation_b);
+
+        // activating stake merge, unmatched credits observed
+        let credits_b = 125_124_521u64;
+        let stake_b = Stake {
+            delegation: Delegation {
+                stake: delegation_b,
+                ..Delegation::default()
+            },
+            credits_observed: credits_b,
+        };
+        let activation_epoch_a = MergeKind::ActivationEpoch(meta, stake_a);
+        let activation_epoch_b = MergeKind::ActivationEpoch(meta, stake_b);
+        let new_stake = activation_epoch_a
+            .merge(&invoke_context, activation_epoch_b, None)
+            .unwrap()
+            .unwrap()
+            .stake()
+            .unwrap();
+        assert_eq!(
+            new_stake.credits_observed,
+            (credits_a * delegation_a + credits_b * (delegation_b + rent_exempt_reserve))
+                / (delegation_a + delegation_b + rent_exempt_reserve)
+                + 1
+        );
+        assert_eq!(
+            new_stake.delegation.stake,
+            delegation_a + delegation_b + rent_exempt_reserve
+        );
+
+        // active stake merge, unmatched credits observed
+        let fully_active_a = MergeKind::FullyActive(meta, stake_a);
+        let fully_active_b = MergeKind::FullyActive(meta, stake_b);
+        let new_stake = fully_active_a
+            .merge(&invoke_context, fully_active_b, None)
+            .unwrap()
+            .unwrap()
+            .stake()
+            .unwrap();
+        assert_eq!(
+            new_stake.credits_observed,
+            (credits_a * delegation_a + credits_b * delegation_b) / (delegation_a + delegation_b)
+                + 1
+        );
+        assert_eq!(new_stake.delegation.stake, delegation_a + delegation_b);
+
+        // active stake merge, unmatched credits observed, no need to ceiling the calculation
+        let delegation = 1_000_000u64;
+        let credits_a = 200_000_000u64;
+        let credits_b = 100_000_000u64;
+        let rent_exempt_reserve = 227_000_000u64;
+        let meta = Meta {
+            rent_exempt_reserve,
+            ..Meta::default()
+        };
+        let stake_a = Stake {
+            delegation: Delegation {
+                stake: delegation,
+                ..Delegation::default()
+            },
+            credits_observed: credits_a,
+        };
+        let stake_b = Stake {
+            delegation: Delegation {
+                stake: delegation,
+                ..Delegation::default()
+            },
+            credits_observed: credits_b,
+        };
+        let fully_active_a = MergeKind::FullyActive(meta, stake_a);
+        let fully_active_b = MergeKind::FullyActive(meta, stake_b);
+        let new_stake = fully_active_a
+            .merge(&invoke_context, fully_active_b, None)
+            .unwrap()
+            .unwrap()
+            .stake()
+            .unwrap();
+        assert_eq!(
+            new_stake.credits_observed,
+            (credits_a * delegation + credits_b * delegation) / (delegation + delegation)
+        );
+        assert_eq!(new_stake.delegation.stake, delegation * 2);
+    }
+
+    prop_compose! {
+        pub fn sum_within(max: u64)(total in 1..max)
+            (intermediate in 1..total, total in Just(total))
+            -> (u64, u64) {
+                (intermediate, total - intermediate)
+        }
+    }
+
+    proptest! {
+        #[test]
+        fn test_stake_weighted_credits_observed(
+            (credits_a, credits_b) in sum_within(u64::MAX),
+            (delegation_a, delegation_b) in sum_within(u64::MAX),
+        ) {
+            let stake = Stake {
+                delegation: Delegation {
+                    stake: delegation_a,
+                    ..Delegation::default()
+                },
+                credits_observed: credits_a
+            };
+            let credits_observed = stake_weighted_credits_observed(
+                &stake,
+                delegation_b,
+                credits_b,
+            ).unwrap();
+
+            // calculated credits observed should always be between the credits of a and b
+            if credits_a < credits_b {
+                assert!(credits_a < credits_observed);
+                assert!(credits_observed <= credits_b);
+            } else {
+                assert!(credits_b <= credits_observed);
+                assert!(credits_observed <= credits_a);
+            }
+
+            // the difference of the combined weighted credits and the separate weighted credits
+            // should be 1 or 0
+            let weighted_credits_total = credits_observed as u128 * (delegation_a + delegation_b) as u128;
+            let weighted_credits_a = credits_a as u128 * delegation_a as u128;
+            let weighted_credits_b = credits_b as u128 * delegation_b as u128;
+            let raw_diff = weighted_credits_total - (weighted_credits_a + weighted_credits_b);
+            let credits_observed_diff = raw_diff / (delegation_a + delegation_b) as u128;
+            assert!(credits_observed_diff <= 1);
+        }
+    }
 }

sdk/src/feature_set.rs

@@ -187,6 +187,10 @@ pub mod disable_fees_sysvar {
     solana_sdk::declare_id!("JAN1trEUEtZjgXYzNBYHU9DYd7GnThhXfFP7SzPXkPsG");
 }
 
+pub mod stake_merge_with_unmatched_credits_observed {
+    solana_sdk::declare_id!("meRgp4ArRPhD3KtCY9c5yAf2med7mBLsjKTPeVUHqBL");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -228,6 +232,7 @@ lazy_static! {
         (stop_verify_mul64_imm_nonzero::id(), "Sets rbpf vm config verify_mul64_imm_nonzero to false"),
         (merge_nonce_error_into_system_error::id(), "merge NonceError into SystemError"),
         (disable_fees_sysvar::id(), "disable fees sysvar"),
+        (stake_merge_with_unmatched_credits_observed::id(), "allow merging active stakes with unmatched credits_observed #18985"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()