Address PR feedback

rs/nervous_system/common/src/lib.rs

@@ -778,6 +778,16 @@ fn checked_div_mod(dividend: usize, divisor: usize) -> Option<(usize, usize)> {
     Some((quotient, remainder))
 }
 
+/// Converts a sha256 hash into a hex string representation
+pub fn hash_to_hex_string(hash: &[u8]) -> String {
+    use std::fmt::Write;
+    let mut result_hash = String::new();
+    for b in hash {
+        let _ = write!(result_hash, "{:02X}", b);
+    }
+    result_hash
+}
+
 #[cfg(test)]
 mod serve_logs_tests;
 

rs/nns/sns-wasm/src/pb/mod.rs

@@ -13,6 +13,7 @@ use crate::{
 use ic_base_types::CanisterId;
 use ic_cdk::api::stable::StableMemory;
 use ic_crypto_sha2::Sha256;
+use ic_nervous_system_common::hash_to_hex_string;
 use std::{
     collections::HashMap,
     convert::TryFrom,
@@ -24,15 +25,6 @@ use std::{
 #[path = "../gen/ic_sns_wasm.pb.v1.rs"]
 pub mod v1;
 
-/// Converts a sha256 hash into a hex string representation
-pub fn hash_to_hex_string(hash: &[u8; 32]) -> String {
-    let mut result_hash = String::new();
-    for b in hash {
-        let _ = write!(result_hash, "{:02X}", b);
-    }
-    result_hash
-}
-
 impl AddWasmResponse {
     pub fn error(message: String) -> Self {
         Self {

rs/nns/sns-wasm/src/sns_wasm.rs

@@ -1,22 +1,18 @@
 use crate::{
     canister_api::CanisterApi,
-    pb::{
-        hash_to_hex_string,
-        v1::{
-            add_wasm_response, AddWasmRequest, AddWasmResponse, DappCanistersTransferResult,
-            DeployNewSnsRequest, DeployNewSnsResponse, DeployedSns,
-            GetDeployedSnsByProposalIdRequest, GetDeployedSnsByProposalIdResponse,
-            GetNextSnsVersionRequest, GetNextSnsVersionResponse, GetProposalIdThatAddedWasmRequest,
-            GetProposalIdThatAddedWasmResponse, GetSnsSubnetIdsResponse,
-            GetWasmMetadataRequest as GetWasmMetadataRequestPb,
-            GetWasmMetadataResponse as GetWasmMetadataResponsePb, GetWasmRequest, GetWasmResponse,
-            InsertUpgradePathEntriesRequest, InsertUpgradePathEntriesResponse,
-            ListDeployedSnsesRequest, ListDeployedSnsesResponse, ListUpgradeStep,
-            ListUpgradeStepsRequest, ListUpgradeStepsResponse,
-            MetadataSection as MetadataSectionPb, SnsCanisterIds, SnsCanisterType, SnsUpgrade,
-            SnsVersion, SnsWasm, SnsWasmError, SnsWasmStableIndex, StableCanisterState,
-            UpdateSnsSubnetListRequest, UpdateSnsSubnetListResponse,
-        },
+    pb::v1::{
+        add_wasm_response, AddWasmRequest, AddWasmResponse, DappCanistersTransferResult,
+        DeployNewSnsRequest, DeployNewSnsResponse, DeployedSns, GetDeployedSnsByProposalIdRequest,
+        GetDeployedSnsByProposalIdResponse, GetNextSnsVersionRequest, GetNextSnsVersionResponse,
+        GetProposalIdThatAddedWasmRequest, GetProposalIdThatAddedWasmResponse,
+        GetSnsSubnetIdsResponse, GetWasmMetadataRequest as GetWasmMetadataRequestPb,
+        GetWasmMetadataResponse as GetWasmMetadataResponsePb, GetWasmRequest, GetWasmResponse,
+        InsertUpgradePathEntriesRequest, InsertUpgradePathEntriesResponse,
+        ListDeployedSnsesRequest, ListDeployedSnsesResponse, ListUpgradeStep,
+        ListUpgradeStepsRequest, ListUpgradeStepsResponse, MetadataSection as MetadataSectionPb,
+        SnsCanisterIds, SnsCanisterType, SnsUpgrade, SnsVersion, SnsWasm, SnsWasmError,
+        SnsWasmStableIndex, StableCanisterState, UpdateSnsSubnetListRequest,
+        UpdateSnsSubnetListResponse,
     },
     stable_memory::SnsWasmStableMemory,
     wasm_metadata::MetadataSection,
@@ -25,7 +21,7 @@ use candid::Encode;
 use ic_base_types::{CanisterId, PrincipalId};
 use ic_cdk::api::stable::StableMemory;
 use ic_nervous_system_clients::canister_id_record::CanisterIdRecord;
-use ic_nervous_system_common::{ONE_TRILLION, SNS_CREATION_FEE};
+use ic_nervous_system_common::{hash_to_hex_string, ONE_TRILLION, SNS_CREATION_FEE};
 use ic_nervous_system_proto::pb::v1::Canister;
 use ic_nns_constants::{
     DEFAULT_SNS_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT,

rs/sns/governance/src/governance.rs

@@ -389,6 +389,13 @@ impl GovernanceProto {
         index
     }
 
+    pub fn root_canister_id(&self) -> Result<CanisterId, GovernanceError> {
+        let root_canister_id = self.root_canister_id.ok_or_else(|| {
+            GovernanceError::new_with_message(ErrorType::Unavailable, "No root_canister_id.")
+        })?;
+        Ok(CanisterId::unchecked_from_principal(root_canister_id))
+    }
+
     pub fn root_canister_id_or_panic(&self) -> CanisterId {
         CanisterId::unchecked_from_principal(self.root_canister_id.expect("No root_canister_id."))
     }
@@ -2467,7 +2474,7 @@ impl Governance {
                 .map_err(|e| {
                     GovernanceError::new_with_message(
                         ErrorType::External,
-                        format!("Could not get list of SNS canisters from root: {}", e),
+                        format!("Could not get list of SNS canisters from SNS Root: {}", e),
                     )
                 })?;
 
@@ -2546,7 +2553,6 @@ impl Governance {
             })
     }
 
-
     /// Used for checking that no upgrades are in progress. Also checks that there are no upgrade proposals in progress except, optionally, one that you pass in as `proposal_id`
     pub fn check_no_upgrades_in_progress(
         &self,
@@ -2571,7 +2577,13 @@ impl Governance {
         if self.proto.pending_version.is_some() {
             return Err(GovernanceError::new_with_message(
                 ErrorType::ResourceExhausted,
-                "Upgrade lock currently acquired, not upgrading".to_string(),
+                format!(
+                    "Upgrade lock acquired (expires at {:?}), not upgrading",
+                    self.proto
+                        .pending_version
+                        .as_ref()
+                        .map(|p| p.mark_failed_at_seconds)
+                ),
             ));
         }
 
@@ -2665,7 +2677,7 @@ impl Governance {
         new_wasm_hash: Vec<u8>,
         canister_type_to_upgrade: SnsCanisterType,
     ) -> Result<(), GovernanceError> {
-        let root_canister_id = self.proto.root_canister_id_or_panic();
+        let root_canister_id = self.proto.root_canister_id()?;
 
         let target_wasm = get_wasm(&*self.env, new_wasm_hash.to_vec(), canister_type_to_upgrade)
             .await
@@ -2694,7 +2706,7 @@ impl Governance {
                     .map_err(|e| {
                         GovernanceError::new_with_message(
                             ErrorType::External,
-                            format!("Could not get list of SNS canisters from root: {}", e),
+                            format!("Could not get list of SNS canisters from SNS Root: {}", e),
                         )
                     })?;
             for target_canister_id in canister_ids_to_upgrade {
@@ -4755,7 +4767,7 @@ impl Governance {
                 self.refresh_cached_upgrade_steps().await;
             }
 
-            self.initiate_upgrade_if_behind_target_version().await;
+            self.initiate_upgrade_if_sns_behind_target_version().await;
 
             self.release_upgrade_periodic_task_lock();
         }
@@ -4812,14 +4824,16 @@ impl Governance {
     }
     /// Checks if an automatic upgrade is needed and initiates it.
     /// An automatic upgrade is needed if `target_version` is set to a future version on the upgrade path
-    async fn initiate_upgrade_if_behind_target_version(&mut self) {
+    async fn initiate_upgrade_if_sns_behind_target_version(&mut self) {
         // Check that no upgrades are in progress
         if self.check_no_upgrades_in_progress(None).is_err() {
             // An upgrade is already in progress
             return;
         }
 
         let Some(deployed_version) = self.proto.deployed_version.clone() else {
+            // TODO(NNS1-3445): there should be some way to recover from this state.
+            log!(ERROR, "No deployed version! This is an internal bug");
             return;
         };
 
@@ -4841,7 +4855,12 @@ impl Governance {
         // Find the current position of the deployed version
         let Some(deployed_position) = upgrade_steps.iter().position(|v| v == &deployed_version)
         else {
-            log!(ERROR, "Deployed version is not on the upgrade path");
+            log!(
+                ERROR,
+                "Deployed version {} is not on the upgrade path {:?}",
+                deployed_version,
+                upgrade_steps
+            );
             self.invalidate_cached_upgrade_steps();
             return;
         };
@@ -4871,13 +4890,10 @@ impl Governance {
                 }
             };
 
-        self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStarted {
-            current_version: self.proto.deployed_version.clone(),
-            expected_version: Some(next_version.clone()),
-            reason: Some(
-                upgrade_journal_entry::upgrade_started::Reason::BehindTargetVersion(Empty {}),
-            ),
-        });
+        self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStarted::behind_target(
+            deployed_version.clone(),
+            next_version.clone(),
+        ));
 
         self.proto.pending_version = Some(PendingVersion {
             target_version: Some(next_version.clone()),

rs/sns/governance/src/types.rs

@@ -16,7 +16,7 @@ use crate::{
             governance::{
                 self,
                 neuron_in_flight_command::{self, SyncCommand},
-                SnsMetadata,
+                SnsMetadata, Version,
             },
             governance_error::ErrorType,
             manage_neuron,
@@ -46,8 +46,8 @@ use ic_icrc1_ledger::UpgradeArgs as LedgerUpgradeArgs;
 use ic_ledger_core::tokens::TOKEN_SUBDIVIDABLE_BY;
 use ic_management_canister_types::CanisterInstallModeError;
 use ic_nervous_system_common::{
-    ledger_validation::MAX_LOGO_LENGTH, NervousSystemError, DEFAULT_TRANSFER_FEE, ONE_DAY_SECONDS,
-    ONE_MONTH_SECONDS, ONE_YEAR_SECONDS,
+    hash_to_hex_string, ledger_validation::MAX_LOGO_LENGTH, NervousSystemError,
+    DEFAULT_TRANSFER_FEE, ONE_DAY_SECONDS, ONE_MONTH_SECONDS, ONE_YEAR_SECONDS,
 };
 use ic_nervous_system_common_validation::validate_proposal_url;
 use ic_nervous_system_proto::pb::v1::{Duration as PbDuration, Percentage};
@@ -2531,6 +2531,21 @@ impl From<NeuronRecipes> for Vec<NeuronRecipe> {
     }
 }
 
+impl std::fmt::Display for Version {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "SnsVersion {{ root:{}, governance:{}, swap:{}, index:{}, ledger:{}, archive:{} }}",
+            hash_to_hex_string(&self.root_wasm_hash),
+            hash_to_hex_string(&self.governance_wasm_hash),
+            hash_to_hex_string(&self.swap_wasm_hash),
+            hash_to_hex_string(&self.index_wasm_hash),
+            hash_to_hex_string(&self.ledger_wasm_hash),
+            hash_to_hex_string(&self.archive_wasm_hash)
+        )
+    }
+}
+
 pub mod test_helpers {
     use super::*;
     use rand::Rng;