refactor(nns): move random subaccount generation into NeuronStore (#9185)

### Why

Random subaccount generation in `create_neuron`, `split_neuron`, and
`spawn_neuron` was duplicated inline with a single-shot collision check
that would fail on collision rather than retry. This is inconsistent
with `new_neuron_id()` which retries.

### What

- Add `NeuronStore::new_neuron_subaccount()` which generates a unique
random subaccount with retry-on-collision, mirroring `new_neuron_id()`
- Update `create_neuron`, `split_neuron`, and `spawn_neuron` to use it
for the random subaccount path
- Deterministic subaccount paths (from user-supplied memo/nonce) are
unchanged

### PR Chain
- ➡️ Next: #9186

### Testing
- Added unit tests for `new_neuron_subaccount` (no-collision and
retry-on-collision cases)

Author: jasonz-dfinity

rs/nns/governance/src/governance.rs

@@ -2203,20 +2203,24 @@ impl Governance {
 
         let from_subaccount = parent_neuron.subaccount();
 
-        let to_subaccount_bytes = if let Some(memo) = memo {
-            ledger::compute_neuron_split_subaccount_bytes(parent_neuron.controller(), memo)
+        let to_subaccount = if let Some(memo) = memo {
+            let to_subaccount = Subaccount(ledger::compute_neuron_split_subaccount_bytes(
+                parent_neuron.controller(),
+                memo,
+            ));
+            // Deterministic subaccount: fail immediately on collision since
+            // retrying would produce the same result.
+            if self.neuron_store.has_neuron_with_subaccount(to_subaccount) {
+                return Err(GovernanceError::new_with_message(
+                    ErrorType::PreconditionFailed,
+                    "There is already a neuron with the same subaccount.",
+                ));
+            }
+            to_subaccount
         } else {
-            self.randomness.random_byte_array()?
+            self.neuron_store
+                .new_neuron_subaccount(&mut *self.randomness)?
         };
-        let to_subaccount = Subaccount(to_subaccount_bytes);
-
-        // Make sure there isn't already a neuron with the same sub-account.
-        if self.neuron_store.has_neuron_with_subaccount(to_subaccount) {
-            return Err(GovernanceError::new_with_message(
-                ErrorType::PreconditionFailed,
-                "There is already a neuron with the same subaccount.",
-            ));
-        }
 
         let in_flight_command = NeuronInFlightCommand {
             timestamp: created_timestamp_seconds,
@@ -2671,22 +2675,26 @@ impl Governance {
 
         let child_nid = self.neuron_store.new_neuron_id(&mut *self.randomness)?;
 
-        // use provided sub-account if any, otherwise generate a random one.
+        // Use provided sub-account if any, otherwise generate a random one.
         let to_subaccount = match spawn.nonce {
-            None => Subaccount(self.randomness.random_byte_array()?),
+            None => self
+                .neuron_store
+                .new_neuron_subaccount(&mut *self.randomness)?,
             Some(nonce_val) => {
-                ledger::compute_neuron_staking_subaccount(child_controller, nonce_val)
+                let to_subaccount =
+                    ledger::compute_neuron_staking_subaccount(child_controller, nonce_val);
+                // Deterministic subaccount: fail immediately on collision since
+                // retrying would produce the same result.
+                if self.neuron_store.has_neuron_with_subaccount(to_subaccount) {
+                    return Err(GovernanceError::new_with_message(
+                        ErrorType::PreconditionFailed,
+                        "There is already a neuron with the same subaccount.",
+                    ));
+                }
+                to_subaccount
             }
         };
 
-        // Make sure there isn't already a neuron with the same sub-account.
-        if self.neuron_store.has_neuron_with_subaccount(to_subaccount) {
-            return Err(GovernanceError::new_with_message(
-                ErrorType::PreconditionFailed,
-                "There is already a neuron with the same subaccount.",
-            ));
-        }
-
         let created_timestamp_seconds = self.env.now();
         let dissolve_and_spawn_at_timestamp_seconds =
             created_timestamp_seconds + economics.neuron_spawn_dissolve_delay_seconds;

rs/nns/governance/src/governance/create_neuron.rs

@@ -14,7 +14,6 @@ use ic_cdk::println;
 use ic_nns_constants::GOVERNANCE_CANISTER_ID;
 use ic_nns_governance_api::{CreateNeuronRequest, CreatedNeuron};
 use ic_types::PrincipalId;
-use icp_ledger::Subaccount;
 use icrc_ledger_types::icrc1::account::Account as Icrc1Account;
 use std::{cell::RefCell, thread::LocalKey};
 
@@ -96,28 +95,9 @@ impl Governance {
                     NEURON_RATE_LIMITER_KEY.to_string(),
                     1,
                 )?;
-                let neuron_subaccount =
-                    governance.randomness.random_byte_array().map_err(|_| {
-                        GovernanceError::new_with_message(
-                            ErrorType::Unavailable,
-                            "Failed to generate neuron subaccount",
-                        )
-                    })?;
-                let neuron_subaccount = Subaccount(neuron_subaccount);
-                if governance
+                let neuron_subaccount = governance
                     .neuron_store
-                    .has_neuron_with_subaccount(neuron_subaccount)
-                {
-                    println!(
-                        "{LOG_PREFIX}Warning: An improbable event has occurred: a neuron \
-                    subaccount was generated randomly but there is already a neuron with the same \
-                    subaccount."
-                    );
-                    return Err(GovernanceError::new_with_message(
-                        ErrorType::Unavailable,
-                        "There is already a neuron with the same subaccount.",
-                    ));
-                }
+                    .new_neuron_subaccount(&mut *governance.randomness)?;
                 let neuron_id = governance
                     .neuron_store
                     .new_neuron_id(&mut *governance.randomness)?;

rs/nns/governance/src/neuron_store.rs

@@ -68,6 +68,7 @@ pub enum NeuronStoreError {
         neuron_id: NeuronId,
     },
     NeuronIdGenerationUnavailable,
+    NeuronSubaccountGenerationUnavailable,
     InvalidOperation {
         reason: String,
     },
@@ -172,6 +173,13 @@ impl Display for NeuronStoreError {
                     Likely due to uninitialized RNG."
                 )
             }
+            NeuronStoreError::NeuronSubaccountGenerationUnavailable => {
+                write!(
+                    f,
+                    "Neuron subaccount generation is not available currently. \
+                    Likely due to uninitialized RNG."
+                )
+            }
             NeuronStoreError::InvalidOperation { reason } => {
                 write!(f, "Invalid operation: {reason}")
             }
@@ -198,6 +206,7 @@ impl From<NeuronStoreError> for GovernanceError {
             NeuronStoreError::InvalidData { .. } => ErrorType::PreconditionFailed,
             NeuronStoreError::NotAuthorizedToGetFullNeuron { .. } => ErrorType::NotAuthorized,
             NeuronStoreError::NeuronIdGenerationUnavailable => ErrorType::Unavailable,
+            NeuronStoreError::NeuronSubaccountGenerationUnavailable => ErrorType::Unavailable,
             NeuronStoreError::InvalidOperation { .. } => ErrorType::PreconditionFailed,
             NeuronStoreError::TotalPotentialVotingPowerOverflow => ErrorType::PreconditionFailed,
             NeuronStoreError::TotalDecidingVotingPowerOverflow => ErrorType::PreconditionFailed,
@@ -304,6 +313,32 @@ impl NeuronStore {
         }
     }
 
+    /// Generates a unique random neuron subaccount, retrying on collision.
+    pub fn new_neuron_subaccount(
+        &self,
+        random: &mut dyn RandomnessGenerator,
+    ) -> Result<Subaccount, NeuronStoreError> {
+        loop {
+            let subaccount = Subaccount(
+                random
+                    .random_byte_array()
+                    .map_err(|_| NeuronStoreError::NeuronSubaccountGenerationUnavailable)?,
+            );
+
+            if !self.has_neuron_with_subaccount(subaccount) {
+                return Ok(subaccount);
+            }
+
+            ic_cdk::println!(
+                "{}WARNING: A suspiciously near-impossible event has just occurred: \
+                 we randomly picked a neuron subaccount, but it's already used: \
+                 {:?}. Trying again...",
+                LOG_PREFIX,
+                subaccount,
+            );
+        }
+    }
+
     /// Returns if store contains a Neuron by id
     pub fn contains(&self, neuron_id: NeuronId) -> bool {
         with_stable_neuron_store(|stable_neuron_store| stable_neuron_store.contains(neuron_id))

rs/nns/governance/src/neuron_store/neuron_store_tests.rs

@@ -1,6 +1,6 @@
 use super::*;
 use crate::{
-    governance::max_dissolve_delay_seconds,
+    governance::{RandomnessGenerator, RngError, max_dissolve_delay_seconds},
     neuron::{DissolveStateAndAge, NeuronBuilder},
     pb::v1::{
         BallotInfo, Followees, KnownNeuronData, MaturityDisbursement, NeuronDissolveStateSnapshot,
@@ -1194,3 +1194,80 @@ fn test_clamp_dissolve_delay_for_all_neurons_multiple_neurons() {
         })
         .unwrap();
 }
+
+/// A mock RNG that returns predefined byte arrays in sequence.
+struct SequentialMockRng {
+    byte_arrays: Vec<[u8; 32]>,
+    index: usize,
+}
+
+impl SequentialMockRng {
+    fn new(byte_arrays: Vec<[u8; 32]>) -> Self {
+        Self {
+            byte_arrays,
+            index: 0,
+        }
+    }
+}
+
+impl RandomnessGenerator for SequentialMockRng {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        unimplemented!("not needed for subaccount tests")
+    }
+
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
+        let result = *self
+            .byte_arrays
+            .get(self.index)
+            .expect("SequentialMockRng exhausted predefined byte arrays");
+        self.index += 1;
+        Ok(result)
+    }
+
+    fn seed_rng(&mut self, _seed: [u8; 32]) {}
+
+    fn get_rng_seed(&self) -> Option<[u8; 32]> {
+        None
+    }
+}
+
+#[test]
+fn test_new_neuron_subaccount_succeeds_without_collision() {
+    let neuron_store = NeuronStore::new(BTreeMap::new());
+
+    let expected_bytes = [42_u8; 32];
+    let mut rng = SequentialMockRng::new(vec![expected_bytes]);
+
+    let observed = neuron_store.new_neuron_subaccount(&mut rng);
+
+    assert_eq!(observed, Ok(Subaccount(expected_bytes)));
+}
+
+#[test]
+fn test_new_neuron_subaccount_retries_on_collision() {
+    let colliding_bytes = [1_u8; 32];
+    let unique_bytes = [2_u8; 32];
+
+    // Create a neuron store with a neuron whose subaccount matches colliding_bytes.
+    let neuron = NeuronBuilder::new(
+        NeuronId { id: 1 },
+        Subaccount(colliding_bytes),
+        PrincipalId::new_user_test_id(1),
+        DissolveStateAndAge::NotDissolving {
+            dissolve_delay_seconds: 1,
+            aging_since_timestamp_seconds: 0,
+        },
+        CREATED_TIMESTAMP_SECONDS,
+    )
+    .build();
+    let neuron_store = NeuronStore::new(btreemap! { 1 => neuron });
+
+    // The first call returns the colliding subaccount; the second returns a unique one.
+    let mut rng = SequentialMockRng::new(vec![colliding_bytes, unique_bytes]);
+
+    let observed = neuron_store.new_neuron_subaccount(&mut rng);
+
+    assert_eq!(observed, Ok(Subaccount(unique_bytes)));
+    // Verify that both byte arrays were consumed (i.e., a retry happened).
+    assert_eq!(rng.index, 2);
+}