From 0edfa11fc8f723d927ad01580bd9e1a4239a3882 Mon Sep 17 00:00:00 2001 From: Andre Popovitch Date: Fri, 8 Nov 2024 19:28:53 -0500 Subject: [PATCH] automatically trigger upgrades --- .../tests/advance_target_version.rs | 96 ++++++- rs/sns/governance/canister/canister.rs | 2 +- rs/sns/governance/canister/governance.did | 12 +- .../governance/canister/governance_test.did | 12 +- .../ic_sns_governance/pb/v1/governance.proto | 3 +- .../src/gen/ic_sns_governance.pb.v1.rs | 6 +- rs/sns/governance/src/governance.rs | 243 +++++++++++++++--- .../fail_stuck_upgrade_in_progress_tests.rs | 6 +- rs/sns/governance/src/sns_upgrade.rs | 4 +- 9 files changed, 326 insertions(+), 58 deletions(-) diff --git a/rs/nervous_system/integration_tests/tests/advance_target_version.rs b/rs/nervous_system/integration_tests/tests/advance_target_version.rs index 4c2feb9b3a8..e7e084033c9 100644 --- a/rs/nervous_system/integration_tests/tests/advance_target_version.rs +++ b/rs/nervous_system/integration_tests/tests/advance_target_version.rs @@ -253,7 +253,7 @@ async fn test_get_upgrade_journal() { expected_upgrade_journal_entries.push(sns_pb::upgrade_journal_entry::Event::TargetVersionSet( sns_pb::upgrade_journal_entry::TargetVersionSet { old_target_version: None, - new_target_version: Some(new_sns_version_2), + new_target_version: Some(new_sns_version_2.clone()), }, )); @@ -263,4 +263,98 @@ async fn test_get_upgrade_journal() { &expected_upgrade_journal_entries, ) .await; + + // Check that the target version is set to the new version. + { + let sns_pb::GetUpgradeJournalResponse { target_version, .. } = + sns::governance::get_upgrade_journal(&pocket_ic, sns.governance.canister_id).await; + + assert_eq!(target_version, Some(new_sns_version_2.clone())); + } + + await_with_timeout( + &pocket_ic, + UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS, + |pocket_ic| async { + sns::governance::get_upgrade_journal(pocket_ic, sns.governance.canister_id) + .await + .deployed_version + }, + &Some(new_sns_version_2.clone()), + ) + .await + .unwrap(); + + // Check that the deployed version is now set to the new version. + { + let sns_pb::GetUpgradeJournalResponse { + deployed_version, .. + } = sns::governance::get_upgrade_journal(&pocket_ic, sns.governance.canister_id).await; + + assert_eq!(deployed_version, Some(new_sns_version_2.clone())); + } + + // Check that the upgrade journal contains the correct entries. + { + expected_upgrade_journal_entries.push( + sns_pb::upgrade_journal_entry::Event::UpgradeStarted( + sns_pb::upgrade_journal_entry::UpgradeStarted { + current_version: Some(initial_sns_version.clone()), + expected_version: Some(new_sns_version_1.clone()), + reason: Some( + sns_pb::upgrade_journal_entry::upgrade_started::Reason::BehindTargetVersion( + sns_pb::Empty {}, + ), + ), + }, + ), + ); + + expected_upgrade_journal_entries.push( + sns_pb::upgrade_journal_entry::Event::UpgradeOutcome( + sns_pb::upgrade_journal_entry::UpgradeOutcome { + status: Some( + sns_pb::upgrade_journal_entry::upgrade_outcome::Status::Success( + sns_pb::Empty {}, + ), + ), + human_readable: None, + }, + ), + ); + + expected_upgrade_journal_entries.push( + sns_pb::upgrade_journal_entry::Event::UpgradeStarted( + sns_pb::upgrade_journal_entry::UpgradeStarted { + current_version: Some(new_sns_version_1.clone()), + expected_version: Some(new_sns_version_2.clone()), + reason: Some( + sns_pb::upgrade_journal_entry::upgrade_started::Reason::BehindTargetVersion( + sns_pb::Empty {}, + ), + ), + }, + ), + ); + + expected_upgrade_journal_entries.push( + sns_pb::upgrade_journal_entry::Event::UpgradeOutcome( + sns_pb::upgrade_journal_entry::UpgradeOutcome { + status: Some( + sns_pb::upgrade_journal_entry::upgrade_outcome::Status::Success( + sns_pb::Empty {}, + ), + ), + human_readable: None, + }, + ), + ); + + assert_upgrade_journal( + &pocket_ic, + sns.governance, + &expected_upgrade_journal_entries, + ) + .await; + } } diff --git a/rs/sns/governance/canister/canister.rs b/rs/sns/governance/canister/canister.rs index 8358ef4bd9f..bd26026bdd9 100644 --- a/rs/sns/governance/canister/canister.rs +++ b/rs/sns/governance/canister/canister.rs @@ -473,7 +473,7 @@ fn get_running_sns_version(_: GetRunningSnsVersionRequest) -> GetRunningSnsVersi target_version: upgrade_in_progress.target_version.clone(), mark_failed_at_seconds: upgrade_in_progress.mark_failed_at_seconds, checking_upgrade_lock: upgrade_in_progress.checking_upgrade_lock, - proposal_id: upgrade_in_progress.proposal_id, + proposal_id: upgrade_in_progress.proposal_id.unwrap_or(0), }); GetRunningSnsVersionResponse { deployed_version: governance().proto.deployed_version.clone(), diff --git a/rs/sns/governance/canister/governance.did b/rs/sns/governance/canister/governance.did index 96c85afbf8e..4f9b9b11130 100644 --- a/rs/sns/governance/canister/governance.did +++ b/rs/sns/governance/canister/governance.did @@ -257,7 +257,12 @@ type GetProposalResponse = record { type GetRunningSnsVersionResponse = record { deployed_version : opt Version; - pending_version : opt UpgradeInProgress; + pending_version : opt record { + mark_failed_at_seconds : nat64; + checking_upgrade_lock : nat64; + proposal_id : nat64; + target_version : opt Version; + }; }; type GetSnsInitializationParametersResponse = record { @@ -674,14 +679,14 @@ type TransferSnsTreasuryFunds = record { type UpgradeInProgress = record { mark_failed_at_seconds : nat64; checking_upgrade_lock : nat64; - proposal_id : nat64; + proposal_id : opt nat64; target_version : opt Version; }; type PendingVersion = record { mark_failed_at_seconds : nat64; checking_upgrade_lock : nat64; - proposal_id : nat64; + proposal_id : opt nat64; target_version : opt Version; }; @@ -781,6 +786,7 @@ type GetUpgradeJournalResponse = record { upgrade_steps : opt Versions; response_timestamp_seconds : opt nat64; target_version : opt Version; + deployed_version : opt Version; upgrade_journal : opt UpgradeJournal; }; diff --git a/rs/sns/governance/canister/governance_test.did b/rs/sns/governance/canister/governance_test.did index 2d67529ed68..d29c14ddf9d 100644 --- a/rs/sns/governance/canister/governance_test.did +++ b/rs/sns/governance/canister/governance_test.did @@ -266,7 +266,12 @@ type GetProposalResponse = record { type GetRunningSnsVersionResponse = record { deployed_version : opt Version; - pending_version : opt UpgradeInProgress; + pending_version : opt record { + mark_failed_at_seconds : nat64; + checking_upgrade_lock : nat64; + proposal_id : nat64; + target_version : opt Version; + }; }; type GetSnsInitializationParametersResponse = record { @@ -688,14 +693,14 @@ type TransferSnsTreasuryFunds = record { type UpgradeInProgress = record { mark_failed_at_seconds : nat64; checking_upgrade_lock : nat64; - proposal_id : nat64; + proposal_id : opt nat64; target_version : opt Version; }; type PendingVersion = record { mark_failed_at_seconds : nat64; checking_upgrade_lock : nat64; - proposal_id : nat64; + proposal_id : opt nat64; target_version : opt Version; }; @@ -795,6 +800,7 @@ type GetUpgradeJournalResponse = record { upgrade_steps : opt Versions; response_timestamp_seconds : opt nat64; target_version : opt Version; + deployed_version : opt Version; upgrade_journal : opt UpgradeJournal; }; diff --git a/rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto b/rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto index 8b5711d5f09..3f6b74863ce 100644 --- a/rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto +++ b/rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto @@ -1424,7 +1424,7 @@ message Governance { // allowing us to fail in case we otherwise have gotten stuck. uint64 checking_upgrade_lock = 3; // The proposal that initiated this upgrade - uint64 proposal_id = 4; + optional uint64 proposal_id = 4; } // Version SNS is in process of upgrading to. @@ -2224,6 +2224,7 @@ message GetUpgradeJournalResponse { // Currently, this field is always None, but in the "effortless SNS upgrade" // feature, it reflect the version of the SNS that the community has decided to upgrade to. Governance.Version target_version = 3; + Governance.Version deployed_version = 5; UpgradeJournal upgrade_journal = 4; } diff --git a/rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs b/rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs index 0231dcdf5dc..a5250c1d3c4 100644 --- a/rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs +++ b/rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs @@ -1900,8 +1900,8 @@ pub mod governance { #[prost(uint64, tag = "3")] pub checking_upgrade_lock: u64, /// The proposal that initiated this upgrade - #[prost(uint64, tag = "4")] - pub proposal_id: u64, + #[prost(uint64, optional, tag = "4")] + pub proposal_id: ::core::option::Option, } #[derive( candid::CandidType, @@ -3583,6 +3583,8 @@ pub struct GetUpgradeJournalResponse { /// feature, it reflect the version of the SNS that the community has decided to upgrade to. #[prost(message, optional, tag = "3")] pub target_version: ::core::option::Option, + #[prost(message, optional, tag = "5")] + pub deployed_version: ::core::option::Option, #[prost(message, optional, tag = "4")] pub upgrade_journal: ::core::option::Option, } diff --git a/rs/sns/governance/src/governance.rs b/rs/sns/governance/src/governance.rs index 9d7f0ce4590..5c87ce806f5 100644 --- a/rs/sns/governance/src/governance.rs +++ b/rs/sns/governance/src/governance.rs @@ -65,8 +65,8 @@ use crate::{ MAX_NUMBER_OF_PROPOSALS_WITH_BALLOTS, }, sns_upgrade::{ - get_all_sns_canisters, get_running_version, get_upgrade_params, get_wasm, SnsCanisterType, - UpgradeSnsParams, + canister_type_and_wasm_hash_for_upgrade, get_all_sns_canisters, get_canisters_to_upgrade, + get_running_version, get_upgrade_params, get_wasm, SnsCanisterType, UpgradeSnsParams, }, types::{ function_id_to_proposal_criticality, is_registered_function_id, Environment, @@ -2063,8 +2063,9 @@ impl Governance { } Action::UpgradeSnsToNextVersion(_) => { log!(INFO, "Executing UpgradeSnsToNextVersion action",); - let upgrade_sns_result = - self.perform_upgrade_to_next_sns_version(proposal_id).await; + let upgrade_sns_result = self + .perform_upgrade_to_next_sns_version_legacy(proposal_id) + .await; // If the upgrade returned `Ok(true)` that means the upgrade completed successfully // and the proposal can be marked as "executed". If the upgrade returned `Ok(false)` @@ -2458,7 +2459,7 @@ impl Governance { proposal_id: u64, upgrade: UpgradeSnsControlledCanister, ) -> Result<(), GovernanceError> { - self.check_no_other_upgrades_in_progress(proposal_id)?; + self.check_no_upgrades_in_progress(Some(proposal_id))?; let sns_canisters = get_all_sns_canisters(&*self.env, self.proto.root_canister_id_or_panic()) @@ -2545,12 +2546,14 @@ impl Governance { }) } - pub fn check_no_other_upgrades_in_progress( + + /// 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, - proposal_id: u64, + proposal_id: Option, ) -> Result<(), GovernanceError> { let upgrade_proposals_in_progress = self.upgrade_proposals_in_progress(); - if upgrade_proposals_in_progress != BTreeSet::from([proposal_id]) { + if upgrade_proposals_in_progress != proposal_id.into_iter().collect() { return Err(GovernanceError::new_with_message( ErrorType::ResourceExhausted, format!( @@ -2577,11 +2580,11 @@ impl Governance { /// Return `Ok(true)` if the upgrade was completed successfully, return `Ok(false)` if an /// upgrade was successfully kicked-off, but its completion is pending. - async fn perform_upgrade_to_next_sns_version( + async fn perform_upgrade_to_next_sns_version_legacy( &mut self, proposal_id: u64, ) -> Result { - self.check_no_other_upgrades_in_progress(proposal_id)?; + self.check_no_upgrades_in_progress(Some(proposal_id))?; let current_version = self.proto.deployed_version_or_panic(); let root_canister_id = self.proto.root_canister_id_or_panic(); @@ -2649,7 +2652,7 @@ impl Governance { target_version: Some(next_version), mark_failed_at_seconds: self.env.now() + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }); log!( @@ -2661,6 +2664,63 @@ impl Governance { Ok(false) } + async fn upgrade_sns_framework_canister( + &mut self, + new_wasm_hash: Vec, + canister_type_to_upgrade: SnsCanisterType, + ) -> Result<(), GovernanceError> { + let root_canister_id = self.proto.root_canister_id_or_panic(); + + let target_wasm = get_wasm(&*self.env, new_wasm_hash.to_vec(), canister_type_to_upgrade) + .await + .map_err(|e| { + GovernanceError::new_with_message( + ErrorType::External, + format!("Could not get wasm for upgrade: {}", e), + ) + })? + .wasm; + + let target_is_root = canister_type_to_upgrade == SnsCanisterType::Root; + + if target_is_root { + upgrade_canister_directly( + &*self.env, + root_canister_id, + target_wasm, + Encode!().unwrap(), + ) + .await?; + } else { + let canister_ids_to_upgrade = + get_canisters_to_upgrade(&*self.env, root_canister_id, canister_type_to_upgrade) + .await + .map_err(|e| { + GovernanceError::new_with_message( + ErrorType::External, + format!("Could not get list of SNS canisters from root: {}", e), + ) + })?; + for target_canister_id in canister_ids_to_upgrade { + self.upgrade_non_root_canister( + target_canister_id, + target_wasm.clone(), + Encode!().unwrap(), + CanisterInstallMode::Upgrade, + ) + .await?; + } + } + + log!( + INFO, + "Successfully kicked off upgrade for SNS canister {:?}", + canister_type_to_upgrade, + ); + + Ok(()) + } + async fn perform_transfer_sns_treasury_funds( &mut self, proposal_id: u64, // This is just to control concurrency. @@ -2781,7 +2841,7 @@ impl Governance { proposal_id: u64, manage_ledger_parameters: ManageLedgerParameters, ) -> Result<(), GovernanceError> { - self.check_no_other_upgrades_in_progress(proposal_id)?; + self.check_no_upgrades_in_progress(Some(proposal_id))?; let current_version = self.proto.deployed_version_or_panic(); let ledger_canister_id = self.proto.ledger_canister_id_or_panic(); @@ -4699,6 +4759,8 @@ impl Governance { self.refresh_cached_upgrade_steps().await; } + self.initiate_upgrade_if_behind_target_version().await; + self.release_upgrade_periodic_task_lock(); } @@ -4752,6 +4814,97 @@ 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) { + // 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 { + return; + }; + + let Some(target_version) = self.proto.target_version.clone() else { + return; + }; + + let Some(CachedUpgradeSteps { + upgrade_steps: Some(Versions { + versions: upgrade_steps, + }), + .. + }) = &self.proto.cached_upgrade_steps + else { + log!(ERROR, "Cached upgrade steps set to None"); + return; + }; + + // 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"); + self.invalidate_cached_upgrade_steps(); + return; + }; + + // Find the target position of the target version + let Some(target_position) = upgrade_steps.iter().position(|v| v == &target_version) else { + log!(ERROR, "Target version is not on the upgrade path"); + return; + }; + + if target_position <= deployed_position { + // Target version is not after the deployed version + return; + } + + // since `target_position > deployed_position`, `deployed_position + 1 < upgrade_steps.len()` + let next_version = upgrade_steps[deployed_position + 1].clone(); + + let (canister_type, wasm_hash) = + match canister_type_and_wasm_hash_for_upgrade(&deployed_version, &next_version) { + Ok((canister_type, wasm_hash)) => (canister_type, wasm_hash), + + Err(err) => { + log!(ERROR, "Upgrade attempt failed: {}", err); + self.proto.target_version = None; + return; + } + }; + + 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.proto.pending_version = Some(PendingVersion { + target_version: Some(next_version.clone()), + mark_failed_at_seconds: self.env.now() + 5 * 60, + checking_upgrade_lock: 0, + proposal_id: None, + }); + + println!("Initiating upgrade to version: {:?}", next_version); + let upgrade_attempt = self + .upgrade_sns_framework_canister(wasm_hash, canister_type) + .await; + if let Err(err) = upgrade_attempt { + log!(ERROR, "Upgrade attempt failed: {}", err); + self.proto.pending_version = None; + self.proto.target_version = None; + } + } + + /// Invalidates the cached upgrade steps. + fn invalidate_cached_upgrade_steps(&mut self) { + self.proto.cached_upgrade_steps = None; + } fn release_upgrade_periodic_task_lock(&mut self) { self.upgrade_periodic_task_lock = None; @@ -4862,6 +5015,7 @@ impl Governance { upgrade_steps: cached_upgrade_steps.upgrade_steps, response_timestamp_seconds: cached_upgrade_steps.response_timestamp_seconds, target_version: self.proto.target_version.clone(), + deployed_version: self.proto.deployed_version.clone(), // TODO(NNS1-3416): Bound the size of the response. upgrade_journal: self.proto.upgrade_journal.clone(), }, @@ -4869,6 +5023,7 @@ impl Governance { upgrade_steps: None, response_timestamp_seconds: None, target_version: None, + deployed_version: self.proto.deployed_version.clone(), // TODO(NNS1-3416): Bound the size of the response. upgrade_journal: self.proto.upgrade_journal.clone(), }, @@ -5505,7 +5660,9 @@ impl Governance { )), human_readable: None, }); - self.set_proposal_execution_status(proposal_id, Ok(())); + if let Some(proposal_id) = proposal_id { + self.set_proposal_execution_status(proposal_id, Ok(())); + } self.proto.deployed_version = Some(target_version); self.proto.pending_version = None; } @@ -5539,12 +5696,14 @@ impl Governance { // an error for an UpgradeSnsToNextVersion actions failure. This unblocks further upgrade proposals. fn fail_sns_upgrade_to_next_version_proposal( &mut self, - proposal_id: u64, + proposal_id: Option, error: GovernanceError, ) { log!(ERROR, "{}", error.error_message); let result = Err(error); - self.set_proposal_execution_status(proposal_id, result); + if let Some(proposal_id) = proposal_id { + self.set_proposal_execution_status(proposal_id, result); + } self.proto.pending_version = None; } @@ -5585,7 +5744,7 @@ impl Governance { self.fail_sns_upgrade_to_next_version_proposal( pending_version.proposal_id, GovernanceError::new_with_message(ErrorType::External, error), - ); + ) } FailStuckUpgradeInProgressResponse {} @@ -7325,7 +7484,7 @@ mod tests { target_version: Some(next_version.into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); // We do not check the upgrade completion in this test because of limitations @@ -7640,7 +7799,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds, checking_upgrade_lock: 0, - proposal_id: 0, + proposal_id: Some(0), }); // Make sure Governance state is correctly set @@ -7650,7 +7809,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds, checking_upgrade_lock: 0, - proposal_id: 0, + proposal_id: Some(0), } ); assert_eq!( @@ -7738,7 +7897,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now - 1, checking_upgrade_lock: 0, - proposal_id: 0, + proposal_id: Some(0), }), ..basic_governance_proto() } @@ -7756,7 +7915,7 @@ mod tests { target_version: Some(next_version.into()), mark_failed_at_seconds: now - 1, checking_upgrade_lock: 0, - proposal_id: 0, + proposal_id: Some(0), } ); assert_eq!( @@ -7833,7 +7992,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -7879,7 +8038,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); assert_eq!( @@ -7974,7 +8133,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -8020,7 +8179,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); assert_eq!( @@ -8037,7 +8196,7 @@ mod tests { target_version: Some(next_version.into()), mark_failed_at_seconds: now + 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); @@ -8110,7 +8269,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now - 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -8156,7 +8315,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now - 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); assert_eq!( @@ -8258,7 +8417,7 @@ mod tests { target_version: None, mark_failed_at_seconds: now - 1, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -8393,7 +8552,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -8439,7 +8598,7 @@ mod tests { target_version: Some(next_version.into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); @@ -8600,7 +8759,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), ..basic_governance_proto() } @@ -8618,7 +8777,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); assert_eq!( @@ -8678,7 +8837,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), }), ..basic_governance_proto() } @@ -8696,7 +8855,7 @@ mod tests { target_version: Some(next_version.clone().into()), mark_failed_at_seconds: now + 5 * 60, checking_upgrade_lock: 0, - proposal_id, + proposal_id: Some(proposal_id), } ); assert_eq!( @@ -8892,7 +9051,7 @@ mod tests { ); // Step 2: Run code under test. - let result = governance.check_no_other_upgrades_in_progress(upgrade_proposal_id); + let result = governance.check_no_upgrades_in_progress(Some(upgrade_proposal_id)); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -8952,7 +9111,7 @@ mod tests { ); // Step 2: Run code under test. - let result = governance.check_no_other_upgrades_in_progress(executing_upgrade_proposal_id); + let result = governance.check_no_upgrades_in_progress(Some(executing_upgrade_proposal_id)); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -9013,7 +9172,7 @@ mod tests { ); // Step 2: Run code under test. - let result = governance.check_no_other_upgrades_in_progress(upgrade_proposal_id); + let result = governance.check_no_upgrades_in_progress(Some(upgrade_proposal_id)); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -9058,12 +9217,12 @@ mod tests { ); // Step 2 & 3: Run code under test, and inspect results. - let result = governance.check_no_other_upgrades_in_progress(proposal_id); + let result = governance.check_no_upgrades_in_progress(Some(proposal_id)); assert!(result.is_ok(), "{:#?}", result); // Other upgrades should be blocked by proposal 1 though. let some_other_proposal_id = 99_u64; - match governance.check_no_other_upgrades_in_progress(some_other_proposal_id) { + match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) { Ok(_) => panic!("Some other upgrade proposal was not blocked."), Err(err) => assert_eq!( err.error_type, @@ -9117,7 +9276,7 @@ mod tests { ); // Step 2 & 3: Run code under test, and inspect results. - match governance.check_no_other_upgrades_in_progress(proposal_id) { + match governance.check_no_upgrades_in_progress(Some(proposal_id)) { Ok(_) => panic!("Some other upgrade proposal was not blocked."), Err(err) => assert_eq!( err.error_type, @@ -9128,7 +9287,7 @@ mod tests { } let some_other_proposal_id = 99_u64; - match governance.check_no_other_upgrades_in_progress(some_other_proposal_id) { + match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) { Ok(_) => panic!("Some other upgrade proposal was not blocked."), Err(err) => assert_eq!( err.error_type, diff --git a/rs/sns/governance/src/governance/tests/fail_stuck_upgrade_in_progress_tests.rs b/rs/sns/governance/src/governance/tests/fail_stuck_upgrade_in_progress_tests.rs index 3ac095b0561..e4fc9344cbc 100644 --- a/rs/sns/governance/src/governance/tests/fail_stuck_upgrade_in_progress_tests.rs +++ b/rs/sns/governance/src/governance/tests/fail_stuck_upgrade_in_progress_tests.rs @@ -79,7 +79,7 @@ lazy_static! { target_version: Some(SNS_VERSION_2.clone()), mark_failed_at_seconds: UPGRADE_DEADLINE_TIMESTAMP_SECONDS, checking_upgrade_lock: 10, - proposal_id: UPGRADE_PROPOSAL_ID, + proposal_id: Some(UPGRADE_PROPOSAL_ID), }), // we make a proposal that is already decided so that it won't execute again because // proposals to upgrade SNS's cannot execute if there's no deployed_version set on Governance state @@ -173,7 +173,7 @@ fn test_does_nothing_if_upgrade_attempt_not_expired() { target_version: Some(SNS_VERSION_2.clone()), mark_failed_at_seconds: UPGRADE_DEADLINE_TIMESTAMP_SECONDS, checking_upgrade_lock: 10, - proposal_id: UPGRADE_PROPOSAL_ID, + proposal_id: Some(UPGRADE_PROPOSAL_ID), }; assert_eq!( governance.proto.pending_version.clone().unwrap(), @@ -252,7 +252,7 @@ fn test_fails_proposal_and_removes_upgrade_if_upgrade_attempt_is_expired() { target_version: Some(SNS_VERSION_2.clone()), mark_failed_at_seconds: UPGRADE_DEADLINE_TIMESTAMP_SECONDS, checking_upgrade_lock: 10, - proposal_id: UPGRADE_PROPOSAL_ID, + proposal_id: Some(UPGRADE_PROPOSAL_ID), } ); assert_eq!( diff --git a/rs/sns/governance/src/sns_upgrade.rs b/rs/sns/governance/src/sns_upgrade.rs index 82ea9695d7a..c778d3638a9 100644 --- a/rs/sns/governance/src/sns_upgrade.rs +++ b/rs/sns/governance/src/sns_upgrade.rs @@ -137,7 +137,7 @@ pub(crate) async fn get_proposal_id_that_added_wasm( Ok(proposal_id) } -async fn get_canisters_to_upgrade( +pub(crate) async fn get_canisters_to_upgrade( env: &dyn Environment, root_canister_id: CanisterId, canister_type: SnsCanisterType, @@ -174,7 +174,7 @@ async fn get_canisters_to_upgrade( .collect() } -fn canister_type_and_wasm_hash_for_upgrade( +pub(crate) fn canister_type_and_wasm_hash_for_upgrade( current_version: &Version, next_version: &Version, ) -> Result<(SnsCanisterType, Vec), String> {