diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index be098e14eb0..ab5aaebc3a1 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -3441,6 +3441,17 @@ impl Governance { MAX_DISSOLVE_DELAY_SECONDS, ); + let dissolve_state_and_age = if dissolve_delay_seconds > 0 { + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds: created_timestamp_seconds, + } + } else { + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: created_timestamp_seconds, + } + }; + // Before we do the transfer, we need to save the neuron in the map // otherwise a trap after the transfer is successful but before this // method finishes would cause the funds to be lost. @@ -3451,10 +3462,7 @@ impl Governance { child_nid, to_subaccount, child_controller, - DissolveStateAndAge::NotDissolving { - dissolve_delay_seconds, - aging_since_timestamp_seconds: created_timestamp_seconds, - }, + dissolve_state_and_age, created_timestamp_seconds, ) .with_followees(self.heap_data.default_followees.clone()) @@ -4205,15 +4213,24 @@ impl Governance { reward_to_neuron.dissolve_delay_seconds, MAX_DISSOLVE_DELAY_SECONDS, ); + + let dissolve_state_and_age = if dissolve_delay_seconds > 0 { + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds, + aging_since_timestamp_seconds: now, + } + } else { + DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds: now, + } + }; + // Transfer successful. let neuron = NeuronBuilder::new( nid, to_subaccount, *np_principal, - DissolveStateAndAge::NotDissolving { - dissolve_delay_seconds, - aging_since_timestamp_seconds: now, - }, + dissolve_state_and_age, now, ) .with_followees(self.heap_data.default_followees.clone()) diff --git a/rs/nns/governance/src/neuron/dissolve_state_and_age.rs b/rs/nns/governance/src/neuron/dissolve_state_and_age.rs index 4b93e727cba..cd46233edcc 100644 --- a/rs/nns/governance/src/neuron/dissolve_state_and_age.rs +++ b/rs/nns/governance/src/neuron/dissolve_state_and_age.rs @@ -1,4 +1,6 @@ -use crate::{governance::MAX_DISSOLVE_DELAY_SECONDS, pb::v1::NeuronState}; +use crate::{ + governance::MAX_DISSOLVE_DELAY_SECONDS, neuron::StoredDissolveStateAndAge, 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 @@ -20,6 +22,22 @@ pub enum DissolveStateAndAge { } impl DissolveStateAndAge { + pub fn validate(self) -> Result { + let original = self; + let stored_dissolve_state_and_age = StoredDissolveStateAndAge::from(self); + + let validated_dissolve_state_and_age = Self::try_from(stored_dissolve_state_and_age) + .map_err(|e| format!("Invalid dissolve state and age: {}", e))?; + + if validated_dissolve_state_and_age != original { + return Err( format!( + "Dissolve state and age is not valid, as it does not serialize/deserialize symmetrically. In: {:?}, Out: {:?}", + original, validated_dissolve_state_and_age + )); + } + + Ok(self) + } /// 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 { diff --git a/rs/nns/governance/src/neuron/types.rs b/rs/nns/governance/src/neuron/types.rs index 9c60e6648cc..77ece75d423 100644 --- a/rs/nns/governance/src/neuron/types.rs +++ b/rs/nns/governance/src/neuron/types.rs @@ -23,8 +23,7 @@ use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use icp_ledger::Subaccount; use std::collections::{BTreeSet, HashMap}; -/// A neuron type internal to the governance crate. Currently, this type is identical to the -/// prost-generated Neuron type (except for derivations for prost). Gradually, this type will evolve +/// A neuron type internal to the governance crate. Gradually, this type will evolve /// towards having all private fields while exposing methods for mutations, which allows it to hold /// invariants. #[derive(Clone, Debug)] @@ -1681,9 +1680,13 @@ impl TryFrom for DissolveStateAndAge { when_dissolved_timestamp_seconds, ) => { if aging_since_timestamp_seconds == u64::MAX { - Ok(DissolveStateAndAge::DissolvingOrDissolved { - when_dissolved_timestamp_seconds, - }) + if when_dissolved_timestamp_seconds > 0 { + Ok(DissolveStateAndAge::DissolvingOrDissolved { + when_dissolved_timestamp_seconds, + }) + } else { + Err("Dissolve delay must be greater than 0".to_string()) + } } else { Err("Aging since timestamp must be u64::MAX for dissolving or dissolved neurons".to_string()) } diff --git a/rs/nns/governance/src/neuron/types/tests.rs b/rs/nns/governance/src/neuron/types/tests.rs index 60e0fcf132e..b6e81c5230b 100644 --- a/rs/nns/governance/src/neuron/types/tests.rs +++ b/rs/nns/governance/src/neuron/types/tests.rs @@ -6,6 +6,7 @@ use crate::{ }; use ic_cdk::println; +use crate::pb::v1::neuron::DissolveState; use ic_nervous_system_common::{E8, ONE_YEAR_SECONDS}; use ic_stable_structures::Storable; use icp_ledger::Subaccount; @@ -88,6 +89,13 @@ fn test_dissolve_state_and_age_conversion_failure() { }, "Dissolve delay must be greater than 0", ), + ( + StoredDissolveStateAndAge { + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + aging_since_timestamp_seconds: u64::MAX, + }, + "Dissolve delay must be greater than 0", + ), ]; for (invalid_stored_dissolve_state_and_age, error) in test_cases { diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs index 3515d0f1cc4..5eedc59ea9f 100644 --- a/rs/nns/governance/src/neuron_store.rs +++ b/rs/nns/governance/src/neuron_store.rs @@ -451,6 +451,8 @@ impl NeuronStore { pub fn add_neuron(&mut self, neuron: Neuron) -> Result { let neuron_id = neuron.id(); + self.validate_neuron(&neuron)?; + if self.contains(neuron_id) { return Err(NeuronStoreError::NeuronAlreadyExists(neuron_id)); } @@ -472,6 +474,17 @@ impl NeuronStore { Ok(neuron_id) } + fn validate_neuron(&self, neuron: &Neuron) -> Result<(), NeuronStoreError> { + neuron + .dissolve_state_and_age() + .validate() + .map_err(|reason| NeuronStoreError::InvalidData { + reason: format!("Neuron cannot be saved: {}", reason), + })?; + + Ok(()) + } + fn add_neuron_to_indexes(&mut self, neuron: &Neuron) { if let Err(error) = with_stable_neuron_indexes_mut(|indexes| indexes.add_neuron(neuron)) { println!( @@ -619,6 +632,8 @@ impl NeuronStore { }; let is_neuron_changed = *old_neuron != new_neuron; + self.validate_neuron(&new_neuron)?; + // Perform transition between 2 storage if necessary. // // Note: diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 2f8bdbd2880..22469da2496 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -1995,7 +1995,7 @@ fn fixture_for_manage_neuron() -> GovernanceProto { .random_byte_array() .expect("Could not get random byte array") .to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }; @@ -3917,6 +3917,8 @@ fn fixture_for_approve_kyc() -> GovernanceProto { let network_economics = NetworkEconomics::with_default_values(); + let now = driver.now(); + let neurons = vec![ Neuron { id: Some(NeuronId { id: 1 }), @@ -3927,7 +3929,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto { .expect("Could not get random byte array") .to_vec(), kyc_verified: false, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -3940,7 +3942,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto { .expect("Could not get random byte array") .to_vec(), kyc_verified: false, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -3953,7 +3955,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto { .expect("Could not get random byte array") .to_vec(), kyc_verified: false, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -3966,7 +3968,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto { .expect("Could not get random byte array") .to_vec(), kyc_verified: false, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -4140,7 +4142,7 @@ fn test_get_neuron_ids_by_principal() { .random_byte_array() .expect("Could not get random byte array") .to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }; @@ -4151,7 +4153,7 @@ fn test_get_neuron_ids_by_principal() { .random_byte_array() .expect("Could not get random byte array") .to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }; @@ -4162,7 +4164,7 @@ fn test_get_neuron_ids_by_principal() { .random_byte_array() .expect("Could not get random byte array") .to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }; @@ -4174,7 +4176,7 @@ fn test_get_neuron_ids_by_principal() { .random_byte_array() .expect("Could not get random byte array") .to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }; @@ -7880,7 +7882,7 @@ fn test_filter_proposals_neuron_visibility() { hot_keys: vec![principal_hot], cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -7889,7 +7891,7 @@ fn test_filter_proposals_neuron_visibility() { controller: Some(principal2), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -7903,7 +7905,7 @@ fn test_filter_proposals_neuron_visibility() { followees: vec![NeuronId { id: 1 }], }, }, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -7974,7 +7976,7 @@ fn test_filter_proposals_include_all_manage_neuron_ignores_visibility() { controller: Some(principal1), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -7983,7 +7985,7 @@ fn test_filter_proposals_include_all_manage_neuron_ignores_visibility() { controller: Some(principal2), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -7997,7 +7999,7 @@ fn test_filter_proposals_include_all_manage_neuron_ignores_visibility() { followees: vec![NeuronId { id: 1 }], }, }, - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -8091,7 +8093,7 @@ fn test_filter_proposals_by_status() { controller: Some(principal1), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() } @@ -8191,7 +8193,7 @@ fn test_filter_proposals_by_reward_status() { controller: Some(principal1), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() } @@ -8282,7 +8284,7 @@ fn test_filter_proposals_excluding_topics() { controller: Some(principal1), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() } @@ -8378,7 +8380,7 @@ fn test_filter_proposal_ballots() { hot_keys: vec![principal_hot], cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -8388,7 +8390,7 @@ fn test_filter_proposal_ballots() { controller: Some(principal2), cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -8567,7 +8569,7 @@ fn test_omit_large_fields() { hot_keys: vec![], cached_neuron_stake_e8s: 10 * E8, account: driver.random_byte_array().expect("Could not get random byte array").to_vec(), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Default::default() }, @@ -9580,7 +9582,7 @@ fn test_join_neurons_fund() { account: account(1), cached_neuron_stake_e8s: 10 * E8, controller: Some(principal(principal_a)), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Neuron::default() }, @@ -9589,7 +9591,7 @@ fn test_join_neurons_fund() { account: account(2), cached_neuron_stake_e8s: 20 * 100_000_000, controller: Some(principal(principal_b)), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Neuron::default() }, @@ -9598,7 +9600,7 @@ fn test_join_neurons_fund() { account: account(3), cached_neuron_stake_e8s: 100 * 100_000_000, controller: Some(principal(principal_b)), - dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)), + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), aging_since_timestamp_seconds: u64::MAX, ..Neuron::default() } @@ -13427,7 +13429,7 @@ async fn test_metrics() { controller: Some(principal(4)), cached_neuron_stake_e8s: 400_000_000, dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds( - 0, + 1, )), aging_since_timestamp_seconds: u64::MAX, ..Default::default() diff --git a/rs/nns/governance/tests/interleaving_tests.rs b/rs/nns/governance/tests/interleaving_tests.rs index 992678c0f37..ab5f44c40bf 100644 --- a/rs/nns/governance/tests/interleaving_tests.rs +++ b/rs/nns/governance/tests/interleaving_tests.rs @@ -56,7 +56,7 @@ fn test_cant_increase_dissolve_delay_while_disbursing() { let nns = NNSBuilder::new() .add_neuron( NeuronBuilder::new(neuron_id_u64, 10, owner) - .set_dissolve_state(Some(DissolveState::WhenDissolvedTimestampSeconds(0))) + .set_dissolve_state(Some(DissolveState::WhenDissolvedTimestampSeconds(1))) .set_kyc_verified(true), ) .add_ledger_transform(Box::new(move |l| { @@ -238,7 +238,7 @@ fn test_cant_interleave_calls_to_settle_neurons_fund() { }) .add_neuron( NeuronBuilder::new(nf_neuron_id_u64, 100, nf_neurons_controller) - .set_dissolve_state(Some(DissolveState::WhenDissolvedTimestampSeconds(0))) + .set_dissolve_state(Some(DissolveState::WhenDissolvedTimestampSeconds(1))) .set_maturity(nf_neuron_maturity) .set_joined_community_fund(100), )