diff --git a/Cargo.lock b/Cargo.lock index 670350e3b1..b0f4dc32a9 100644 --- a/Cargo.lock +++ b/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", diff --git a/programs/stake/Cargo.toml b/programs/stake/Cargo.toml index 2a79be2eb9..54cf1c658f 100644 --- a/programs/stake/Cargo.toml +++ b/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] diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 81c8aac52a..c231c8858d 100644 --- a/programs/stake/src/stake_state.rs +++ b/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 = - checked_add(stake.delegation.stake, source_stake.delegation.stake)?; + merge_delegation_stake_and_credits_observed( + 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 { + 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); + } + } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index db68435113..b982b1f8af 100644 --- a/sdk/src/feature_set.rs +++ b/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 = [ @@ -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()