From 0c1d522722918223b9b6ec8ad85e98746f53ab46 Mon Sep 17 00:00:00 2001
From: Max Summe <maximilian.summe@dfinity.org>
Date: Wed, 23 Oct 2024 16:48:09 -0700
Subject: [PATCH 1/4] fix(nns): minor validation fix in edge case affecting
 tests

---
 rs/nns/governance/src/neuron/types.rs       | 13 ++++++++-----
 rs/nns/governance/src/neuron/types/tests.rs |  8 ++++++++
 rs/nns/governance/tests/governance.rs       | 10 ++++++----
 3 files changed, 22 insertions(+), 9 deletions(-)

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<StoredDissolveStateAndAge> 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/tests/governance.rs b/rs/nns/governance/tests/governance.rs
index 2f8bdbd2880..19bd622da3c 100644
--- a/rs/nns/governance/tests/governance.rs
+++ b/rs/nns/governance/tests/governance.rs
@@ -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()
         },

From 5608c3c186acdc0b01557dbe8c09ef14afd9d543 Mon Sep 17 00:00:00 2001
From: Max Summe <maximilian.summe@dfinity.org>
Date: Fri, 25 Oct 2024 11:44:59 -0700
Subject: [PATCH 2/4] Pull other relevant changes from branch

---
 rs/nns/governance/src/governance.rs              | 16 ++++++++++++----
 .../src/neuron/dissolve_state_and_age.rs         | 15 +++++++++++++++
 rs/nns/governance/src/neuron_store.rs            |  8 ++++++++
 rs/nns/governance/tests/governance.rs            | 10 ++++------
 4 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs
index be098e14eb0..3f6ab32fa98 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())
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..ea5c52559eb 100644
--- a/rs/nns/governance/src/neuron/dissolve_state_and_age.rs
+++ b/rs/nns/governance/src/neuron/dissolve_state_and_age.rs
@@ -20,6 +20,21 @@ pub enum DissolveStateAndAge {
 }
 
 impl DissolveStateAndAge {
+    pub fn validate(self) -> Result<Self, String> {
+        match self {
+            DissolveStateAndAge::NotDissolving {
+                dissolve_delay_seconds,
+                aging_since_timestamp_seconds: _,
+            } => {
+                if dissolve_delay_seconds == 0 {
+                    return Err("Dissolve delay must be greater than 0".to_string());
+                }
+            }
+            DissolveStateAndAge::DissolvingOrDissolved { .. } => {}
+        };
+
+        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_store.rs b/rs/nns/governance/src/neuron_store.rs
index 3515d0f1cc4..ecf71342b88 100644
--- a/rs/nns/governance/src/neuron_store.rs
+++ b/rs/nns/governance/src/neuron_store.rs
@@ -451,6 +451,14 @@ impl NeuronStore {
     pub fn add_neuron(&mut self, neuron: Neuron) -> Result<NeuronId, NeuronStoreError> {
         let neuron_id = neuron.id();
 
+        // Don't write invalid data
+        neuron
+            .dissolve_state_and_age()
+            .validate()
+            .map_err(|reason| NeuronStoreError::InvalidData {
+                reason: format!("Neuron is invalid: {}", reason),
+            })?;
+
         if self.contains(neuron_id) {
             return Err(NeuronStoreError::NeuronAlreadyExists(neuron_id));
         }
diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs
index 19bd622da3c..2f8bdbd2880 100644
--- a/rs/nns/governance/tests/governance.rs
+++ b/rs/nns/governance/tests/governance.rs
@@ -3917,8 +3917,6 @@ 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 }),
@@ -3929,7 +3927,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto {
                 .expect("Could not get random byte array")
                 .to_vec(),
             kyc_verified: false,
-            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)),
+            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)),
             aging_since_timestamp_seconds: u64::MAX,
             ..Default::default()
         },
@@ -3942,7 +3940,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto {
                 .expect("Could not get random byte array")
                 .to_vec(),
             kyc_verified: false,
-            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)),
+            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)),
             aging_since_timestamp_seconds: u64::MAX,
             ..Default::default()
         },
@@ -3955,7 +3953,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto {
                 .expect("Could not get random byte array")
                 .to_vec(),
             kyc_verified: false,
-            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)),
+            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)),
             aging_since_timestamp_seconds: u64::MAX,
             ..Default::default()
         },
@@ -3968,7 +3966,7 @@ fn fixture_for_approve_kyc() -> GovernanceProto {
                 .expect("Could not get random byte array")
                 .to_vec(),
             kyc_verified: false,
-            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(now)),
+            dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)),
             aging_since_timestamp_seconds: u64::MAX,
             ..Default::default()
         },

From 040deb5fd2bad73f6b5665705d9dd093f4dec12d Mon Sep 17 00:00:00 2001
From: Max Summe <maximilian.summe@dfinity.org>
Date: Fri, 25 Oct 2024 13:24:52 -0700
Subject: [PATCH 3/4] fix better

---
 rs/nns/governance/src/neuron_store.rs         | 42 ++++++++++++---
 rs/nns/governance/tests/governance.rs         | 52 ++++++++++---------
 rs/nns/governance/tests/interleaving_tests.rs |  4 +-
 3 files changed, 64 insertions(+), 34 deletions(-)

diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs
index ecf71342b88..bec0aad8fb1 100644
--- a/rs/nns/governance/src/neuron_store.rs
+++ b/rs/nns/governance/src/neuron_store.rs
@@ -34,6 +34,7 @@ use std::{
 };
 
 pub mod metrics;
+use crate::neuron::{DissolveStateAndAge, StoredDissolveStateAndAge};
 pub(crate) use metrics::NeuronMetrics;
 
 #[derive(Eq, PartialEq, Debug)]
@@ -451,13 +452,7 @@ impl NeuronStore {
     pub fn add_neuron(&mut self, neuron: Neuron) -> Result<NeuronId, NeuronStoreError> {
         let neuron_id = neuron.id();
 
-        // Don't write invalid data
-        neuron
-            .dissolve_state_and_age()
-            .validate()
-            .map_err(|reason| NeuronStoreError::InvalidData {
-                reason: format!("Neuron is invalid: {}", reason),
-            })?;
+        self.validate_neuron(&neuron)?;
 
         if self.contains(neuron_id) {
             return Err(NeuronStoreError::NeuronAlreadyExists(neuron_id));
@@ -480,6 +475,37 @@ impl NeuronStore {
         Ok(neuron_id)
     }
 
+    fn validate_neuron(&self, neuron: &Neuron) -> Result<(), NeuronStoreError> {
+        let dissolve_state_and_age = neuron.dissolve_state_and_age();
+
+        let stored_dissolve_state_and_age = StoredDissolveStateAndAge::from(dissolve_state_and_age);
+
+        let validated_dissolve_state_and_age =
+            DissolveStateAndAge::try_from(stored_dissolve_state_and_age).map_err(|e| {
+                NeuronStoreError::InvalidData {
+                    reason: format!("Invalid dissolve state and age: {}", e),
+                }
+            })?;
+
+        if validated_dissolve_state_and_age != dissolve_state_and_age {
+            return Err(NeuronStoreError::InvalidData {
+                reason: format!(
+                    "Dissolve state and age is not valid, as it reads and writes in a different shape. In: {:?}, Out: {:?}",
+                    dissolve_state_and_age, validated_dissolve_state_and_age
+                ),
+            });
+        }
+
+        neuron
+            .dissolve_state_and_age()
+            .validate()
+            .map_err(|reason| NeuronStoreError::InvalidData {
+                reason: format!("Neuron is invalid: {}", 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!(
@@ -627,6 +653,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),
         )

From 45dfc1f981ae75e1fc61f171cd02249b5f344df2 Mon Sep 17 00:00:00 2001
From: Max Summe <maximilian.summe@dfinity.org>
Date: Fri, 25 Oct 2024 13:29:48 -0700
Subject: [PATCH 4/4] one other edge case

---
 rs/nns/governance/src/governance.rs           | 17 +++++++++---
 .../src/neuron/dissolve_state_and_age.rs      | 27 ++++++++++---------
 rs/nns/governance/src/neuron_store.rs         | 23 +---------------
 3 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs
index 3f6ab32fa98..ab5aaebc3a1 100644
--- a/rs/nns/governance/src/governance.rs
+++ b/rs/nns/governance/src/governance.rs
@@ -4213,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 ea5c52559eb..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
@@ -21,17 +23,18 @@ pub enum DissolveStateAndAge {
 
 impl DissolveStateAndAge {
     pub fn validate(self) -> Result<Self, String> {
-        match self {
-            DissolveStateAndAge::NotDissolving {
-                dissolve_delay_seconds,
-                aging_since_timestamp_seconds: _,
-            } => {
-                if dissolve_delay_seconds == 0 {
-                    return Err("Dissolve delay must be greater than 0".to_string());
-                }
-            }
-            DissolveStateAndAge::DissolvingOrDissolved { .. } => {}
-        };
+        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)
     }
diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs
index bec0aad8fb1..5eedc59ea9f 100644
--- a/rs/nns/governance/src/neuron_store.rs
+++ b/rs/nns/governance/src/neuron_store.rs
@@ -34,7 +34,6 @@ use std::{
 };
 
 pub mod metrics;
-use crate::neuron::{DissolveStateAndAge, StoredDissolveStateAndAge};
 pub(crate) use metrics::NeuronMetrics;
 
 #[derive(Eq, PartialEq, Debug)]
@@ -476,31 +475,11 @@ impl NeuronStore {
     }
 
     fn validate_neuron(&self, neuron: &Neuron) -> Result<(), NeuronStoreError> {
-        let dissolve_state_and_age = neuron.dissolve_state_and_age();
-
-        let stored_dissolve_state_and_age = StoredDissolveStateAndAge::from(dissolve_state_and_age);
-
-        let validated_dissolve_state_and_age =
-            DissolveStateAndAge::try_from(stored_dissolve_state_and_age).map_err(|e| {
-                NeuronStoreError::InvalidData {
-                    reason: format!("Invalid dissolve state and age: {}", e),
-                }
-            })?;
-
-        if validated_dissolve_state_and_age != dissolve_state_and_age {
-            return Err(NeuronStoreError::InvalidData {
-                reason: format!(
-                    "Dissolve state and age is not valid, as it reads and writes in a different shape. In: {:?}, Out: {:?}",
-                    dissolve_state_and_age, validated_dissolve_state_and_age
-                ),
-            });
-        }
-
         neuron
             .dissolve_state_and_age()
             .validate()
             .map_err(|reason| NeuronStoreError::InvalidData {
-                reason: format!("Neuron is invalid: {}", reason),
+                reason: format!("Neuron cannot be saved: {}", reason),
             })?;
 
         Ok(())