Change signature of random methods to return errors if uninitialized RNG

rs/nns/governance/benches/scale.rs

@@ -20,7 +20,7 @@ use ic_base_types::{CanisterId, PrincipalId};
 use ic_nervous_system_common::{cmc::FakeCmc, ledger::IcpLedger, NervousSystemError};
 use ic_nns_common::pb::v1::NeuronId;
 use ic_nns_governance::{
-    governance::{Environment, Governance, HeapGrowthPotential},
+    governance::{Environment, Governance, HeapGrowthPotential, RngError},
     pb::v1::{
         neuron, proposal, ExecuteNnsFunction, Governance as GovernanceProto, GovernanceError,
         Motion, NetworkEconomics, Neuron, Proposal, Topic,
@@ -48,11 +48,11 @@ impl Environment for MockEnvironment {
         self.secs
     }
 
-    fn random_u64(&mut self) -> u64 {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
         todo!()
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         todo!()
     }
 

rs/nns/governance/canister/canister.rs

@@ -20,7 +20,7 @@ use ic_nns_common::{
 use ic_nns_constants::LEDGER_CANISTER_ID;
 use ic_nns_governance::{
     decoder_config, encode_metrics,
-    governance::{Environment, Governance, HeapGrowthPotential, TimeWarp as GovTimeWarp},
+    governance::{Environment, Governance, HeapGrowthPotential, RngError, TimeWarp as GovTimeWarp},
     neuron_data_validation::NeuronDataValidationSummary,
     pb::v1::{self as gov_pb, Governance as InternalGovernanceProto},
     storage::{grow_upgrades_memory_to, validate_stable_storage, with_upgrades_memory},
@@ -206,14 +206,14 @@ impl Environment for CanisterEnv {
         self.time_warp = new_time_warp;
     }
 
-    fn random_u64(&mut self) -> u64 {
-        self.rng.next_u64()
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        Ok(self.rng.next_u64())
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         let mut bytes = [0u8; 32];
         self.rng.fill_bytes(&mut bytes);
-        bytes
+        Ok(bytes)
     }
 
     fn execute_nns_function(

rs/nns/governance/src/governance.rs

@@ -1451,8 +1451,24 @@ impl fmt::Display for RewardEvent {
     }
 }
 
+#[derive(Debug)]
+pub enum RngError {
+    RngNotInitialized,
+}
+
+impl From<RngError> for GovernanceError {
+    fn from(e: RngError) -> Self {
+        match e {
+            RngError::RngNotInitialized => GovernanceError::new_with_message(
+                ErrorType::PreconditionFailed,
+                "Rng not initialized.  Try again later.".to_string(),
+            ),
+        }
+    }
+}
+
 /// A general trait for the environment in which governance is running.
-#[automock]
+/// DO NOT MERGE - restore automock annotation
 #[async_trait]
 pub trait Environment: Send + Sync {
     /// Returns the current time, in seconds since the epoch.
@@ -1465,12 +1481,12 @@ pub trait Environment: Send + Sync {
     /// Returns a random number.
     ///
     /// This number is the same in all replicas.
-    fn random_u64(&mut self) -> u64;
+    fn random_u64(&mut self) -> Result<u64, RngError>;
 
     /// Returns a random byte array with 32 bytes.
     ///
     /// This number is the same in all replicas.
-    fn random_byte_array(&mut self) -> [u8; 32];
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError>;
 
     /// Executes a `ExecuteNnsFunction`. The standard implementation is
     /// expected to call out to another canister and eventually report the
@@ -2617,11 +2633,11 @@ impl Governance {
         }
 
         let created_timestamp_seconds = self.env.now();
-        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env);
+        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env)?;
 
         let from_subaccount = parent_neuron.subaccount();
 
-        let to_subaccount = Subaccount(self.env.random_byte_array());
+        let to_subaccount = Subaccount(self.env.random_byte_array()?);
 
         // Make sure there isn't already a neuron with the same sub-account.
         if self.neuron_store.has_neuron_with_subaccount(to_subaccount) {
@@ -3024,11 +3040,11 @@ impl Governance {
             ));
         }
 
-        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env);
+        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env)?;
 
         // use provided sub-account if any, otherwise generate a random one.
         let to_subaccount = match spawn.nonce {
-            None => Subaccount(self.env.random_byte_array()),
+            None => Subaccount(self.env.random_byte_array()?),
             Some(nonce_val) => {
                 ledger::compute_neuron_staking_subaccount(child_controller, nonce_val)
             }
@@ -3305,7 +3321,7 @@ impl Governance {
             )
         })?;
 
-        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env);
+        let child_nid = self.neuron_store.new_neuron_id(&mut *self.env)?;
         let from_subaccount = parent_neuron.subaccount();
 
         // The account is derived from the new owner's principal so it can be found by
@@ -4089,7 +4105,7 @@ impl Governance {
                 "Reward node provider proposal must have a reward mode.",
             )),
             Some(RewardMode::RewardToNeuron(reward_to_neuron)) => {
-                let to_subaccount = Subaccount(self.env.random_byte_array());
+                let to_subaccount = Subaccount(self.env.random_byte_array()?);
                 let _block_height = self
                     .ledger
                     .transfer_funds(
@@ -4100,7 +4116,7 @@ impl Governance {
                         now,
                     )
                     .await?;
-                let nid = self.neuron_store.new_neuron_id(&mut *self.env);
+                let nid = self.neuron_store.new_neuron_id(&mut *self.env)?;
                 let dissolve_delay_seconds = std::cmp::min(
                     reward_to_neuron.dissolve_delay_seconds,
                     MAX_DISSOLVE_DELAY_SECONDS,
@@ -6112,7 +6128,7 @@ impl Governance {
         controller: PrincipalId,
         claim_or_refresh: &ClaimOrRefresh,
     ) -> Result<NeuronId, GovernanceError> {
-        let nid = self.neuron_store.new_neuron_id(&mut *self.env);
+        let nid = self.neuron_store.new_neuron_id(&mut *self.env)?;
         let now = self.env.now();
         let neuron = NeuronBuilder::new(
             nid,
@@ -7816,7 +7832,10 @@ impl Governance {
     /// Picks a value at random in [00:00, 23:45] that is a multiple of 15
     /// minutes past midnight.
     pub fn randomly_pick_swap_start(&mut self) -> GlobalTimeOfDay {
-        let time_of_day_seconds = self.env.random_u64() % ONE_DAY_SECONDS;
+        // It's not critical that we have perfect randomness here, so we can default to a fixed value
+        // for the edge case where the RNG is not initialized (which should be very rare in the
+        // context of proposal executions)
+        let time_of_day_seconds = self.env.random_u64().unwrap_or(10_000) % ONE_DAY_SECONDS;
 
         // Round down to nearest multiple of 15 min.
         let remainder_seconds = time_of_day_seconds % (15 * 60);

rs/nns/governance/src/neuron_store.rs

@@ -63,6 +63,7 @@ pub enum NeuronStoreError {
         principal_id: PrincipalId,
         neuron_id: NeuronId,
     },
+    NeuronIdGenerationNotAvailable,
 }
 
 impl NeuronStoreError {
@@ -160,6 +161,13 @@ impl Display for NeuronStoreError {
                     principal_id, neuron_id
                 )
             }
+            NeuronStoreError::NeuronIdGenerationNotAvailable => {
+                write!(
+                    f,
+                    "Neuron ID generation is not available currently. \
+                    Likely due to uninitialized RNG."
+                )
+            }
         }
     }
 }
@@ -176,6 +184,7 @@ impl From<NeuronStoreError> for GovernanceError {
             NeuronStoreError::NeuronAlreadyExists(_) => ErrorType::PreconditionFailed,
             NeuronStoreError::InvalidData { .. } => ErrorType::PreconditionFailed,
             NeuronStoreError::NotAuthorizedToGetFullNeuron { .. } => ErrorType::NotAuthorized,
+            NeuronStoreError::NeuronIdGenerationNotAvailable => ErrorType::PreconditionFailed,
         };
         GovernanceError::new_with_message(error_type, value.to_string())
     }
@@ -384,10 +393,11 @@ impl NeuronStore {
         self.clock.set_time_warp(new_time_warp);
     }
 
-    pub fn new_neuron_id(&self, env: &mut dyn Environment) -> NeuronId {
+    pub fn new_neuron_id(&self, env: &mut dyn Environment) -> Result<NeuronId, NeuronStoreError> {
         loop {
             let id = env
                 .random_u64()
+                .map_err(|_| NeuronStoreError::NeuronIdGenerationNotAvailable)?
                 // Let there be no question that id was chosen
                 // intentionally, not just 0 by default.
                 .saturating_add(1);
@@ -396,7 +406,7 @@ impl NeuronStore {
             let is_unique = !self.contains(neuron_id);
 
             if is_unique {
-                return neuron_id;
+                return Ok(neuron_id);
             }
 
             ic_cdk::println!(

rs/nns/governance/src/test_utils.rs

@@ -1,5 +1,5 @@
 use crate::{
-    governance::{Environment, HeapGrowthPotential},
+    governance::{Environment, HeapGrowthPotential, RngError},
     pb::v1::{ExecuteNnsFunction, GovernanceError, OpenSnsTokenSwap},
 };
 use async_trait::async_trait;
@@ -193,11 +193,11 @@ impl Environment for MockEnvironment {
         *self.now.lock().unwrap()
     }
 
-    fn random_u64(&mut self) -> u64 {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
         unimplemented!();
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         unimplemented!();
     }
 

rs/nns/governance/tests/degraded_mode.rs

@@ -9,7 +9,8 @@ use ic_nns_common::pb::v1::NeuronId;
 use ic_nns_constants::GOVERNANCE_CANISTER_ID;
 use ic_nns_governance::{
     governance::{
-        Environment, Governance, HeapGrowthPotential, HEAP_SIZE_SOFT_LIMIT_IN_WASM32_PAGES,
+        Environment, Governance, HeapGrowthPotential, RngError,
+        HEAP_SIZE_SOFT_LIMIT_IN_WASM32_PAGES,
     },
     pb::v1::{
         governance_error::ErrorType,
@@ -35,11 +36,11 @@ impl Environment for DegradedEnv {
         111000222
     }
 
-    fn random_u64(&mut self) -> u64 {
-        4 // https://xkcd.com/221
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        Ok(4) // https://xkcd.com/221
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         unimplemented!()
     }
 

rs/nns/governance/tests/fake.rs

@@ -14,7 +14,7 @@ use ic_nns_constants::{
     SNS_WASM_CANISTER_ID,
 };
 use ic_nns_governance::{
-    governance::{Environment, Governance, HeapGrowthPotential},
+    governance::{Environment, Governance, HeapGrowthPotential, RngError},
     pb::v1::{
         manage_neuron, manage_neuron::NeuronIdOrSubaccount, manage_neuron_response, proposal,
         ExecuteNnsFunction, GovernanceError, ManageNeuron, ManageNeuronResponse, Motion,
@@ -312,14 +312,14 @@ impl Environment for FakeDriver {
         self.state.try_lock().unwrap().now
     }
 
-    fn random_u64(&mut self) -> u64 {
-        self.state.try_lock().unwrap().rng.next_u64()
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        Ok(self.state.try_lock().unwrap().rng.next_u64())
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         let mut bytes = [0u8; 32];
         self.state.try_lock().unwrap().rng.fill_bytes(&mut bytes);
-        bytes
+        Ok(bytes)
     }
 
     fn execute_nns_function(

rs/nns/governance/tests/fixtures/environment_fixture.rs

@@ -2,7 +2,7 @@ use async_trait::async_trait;
 use candid::{CandidType, Decode, Encode, Error};
 use ic_base_types::CanisterId;
 use ic_nns_governance::{
-    governance::{Environment, HeapGrowthPotential},
+    governance::{Environment, HeapGrowthPotential, RngError},
     pb::v1::{ExecuteNnsFunction, GovernanceError},
 };
 use ic_sns_root::GetSnsCanistersSummaryRequest;
@@ -145,15 +145,16 @@ impl Environment for EnvironmentFixture {
         self.environment_fixture_state.try_lock().unwrap().now
     }
 
-    fn random_u64(&mut self) -> u64 {
-        self.environment_fixture_state
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        Ok(self
+            .environment_fixture_state
             .try_lock()
             .unwrap()
             .rng
-            .next_u64()
+            .next_u64())
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         unimplemented!()
     }
 

rs/nns/governance/tests/fixtures/mod.rs

@@ -20,7 +20,8 @@ use ic_nns_common::{
 use ic_nns_constants::LEDGER_CANISTER_ID;
 use ic_nns_governance::{
     governance::{
-        governance_minting_account, neuron_subaccount, Environment, Governance, HeapGrowthPotential,
+        governance_minting_account, neuron_subaccount, Environment, Governance,
+        HeapGrowthPotential, RngError,
     },
     governance_proto_builder::GovernanceProtoBuilder,
     pb::v1::{
@@ -485,11 +486,11 @@ impl Environment for NNSFixture {
         self.nns_state.try_lock().unwrap().environment.now()
     }
 
-    fn random_u64(&mut self) -> u64 {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
         self.nns_state.try_lock().unwrap().environment.random_u64()
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         self.nns_state
             .try_lock()
             .unwrap()
@@ -942,11 +943,11 @@ impl Environment for NNS {
         self.fixture.now()
     }
 
-    fn random_u64(&mut self) -> u64 {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
         self.fixture.random_u64()
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
         self.fixture.random_byte_array()
     }