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 8714b6db051..83af00597f1 100644 --- a/rs/nervous_system/integration_tests/tests/advance_target_version.rs +++ b/rs/nervous_system/integration_tests/tests/advance_target_version.rs @@ -30,9 +30,18 @@ async fn assert_upgrade_journal( let upgrade_journal = upgrade_journal.unwrap().entries; assert_eq!(upgrade_journal.len(), expected_entries.len()); - for (actual, expected) in upgrade_journal.iter().zip(expected_entries.iter()) { + for (index, (actual, expected)) in upgrade_journal + .iter() + .zip(expected_entries.iter()) + .enumerate() + { assert!(actual.timestamp_seconds.is_some()); - assert_eq!(&actual.event, &Some(expected.clone())); + assert_eq!( + &actual.event, + &Some(expected.clone()), + "Upgrade journal entry at index {} does not match", + index + ); } } @@ -241,7 +250,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()), }, )); @@ -251,4 +260,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/proto/ic_sns_governance/pb/v1/governance.proto b/rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto index e784aec5db7..a39c1f0959f 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. @@ -2218,7 +2218,8 @@ 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 8636368e499..c5e652116d3 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 @@ -1898,8 +1898,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, @@ -3584,6 +3584,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 2baee7f2f6f..c635c48b5a0 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, @@ -2620,7 +2620,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!( @@ -2632,6 +2632,63 @@ impl Governance { Ok(false) } + async fn upgrade_test_hooray( + &mut self, + new_wasm_hash: Vec, + canister_type_to_upgrade: SnsCanisterType, + ) -> Result { + 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(false) + } + async fn perform_transfer_sns_treasury_funds( &mut self, proposal_id: u64, // This is just to control concurrency. @@ -4670,6 +4727,8 @@ impl Governance { self.refresh_cached_upgrade_steps().await; } + self.initiate_upgrade_if_needed().await; + self.release_upgrade_periodic_task_lock(); } @@ -4724,6 +4783,119 @@ impl Governance { } } + /// Checks if an automatic upgrade is needed and initiates it. + /// An automatic upgrade may be needed if `target_version` is set to a future version + async fn initiate_upgrade_if_needed(&mut self) { + if self.proto.pending_version.is_some() { + // An upgrade is already in progress + return; + } + + if let Some(deployed_version) = self.proto.deployed_version.clone() { + if let Some(target_version) = self.proto.target_version.clone() { + self.validate_and_prepare_upgrade(deployed_version, target_version) + .await; + } + } + } + + /// Validates that the target version is after the deployed version + /// and prepares the next upgrade step. + async fn validate_and_prepare_upgrade( + &mut self, + deployed_version: Version, + target_version: Version, + ) { + 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 { + // since `target_position > deployed_position`, `deployed_position + 1 < upgrade_steps.len()` + let next_version = upgrade_steps[deployed_position + 1].clone(); + + match canister_type_and_wasm_hash_for_upgrade(&deployed_version, &next_version) { + Ok((canister_type, wasm_hash)) => { + self.prepare_upgrade(next_version, canister_type, wasm_hash) + .await; + } + Err(err) => { + log!(ERROR, "Upgrade attempt failed: {}", err); + self.proto.target_version = None; + } + } + } + } + + async fn prepare_upgrade( + &mut self, + next_version: Version, + canister_type: SnsCanisterType, + wasm_hash: Vec, + ) { + 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 {}), + ), + }); + + // In the case of swap, upgrades are no-ops. + if canister_type == SnsCanisterType::Swap { + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { + status: Some(upgrade_journal_entry::upgrade_outcome::Status::Success( + Empty {}, + )), + human_readable: Some("Swap upgrades are no-ops".to_string()), + }); + + self.proto.deployed_version = Some(next_version); + return; + } + + self.proto.pending_version = Some(UpgradeInProgress { + 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_test_hooray(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; } @@ -4833,6 +5005,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(), }, @@ -4840,6 +5013,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(), }, @@ -5460,7 +5634,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; } @@ -5494,12 +5670,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; } @@ -5540,7 +5718,7 @@ impl Governance { self.fail_sns_upgrade_to_next_version_proposal( pending_version.proposal_id, GovernanceError::new_with_message(ErrorType::External, error), - ); + ) } FailStuckUpgradeInProgressResponse {} @@ -7308,7 +7486,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 @@ -7623,7 +7801,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 @@ -7633,7 +7811,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!( @@ -7721,7 +7899,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() } @@ -7739,7 +7917,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!( @@ -7816,7 +7994,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 @@ -7862,7 +8040,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!( @@ -7957,7 +8135,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 @@ -8003,7 +8181,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!( @@ -8020,7 +8198,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), } ); @@ -8093,7 +8271,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 @@ -8139,7 +8317,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!( @@ -8241,7 +8419,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 @@ -8376,7 +8554,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 @@ -8422,7 +8600,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), } ); @@ -8583,7 +8761,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() } @@ -8601,7 +8779,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!( @@ -8661,7 +8839,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() } @@ -8679,7 +8857,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!( 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 1371b525953..a48a9e1b270 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> {