diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index fb0b60e49c9..484fba4ec2f 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -13,7 +13,7 @@ use crate::{ reassemble_governance_proto, split_governance_proto, HeapGovernanceData, XdrConversionRate, }, migrations::maybe_run_migrations, - neuron::types::{DissolveStateAndAge, Neuron, NeuronBuilder}, + neuron::{DissolveStateAndAge, Neuron, NeuronBuilder}, neuron_data_validation::{NeuronDataValidationSummary, NeuronDataValidator}, neuron_store::{NeuronMetrics, NeuronStore}, neurons_fund::{ diff --git a/rs/nns/governance/src/governance/ledger_helper.rs b/rs/nns/governance/src/governance/ledger_helper.rs index ba89708b9b9..80cf32b2994 100644 --- a/rs/nns/governance/src/governance/ledger_helper.rs +++ b/rs/nns/governance/src/governance/ledger_helper.rs @@ -1,6 +1,6 @@ use crate::{ governance::{governance_minting_account, neuron_subaccount}, - neuron::types::Neuron, + neuron::Neuron, neuron_store::NeuronStore, pb::v1::{governance_error::ErrorType, GovernanceError}, }; diff --git a/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/merge_neuron_mutation.rs b/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/merge_neuron_mutation.rs index 7b446ff96ef..3a7824a5392 100644 --- a/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/merge_neuron_mutation.rs +++ b/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/merge_neuron_mutation.rs @@ -93,7 +93,7 @@ impl GovernanceNeuronMutation for MergeNeuronMutation { let new_aging_since_timestamp_seconds = now.saturating_sub(new_age_seconds); let aging_timestamp_seconds_delta = (new_aging_since_timestamp_seconds as i128) - .saturating_sub(target_neuron.aging_since_timestamp_seconds as i128); + .saturating_sub(target_neuron.deprecated_aging_since_timestamp_seconds() as i128); Ok(btreemap! { source_neuron.id() => NeuronDeltas { @@ -106,7 +106,7 @@ impl GovernanceNeuronMutation for MergeNeuronMutation { .neg(), // Reset aging aging_since_timestamp_seconds: if source_stake_less_transaction_fee_e8s > 0 { - now.saturating_sub(source_neuron.aging_since_timestamp_seconds) as i128 + now.saturating_sub(source_neuron.deprecated_aging_since_timestamp_seconds()) as i128 } else { 0 }, @@ -212,12 +212,13 @@ impl GovernanceNeuronMutation for MergeNeuronMutation { // here, since we do not change the dissolve state in any other // way. // let source_age_timestamp_seconds = source_neuron_mut.aging_since_timestamp_seconds; - if source_neuron_mut.aging_since_timestamp_seconds != u64::MAX { - source_neuron_mut.aging_since_timestamp_seconds = + if source_neuron_mut.deprecated_aging_since_timestamp_seconds() != u64::MAX { + source_neuron_mut.deprecated_set_aging_since_timestamp_seconds( saturating_add_or_subtract_u64_i128( - source_neuron_mut.aging_since_timestamp_seconds, + source_neuron_mut.deprecated_aging_since_timestamp_seconds(), source_delta_mut.aging_since_timestamp_seconds, - ); + ), + ); // Record that the delta was partially applied source_delta_mut.aging_since_timestamp_seconds = 0; } @@ -241,11 +242,12 @@ impl GovernanceNeuronMutation for MergeNeuronMutation { source_neuron_mut.cached_neuron_stake_e8s, original_delta_cached_neuron_stake_e8s.saturating_neg(), ); - source_neuron_mut.aging_since_timestamp_seconds = + source_neuron_mut.deprecated_set_aging_since_timestamp_seconds( saturating_add_or_subtract_u64_i128( - source_neuron_mut.aging_since_timestamp_seconds, + source_neuron_mut.deprecated_aging_since_timestamp_seconds(), original_delta_aging_since_timestamp_seconds.saturating_neg(), - ); + ), + ); }) .expect("Expected the source neuron to exist"); diff --git a/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/mod.rs b/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/mod.rs index f6bc36f377c..476cc197e03 100644 --- a/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/mod.rs +++ b/rs/nns/governance/src/governance/manage_neuron_request/neuron_mutation/mod.rs @@ -1,4 +1,4 @@ -use crate::{governance::Governance, neuron::types::Neuron, pb::v1::GovernanceError}; +use crate::{governance::Governance, neuron::Neuron, pb::v1::GovernanceError}; use async_trait::async_trait; use ic_nns_common::pb::v1::NeuronId; use std::{ @@ -236,10 +236,10 @@ impl NeuronDeltas { neuron.staked_maturity_e8s_equivalent = Some(staked_maturity_e8s_equivalent_new_value); } - neuron.aging_since_timestamp_seconds = saturating_add_or_subtract_u64_i128( - neuron.aging_since_timestamp_seconds, + neuron.deprecated_set_aging_since_timestamp_seconds(saturating_add_or_subtract_u64_i128( + neuron.deprecated_aging_since_timestamp_seconds(), self.aging_since_timestamp_seconds, - ); + )); } } diff --git a/rs/nns/governance/src/governance/merge_neurons.rs b/rs/nns/governance/src/governance/merge_neurons.rs index 8abf006ae29..c1139c85a3a 100644 --- a/rs/nns/governance/src/governance/merge_neurons.rs +++ b/rs/nns/governance/src/governance/merge_neurons.rs @@ -3,7 +3,7 @@ use crate::{ combine_aged_stakes, ledger_helper::{BurnNeuronFeesOperation, NeuronStakeTransferOperation}, }, - neuron::types::{DissolveStateAndAge, Neuron}, + neuron::{DissolveStateAndAge, Neuron}, neuron_store::NeuronStore, pb::v1::{ governance_error::ErrorType, manage_neuron::Merge, manage_neuron::NeuronIdOrSubaccount, @@ -68,7 +68,7 @@ impl MergeNeuronsEffect { pub fn source_effect(&self) -> MergeNeuronsSourceEffect { MergeNeuronsSourceEffect { - dissolve_state_and_age: self.source_neuron_dissolve_state_and_age.clone(), + dissolve_state_and_age: self.source_neuron_dissolve_state_and_age, subtract_maturity: self.transfer_maturity_e8s, subtract_staked_maturity: self.transfer_staked_maturity_e8s, } @@ -76,7 +76,7 @@ impl MergeNeuronsEffect { pub fn target_effect(&self) -> MergeNeuronsTargetEffect { MergeNeuronsTargetEffect { - dissolve_state_and_age: self.target_neuron_dissolve_state_and_age.clone(), + dissolve_state_and_age: self.target_neuron_dissolve_state_and_age, add_maturity: self.transfer_maturity_e8s, add_staked_maturity: self.transfer_staked_maturity_e8s, } @@ -620,7 +620,7 @@ fn is_neuron_involved_with_open_proposals( mod tests { use super::*; use crate::{ - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::{neuron::Followees, proposal::Action, ManageNeuron, NeuronType, Proposal, Topic}, }; use assert_matches::assert_matches; diff --git a/rs/nns/governance/src/governance/tests/mod.rs b/rs/nns/governance/src/governance/tests/mod.rs index ef8b8af4ca0..3eda45374c6 100644 --- a/rs/nns/governance/src/governance/tests/mod.rs +++ b/rs/nns/governance/src/governance/tests/mod.rs @@ -1,6 +1,6 @@ use super::*; use crate::{ - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::{ governance::{followers_map::Followers, FollowersMap}, Neuron as NeuronProto, @@ -887,7 +887,7 @@ mod metrics_tests { } mod neuron_archiving_tests { - use crate::neuron::types::{DissolveStateAndAge, NeuronBuilder}; + use crate::neuron::{DissolveStateAndAge, NeuronBuilder}; use ic_base_types::PrincipalId; use ic_nns_common::pb::v1::NeuronId; use icp_ledger::Subaccount; @@ -1040,7 +1040,7 @@ mod neuron_archiving_tests { mod cast_vote_and_cascade_follow { use crate::{ governance::{Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS}, - neuron::types::{DissolveStateAndAge, Neuron, NeuronBuilder}, + neuron::{DissolveStateAndAge, Neuron, NeuronBuilder}, neuron_store::NeuronStore, pb::v1::{neuron::Followees, Ballot, Topic, Vote}, }; diff --git a/rs/nns/governance/src/neuron/dissolve_state_and_age.rs b/rs/nns/governance/src/neuron/dissolve_state_and_age.rs new file mode 100644 index 00000000000..f610c0f7143 --- /dev/null +++ b/rs/nns/governance/src/neuron/dissolve_state_and_age.rs @@ -0,0 +1,636 @@ +use crate::{governance::MAX_DISSOLVE_DELAY_SECONDS, pb::v1::NeuronState}; + +/// An enum to represent different combinations of a neurons dissolve_state and +/// aging_since_timestamp_seconds. Currently, the back-and-forth conversions should make sure the +/// legacy states remain the same unless some operations performed on the neuron makes the state/age +/// changes. After we make sure all neuron mutations or creations must mutate states to valid ones +/// and the invalid states have been migrated to valid ones on the mainnet, we can panic in +/// conversion when invalid states are encountered. 2 of the legacy states +/// (LegacyDissolvingOrDissolved and LegacyDissolved) are the cases we already know to be existing +/// on the mainnet. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum DissolveStateAndAge { + /// A non-dissolving neuron has a dissolve delay and an aging since timestamp. + NotDissolving { + dissolve_delay_seconds: u64, + aging_since_timestamp_seconds: u64, + }, + /// A dissolving or dissolved neuron has a dissolved timestamp and no aging since timestamp. + DissolvingOrDissolved { + when_dissolved_timestamp_seconds: u64, + }, + /// We used to allow neurons to have age when they were dissolving or dissolved. This should be + /// mapped to DissolvingOrDissolved { when_dissolved_timestamp_seconds } and its aging singe + /// timestamp removed. + LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: u64, + aging_since_timestamp_seconds: u64, + }, + /// When claiming a neuron, the dissolve delay is set to 0 while the neuron is considered + /// dissolved. Its aging_since_timestamp_seconds is set to the neuron was claimed. This state + /// should be mapped to DissolvingOrDissolved { when_dissolved_timestamp_seconds: + /// aging_since_timestamp_seconds }. + LegacyDissolved { aging_since_timestamp_seconds: u64 }, + + /// The dissolve state is None, which should have never existed, but we keep the current + /// behavior of considering it as a dissolved neuron. + LegacyNoneDissolveState { aging_since_timestamp_seconds: u64 }, +} + +impl DissolveStateAndAge { + /// Returns the current state given the current time. Mainly for differentiating between + /// dissolving and dissolved neurons. + pub fn current_state(self, now_seconds: u64) -> NeuronState { + match self { + DissolveStateAndAge::NotDissolving { .. } => NeuronState::NotDissolving, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + } => { + if now_seconds >= when_dissolved_timestamp_seconds { + NeuronState::Dissolved + } else { + NeuronState::Dissolving + } + } + DissolveStateAndAge::LegacyDissolved { .. } => NeuronState::Dissolved, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds: _, + } => { + if now_seconds >= when_dissolved_timestamp_seconds { + NeuronState::Dissolved + } else { + NeuronState::Dissolving + } + } + DissolveStateAndAge::LegacyNoneDissolveState { .. } => NeuronState::Dissolved, + } + } + + /// Returns the age of the neuron in seconds. When its age isn't well-defined (e.g. dissolving + /// or dissolved), the age is 0. However, in the legacy cases where we still have + /// aging_since_timestamp_seconds, the neuron still has an age, and the legacy cases will be + /// cleaned up soon. + pub fn age_seconds(self, now_seconds: u64) -> u64 { + match self { + Self::NotDissolving { + aging_since_timestamp_seconds, + .. + } => now_seconds.saturating_sub(aging_since_timestamp_seconds), + Self::DissolvingOrDissolved { .. } => 0, + Self::LegacyDissolvingOrDissolved { + aging_since_timestamp_seconds, + .. + } => now_seconds.saturating_sub(aging_since_timestamp_seconds), + Self::LegacyDissolved { + aging_since_timestamp_seconds, + .. + } => now_seconds.saturating_sub(aging_since_timestamp_seconds), + Self::LegacyNoneDissolveState { + aging_since_timestamp_seconds, + } => now_seconds.saturating_sub(aging_since_timestamp_seconds), + } + } + + pub fn dissolve_delay_seconds(self, now_seconds: u64) -> u64 { + match self { + Self::NotDissolving { + dissolve_delay_seconds, + .. + } => dissolve_delay_seconds, + // For the dissolving/dissolved case (legacy or not), the dissolve delay is the time remaining until the + // dissolve timestamp. + Self::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + } => when_dissolved_timestamp_seconds.saturating_sub(now_seconds), + Self::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + .. + } => when_dissolved_timestamp_seconds.saturating_sub(now_seconds), + // The below cases are considered dissolved, so the dissolve delay is 0. + Self::LegacyDissolved { .. } => 0, + Self::LegacyNoneDissolveState { .. } => 0, + } + } + + /// Increases the dissolve delay of the neuron by the given number of seconds. If the neuron is + /// already dissolved, it transitions to a non-dissolving state with the new dissolve delay. If + /// the neuron is dissolving, the dissolve timestamp is increased by the given number of + /// seconds. If the neuron is not dissolving, the dissolve delay is increased by the given + /// number of seconds. The new dissolve delay is capped at MAX_DISSOLVE_DELAY_SECONDS. + pub fn increase_dissolve_delay( + self, + now_seconds: u64, + additional_dissolve_delay_seconds: u32, + ) -> Self { + // If there is no dissolve delay, this is a no-op. Upstream validation can decide if + // an error should be returned to the user. + if additional_dissolve_delay_seconds == 0 { + return self; + } + let additional_dissolve_delay_seconds = additional_dissolve_delay_seconds as u64; + + match self { + Self::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds, + } => { + let new_delay_dissolve_delay_seconds = std::cmp::min( + dissolve_delay_seconds.saturating_add(additional_dissolve_delay_seconds), + MAX_DISSOLVE_DELAY_SECONDS, + ); + Self::NotDissolving { + dissolve_delay_seconds: new_delay_dissolve_delay_seconds, + aging_since_timestamp_seconds, + } + } + Self::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + } => Self::increase_dissolve_delay_for_dissolving_or_dissolved( + when_dissolved_timestamp_seconds, + now_seconds, + additional_dissolve_delay_seconds, + ), + // In the legacy case where we still have an aging_since_timestamp_seconds for + // dissolving neurons, we transition them into a valid case, and its original + // aging_since_timestamp_seconds is ignored. + Self::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds: _, + } => Self::increase_dissolve_delay_for_dissolving_or_dissolved( + when_dissolved_timestamp_seconds, + now_seconds, + additional_dissolve_delay_seconds, + ), + Self::LegacyDissolved { + aging_since_timestamp_seconds: _, + } => { + let dissolve_delay_seconds = std::cmp::min( + additional_dissolve_delay_seconds, + MAX_DISSOLVE_DELAY_SECONDS, + ); + // We transition from `Dissolved` to `NotDissolving`: reset age. + Self::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds: now_seconds, + } + } + Self::LegacyNoneDissolveState { + aging_since_timestamp_seconds: _, + } => { + let dissolve_delay_seconds = std::cmp::min( + additional_dissolve_delay_seconds, + MAX_DISSOLVE_DELAY_SECONDS, + ); + // We transition from `Dissolved` to `NotDissolving`: reset age. + Self::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds: now_seconds, + } + } + } + } + + /// Helper function to make sure legacy dissolving/dissolved case is handled exactly the same as + /// the non-legacy case. + fn increase_dissolve_delay_for_dissolving_or_dissolved( + when_dissolved_timestamp_seconds: u64, + now_seconds: u64, + additional_dissolve_delay_seconds: u64, + ) -> Self { + if now_seconds < when_dissolved_timestamp_seconds { + // Not dissolved yet. Increase the dissolve delay by increasing the dissolve timestamp. + let dissolve_delay_seconds = + when_dissolved_timestamp_seconds.saturating_sub(now_seconds); + let new_delay_seconds = std::cmp::min( + dissolve_delay_seconds.saturating_add(additional_dissolve_delay_seconds), + MAX_DISSOLVE_DELAY_SECONDS, + ); + let new_when_dissolved_timestamp_seconds = + now_seconds.saturating_add(new_delay_seconds); + Self::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: new_when_dissolved_timestamp_seconds, + } + } else { + // This neuron is dissolved. Set it to non-dissolving. + let new_delay_seconds = std::cmp::min( + additional_dissolve_delay_seconds, + MAX_DISSOLVE_DELAY_SECONDS, + ); + Self::NotDissolving { + dissolve_delay_seconds: new_delay_seconds, + aging_since_timestamp_seconds: now_seconds, + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const NOW: u64 = 123_456_789; + + fn assert_current_state( + dissolve_state_and_age: DissolveStateAndAge, + expected_neuron_state: NeuronState, + ) { + assert_eq!( + dissolve_state_and_age.current_state(NOW), + expected_neuron_state + ); + } + + #[test] + fn test_current_state() { + assert_current_state( + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 1000, + aging_since_timestamp_seconds: NOW, + }, + NeuronState::NotDissolving, + ); + for when_dissolved_timestamp_seconds in [0, NOW - 1, NOW] { + assert_current_state( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }, + NeuronState::Dissolved, + ); + assert_current_state( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds: NOW, + }, + NeuronState::Dissolved, + ); + } + for when_dissolved_timestamp_seconds in [NOW + 1, NOW + 100] { + assert_current_state( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }, + NeuronState::Dissolving, + ); + assert_current_state( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds: NOW, + }, + NeuronState::Dissolving, + ); + } + assert_current_state( + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds: NOW, + }, + NeuronState::Dissolved, + ); + assert_current_state( + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds: NOW, + }, + NeuronState::Dissolved, + ); + } + + fn assert_dissolve_delay_seconds( + dissolve_state_and_age: DissolveStateAndAge, + expected_dissolve_delay_seconds: u64, + ) { + assert_eq!( + dissolve_state_and_age.dissolve_delay_seconds(NOW), + expected_dissolve_delay_seconds + ); + } + + #[test] + fn test_dissolve_delay_seconds_non_dissolving() { + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_dissolve_delay_seconds( + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 100, + aging_since_timestamp_seconds, + }, + 100, + ); + } + } + + #[test] + fn test_dissolve_delay_seconds_dissolving_or_dissolved() { + for (when_dissolved_timestamp_seconds, expected_dissolve_delay_seconds) in [ + (0, 0), + (NOW - 1, 0), + (NOW, 0), + (NOW + 1, 1), + (NOW + 100, 100), + ] { + assert_dissolve_delay_seconds( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }, + expected_dissolve_delay_seconds, + ); + } + } + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + #[test] + fn test_dissolve_delay_seconds_legacy_dissolving_or_dissolved() { + for (when_dissolved_timestamp_seconds, expected_dissolve_delay_seconds) in [ + (0, 0), + (NOW - 1, 0), + (NOW, 0), + (NOW + 1, 1), + (NOW + 100, 100), + ] { + // The aging_since_timestamp_seconds is ignored in the dissolve delay calculation. + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_dissolve_delay_seconds( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds, + }, + expected_dissolve_delay_seconds, + ); + } + } + } + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + #[test] + fn test_dissolve_delay_seconds_legacy_dissolved() { + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_dissolve_delay_seconds( + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds, + }, + 0, + ); + } + } + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + #[test] + fn test_dissolve_delay_seconds_legacy_none_dissolve_state() { + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_dissolve_delay_seconds( + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds, + }, + 0, + ); + } + } + + fn assert_age_seconds(dissolve_state_and_age: DissolveStateAndAge, expected_age_seconds: u64) { + assert_eq!( + dissolve_state_and_age.age_seconds(NOW), + expected_age_seconds + ); + } + + #[test] + fn test_age_seconds_non_dissolving() { + for (aging_since_timestamp_seconds, expected_age_seconds) in + [(0, NOW), (NOW - 1, 1), (NOW, 0), (NOW + 1, 0)] + { + for dissolve_delay_seconds in [0, 1, 100, MAX_DISSOLVE_DELAY_SECONDS] { + assert_age_seconds( + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds, + }, + expected_age_seconds, + ); + } + } + } + + #[test] + fn test_age_seconds_dissolving_or_dissolved() { + // Dissolving or dissolved neurons have an age of 0. + for when_dissolved_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_age_seconds( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }, + 0, + ); + } + } + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + #[test] + fn test_age_seconds_legacy_cases() { + for (aging_since_timestamp_seconds, expected_age_seconds) in + [(0, NOW), (NOW - 1, 1), (NOW, 0), (NOW + 1, 0)] + { + // The dissolve timestamp does not matter for calculating the age. + for when_dissolved_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1] { + assert_age_seconds( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds, + }, + expected_age_seconds, + ); + } + + assert_age_seconds( + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds, + }, + expected_age_seconds, + ); + + assert_age_seconds( + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds, + }, + expected_age_seconds, + ); + } + } + + fn assert_increase_dissolve_delay( + original_dissolve_state_and_age: DissolveStateAndAge, + additional_dissolve_delay_seconds: u32, + expected_dissolve_state_and_age: DissolveStateAndAge, + ) { + assert_eq!( + original_dissolve_state_and_age + .increase_dissolve_delay(NOW, additional_dissolve_delay_seconds), + expected_dissolve_state_and_age + ); + } + + #[test] + fn test_increase_dissolve_delay_for_dissolved_neurons() { + for dissolved_state_and_age in [ + // Valid cases + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW, + }, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW - 1, + }, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: 0, + }, + // TODO(NNS1-2951): clean up when the legacy cases are removed. + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW, + aging_since_timestamp_seconds: 0, + }, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW - 1, + aging_since_timestamp_seconds: 0, + }, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: 0, + aging_since_timestamp_seconds: 0, + }, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW, + aging_since_timestamp_seconds: NOW + 100, + }, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW - 1, + aging_since_timestamp_seconds: NOW + 100, + }, + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: 0, + aging_since_timestamp_seconds: NOW + 100, + }, + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds: 0, + }, + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds: NOW - 100, + }, + DissolveStateAndAge::LegacyDissolved { + aging_since_timestamp_seconds: NOW + 100, + }, + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds: 0, + }, + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds: NOW - 100, + }, + DissolveStateAndAge::LegacyNoneDissolveState { + aging_since_timestamp_seconds: NOW + 100, + }, + ] { + assert_increase_dissolve_delay( + dissolved_state_and_age, + 1, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 1, + aging_since_timestamp_seconds: NOW, + }, + ); + + // Test that the dissolve delay is capped at MAX_DISSOLVE_DELAY_SECONDS. + assert_increase_dissolve_delay( + dissolved_state_and_age, + MAX_DISSOLVE_DELAY_SECONDS as u32 + 1000, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: MAX_DISSOLVE_DELAY_SECONDS, + aging_since_timestamp_seconds: NOW, + }, + ); + } + } + + #[test] + fn test_increase_dissolve_delay_for_not_dissolving_neurons() { + for current_aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1, NOW + 2000] { + for current_dissolve_delay_seconds in + [1, 10, 100, NOW, NOW + 1000, MAX_DISSOLVE_DELAY_SECONDS - 1] + { + assert_increase_dissolve_delay( + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: current_dissolve_delay_seconds, + aging_since_timestamp_seconds: current_aging_since_timestamp_seconds, + }, + 1, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: current_dissolve_delay_seconds + 1, + aging_since_timestamp_seconds: current_aging_since_timestamp_seconds, + }, + ); + } + } + + // Test that the dissolve delay is capped at MAX_DISSOLVE_DELAY_SECONDS. + assert_increase_dissolve_delay( + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 1000, + aging_since_timestamp_seconds: NOW, + }, + MAX_DISSOLVE_DELAY_SECONDS as u32, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: MAX_DISSOLVE_DELAY_SECONDS, + aging_since_timestamp_seconds: NOW, + }, + ); + } + + #[test] + fn test_increase_dissolve_delay_for_dissolving_neurons() { + for when_dissolved_timestamp_seconds in + [NOW + 1, NOW + 1000, NOW + MAX_DISSOLVE_DELAY_SECONDS - 1] + { + assert_increase_dissolve_delay( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }, + 1, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: when_dissolved_timestamp_seconds + 1, + }, + ); + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1, NOW + 2000] { + assert_increase_dissolve_delay( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds, + aging_since_timestamp_seconds, + }, + 1, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: when_dissolved_timestamp_seconds + 1, + }, + ); + } + } + + // Test that the dissolve delay is capped at MAX_DISSOLVE_DELAY_SECONDS. + assert_increase_dissolve_delay( + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW + 1000, + }, + MAX_DISSOLVE_DELAY_SECONDS as u32, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW + MAX_DISSOLVE_DELAY_SECONDS, + }, + ); + + // TODO(NNS1-2951): clean up when the legacy cases are removed. + for aging_since_timestamp_seconds in [0, NOW - 1, NOW, NOW + 1, NOW + 2000] { + assert_increase_dissolve_delay( + DissolveStateAndAge::LegacyDissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW + 1000, + aging_since_timestamp_seconds, + }, + MAX_DISSOLVE_DELAY_SECONDS as u32, + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: NOW + MAX_DISSOLVE_DELAY_SECONDS, + }, + ); + } + } +} diff --git a/rs/nns/governance/src/neuron/mod.rs b/rs/nns/governance/src/neuron/mod.rs index 5b98b6a31ef..cd04cddcf1a 100644 --- a/rs/nns/governance/src/neuron/mod.rs +++ b/rs/nns/governance/src/neuron/mod.rs @@ -4,7 +4,6 @@ use crate::{ LOG_PREFIX, MAX_DISSOLVE_DELAY_SECONDS, MAX_NEURON_AGE_FOR_AGE_BONUS, MAX_NEURON_RECENT_BALLOTS, MAX_NUM_HOT_KEYS_PER_NEURON, }, - neuron::types::{DissolveStateAndAge, Neuron, StoredDissolvedStateAndAge}, pb::v1::{ governance_error::ErrorType, manage_neuron::{configure::Operation, Configure}, @@ -23,7 +22,10 @@ use std::{ ops::RangeBounds, }; +pub mod dissolve_state_and_age; +pub use dissolve_state_and_age::*; pub mod types; +pub use types::*; fn neuron_state( now_seconds: u64, @@ -99,11 +101,10 @@ impl Neuron { /// Returns the state the neuron would be in a time /// `now_seconds`. See [NeuronState] for details. pub fn state(&self, now_seconds: u64) -> NeuronState { - neuron_state( - now_seconds, - &self.spawn_at_timestamp_seconds, - &self.dissolve_state, - ) + if self.spawn_at_timestamp_seconds.is_some() { + return NeuronState::Spawning; + } + self.dissolve_state_and_age().current_state(now_seconds) } /// Returns true if and only if `principal` is equal to the @@ -330,62 +331,10 @@ impl Neuron { now_seconds: u64, additional_dissolve_delay_seconds: u32, ) { - let additional_delay = additional_dissolve_delay_seconds as u64; - // If there is no dissolve delay, this is a no-op. Upstream validation can decide if - // an error should be returned to the user. - if additional_delay == 0 { - return; - } - match self.dissolve_state { - Some(DissolveState::DissolveDelaySeconds(delay)) => { - let new_delay = std::cmp::min( - delay.saturating_add(additional_delay), - MAX_DISSOLVE_DELAY_SECONDS, - ); - // Note that if delay == 0, this neuron was - // dissolved and it now becomes non-dissolving. - self.dissolve_state = Some(DissolveState::DissolveDelaySeconds(new_delay)); - if delay == 0 { - // We transition from `Dissolved` to `NotDissolving`: reset age. - self.aging_since_timestamp_seconds = now_seconds; - } - } - Some(DissolveState::WhenDissolvedTimestampSeconds(ts)) => { - if ts > now_seconds { - let delay = ts - now_seconds; - let new_delay = std::cmp::min( - delay.saturating_add(additional_delay), - MAX_DISSOLVE_DELAY_SECONDS, - ); - let new_ts = now_seconds + new_delay; - // Sanity check: - // if additional_delay == 0, then - // new_delay == delay == ts - now_seconds, whence - // new_ts == now_seconds + ts - now_seconds == ts - self.dissolve_state = - Some(DissolveState::WhenDissolvedTimestampSeconds(new_ts)); - // The neuron was and remains `Dissolving`: - // its effective neuron age should already be - // zero by having an `aging_since` timestamp - // in the far future. Reset it just in case. - self.aging_since_timestamp_seconds = u64::MAX; - } else { - // ts <= now_seconds - // This neuron is dissolved. Set it to non-dissolving. - let new_delay = std::cmp::min(additional_delay, MAX_DISSOLVE_DELAY_SECONDS); - self.dissolve_state = Some(DissolveState::DissolveDelaySeconds(new_delay)); - // We transition from `Dissolved` to `NotDissolving`: reset age. - self.aging_since_timestamp_seconds = now_seconds; - } - } - None => { - // This neuron is dissolved. Set it to non-dissolving. - let new_delay = std::cmp::min(additional_delay, MAX_DISSOLVE_DELAY_SECONDS); - self.dissolve_state = Some(DissolveState::DissolveDelaySeconds(new_delay)); - // We transition from `Dissolved` to `NotDissolving`: reset age. - self.aging_since_timestamp_seconds = now_seconds; - } - } + let new_dissolve_state_and_age = self + .dissolve_state_and_age() + .increase_dissolve_delay(now_seconds, additional_dissolve_delay_seconds); + self.set_dissolve_state_and_age(new_dissolve_state_and_age); } /// Join the Internet Computer's Neurons' Fund. If this neuron is @@ -512,7 +461,7 @@ impl Neuron { /// neuron is dissolving, `aging_since` is a value in the far /// future, effectively making its age zero. pub fn age_seconds(&self, now_seconds: u64) -> u64 { - now_seconds.saturating_sub(self.aging_since_timestamp_seconds) + self.dissolve_state_and_age().age_seconds(now_seconds) } /// Returns the dissolve delay of this neuron. For a non-dissolving @@ -521,7 +470,8 @@ impl Neuron { /// `now_seconds`) until the neuron becomes dissolved; for a /// dissolved neuron, this function returns zero. pub fn dissolve_delay_seconds(&self, now_seconds: u64) -> u64 { - neuron_dissolve_delay_seconds(now_seconds, &self.dissolve_state) + self.dissolve_state_and_age() + .dissolve_delay_seconds(now_seconds) } pub fn is_dissolved(&self, now_seconds: u64) -> bool { @@ -934,7 +884,7 @@ mod tests { use super::*; use crate::{ governance::ONE_YEAR_SECONDS, - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::manage_neuron::{SetDissolveTimestamp, StartDissolving}, }; @@ -1202,6 +1152,7 @@ mod tests { ]; for dissolve_state_and_age in cases { + println!("Testing case {:?}", dissolve_state_and_age); test_increase_dissolve_delay_by_1_on_dissolved_neuron(dissolve_state_and_age); } } diff --git a/rs/nns/governance/src/neuron/types.rs b/rs/nns/governance/src/neuron/types.rs index 82cd14cd4c7..98013088947 100644 --- a/rs/nns/governance/src/neuron/types.rs +++ b/rs/nns/governance/src/neuron/types.rs @@ -1,4 +1,5 @@ use crate::{ + neuron::dissolve_state_and_age::DissolveStateAndAge, neuron_store::NeuronStoreError, pb::v1::{ abridged_neuron::DissolveState as AbridgedNeuronDissolveState, @@ -142,6 +143,33 @@ impl Neuron { pub fn set_controller(&mut self, new_controller: PrincipalId) { self.controller = new_controller; } + + /// Returns the dissolve state. Deprecated and only used by the old merge neurons flow and will + /// be cleaned up. + pub fn depregated_dissolve_state(&self) -> Option { + self.dissolve_state.clone() + } + + /// Sets the dissolve state. Deprecated and only used by the old merge neurons flow and will + /// be cleaned up. + pub fn deprecated_set_dissolve_state(&mut self, dissolve_state: NeuronDissolveState) { + self.dissolve_state = Some(dissolve_state); + } + + /// Returns the aging since timestamp in seconds. Deprecated and only used by the old merge + /// neurons flow and will be cleaned up. + pub fn deprecated_aging_since_timestamp_seconds(&self) -> u64 { + self.aging_since_timestamp_seconds + } + + /// Sets the aging since timestamp in seconds. Deprecated and only used by the old merge neurons + /// flow and will be cleaned up. + pub fn deprecated_set_aging_since_timestamp_seconds( + &mut self, + aging_since_timestamp_seconds: u64, + ) { + self.aging_since_timestamp_seconds = aging_since_timestamp_seconds; + } } impl From for NeuronProto { @@ -649,43 +677,6 @@ impl NeuronBuilder { } } -/// An enum to represent different combinations of a neurons dissolve_state and -/// aging_since_timestamp_seconds. Currently, the back-and-forth conversions should make sure the -/// legacy states remain the same unless some operations performed on the neuron makes the state/age -/// changes. After we make sure all neuron mutations or creations must mutate states to valid ones -/// and the invalid states have been migrated to valid ones on the mainnet, we can panic in -/// conversion when invalid states are encountered. 2 of the legacy states -/// (LegacyDissolvingOrDissolved and LegacyDissolved) are the cases we already know to be existing -/// on the mainnet. -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum DissolveStateAndAge { - /// A non-dissolving neuron has a dissolve delay and an aging since timestamp. - NotDissolving { - dissolve_delay_seconds: u64, - aging_since_timestamp_seconds: u64, - }, - /// A dissolving or dissolved neuron has a dissolved timestamp and no aging since timestamp. - DissolvingOrDissolved { - when_dissolved_timestamp_seconds: u64, - }, - /// We used to allow neurons to have age when they were dissolving or dissolved. This should be - /// mapped to DissolvingOrDissolved { when_dissolved_timestamp_seconds } and its aging singe - /// timestamp removed. - LegacyDissolvingOrDissolved { - when_dissolved_timestamp_seconds: u64, - aging_since_timestamp_seconds: u64, - }, - /// When claiming a neuron, the dissolve delay is set to 0 while the neuron is considered - /// dissolved. Its aging_since_timestamp_seconds is set to the neuron was claimed. This state - /// should be mapped to DissolvingOrDissolved { when_dissolved_timestamp_seconds: - /// aging_since_timestamp_seconds }. - LegacyDissolved { aging_since_timestamp_seconds: u64 }, - - /// The dissolve state is None, which should have never existed, but we keep the current - /// behavior of considering it as a dissolved neuron. - LegacyNoneDissolveState { aging_since_timestamp_seconds: u64 }, -} - /// An intermediate struct to represent a neuron's dissolve state and age on the storage layer. #[derive(Clone, Debug, PartialEq)] pub(super) struct StoredDissolvedStateAndAge { @@ -852,7 +843,7 @@ mod tests { for (dissolve_state_and_age, stored_dissolved_state_and_age) in test_cases { assert_eq!( - StoredDissolvedStateAndAge::from(dissolve_state_and_age.clone()), + StoredDissolvedStateAndAge::from(dissolve_state_and_age), stored_dissolved_state_and_age.clone() ); assert_eq!( diff --git a/rs/nns/governance/src/neuron_data_validation.rs b/rs/nns/governance/src/neuron_data_validation.rs index a5878b24768..58c78b4cccd 100644 --- a/rs/nns/governance/src/neuron_data_validation.rs +++ b/rs/nns/governance/src/neuron_data_validation.rs @@ -1,5 +1,5 @@ use crate::{ - neuron::types::Neuron, + neuron::Neuron, neuron_store::NeuronStore, pb::v1::Topic, storage::{with_stable_neuron_indexes, with_stable_neuron_store}, @@ -683,7 +683,7 @@ mod tests { use maplit::{btreemap, hashmap}; use crate::{ - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::{neuron::Followees, KnownNeuronData}, storage::{with_stable_neuron_indexes_mut, with_stable_neuron_store_mut}, }; diff --git a/rs/nns/governance/src/neuron_store/metrics.rs b/rs/nns/governance/src/neuron_store/metrics.rs index daf54e10517..1543731679a 100644 --- a/rs/nns/governance/src/neuron_store/metrics.rs +++ b/rs/nns/governance/src/neuron_store/metrics.rs @@ -221,7 +221,7 @@ mod tests { use super::*; use crate::{ governance::{ONE_DAY_SECONDS, ONE_YEAR_SECONDS}, - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::NeuronType, }; use ic_base_types::PrincipalId; diff --git a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs index 0dc9b368ddc..a468aef7f79 100644 --- a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs +++ b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs @@ -1,6 +1,6 @@ use super::*; use crate::{ - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::neuron::Followees, storage::with_stable_neuron_indexes, }; diff --git a/rs/nns/governance/src/storage/neuron_indexes.rs b/rs/nns/governance/src/storage/neuron_indexes.rs index ac4752d54d2..21f098d3206 100644 --- a/rs/nns/governance/src/storage/neuron_indexes.rs +++ b/rs/nns/governance/src/storage/neuron_indexes.rs @@ -3,7 +3,7 @@ use crate::{ account_id_index::NeuronAccountIdIndex, known_neuron_index::{AddKnownNeuronError, KnownNeuronIndex, RemoveKnownNeuronError}, - neuron::types::Neuron, + neuron::Neuron, neuron_store::NeuronStoreError, pb::v1::Topic, storage::validate_stable_btree_map, diff --git a/rs/nns/governance/src/storage/neuron_indexes/tests.rs b/rs/nns/governance/src/storage/neuron_indexes/tests.rs index 4cd4ad88431..77d3601e925 100644 --- a/rs/nns/governance/src/storage/neuron_indexes/tests.rs +++ b/rs/nns/governance/src/storage/neuron_indexes/tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::{ governance::{Governance, MockEnvironment}, - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::{ neuron::{DissolveState, Followees}, Governance as GovernanceProto, KnownNeuronData, diff --git a/rs/nns/governance/src/storage/neurons.rs b/rs/nns/governance/src/storage/neurons.rs index c16be739329..0da5b589b89 100644 --- a/rs/nns/governance/src/storage/neurons.rs +++ b/rs/nns/governance/src/storage/neurons.rs @@ -1,5 +1,5 @@ use crate::{ - neuron::types::{DecomposedNeuron, Neuron}, + neuron::{DecomposedNeuron, Neuron}, neuron_store::NeuronStoreError, pb::v1::{ neuron::Followees, AbridgedNeuron, BallotInfo, KnownNeuronData, NeuronStakeTransfer, Topic, diff --git a/rs/nns/governance/src/storage/neurons/neurons_tests.rs b/rs/nns/governance/src/storage/neurons/neurons_tests.rs index 0614d557463..51128874f37 100644 --- a/rs/nns/governance/src/storage/neurons/neurons_tests.rs +++ b/rs/nns/governance/src/storage/neurons/neurons_tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::{ - neuron::types::{DissolveStateAndAge, NeuronBuilder}, + neuron::{DissolveStateAndAge, NeuronBuilder}, pb::v1::{abridged_neuron::DissolveState, Vote}, }; use ic_base_types::PrincipalId;