diff --git a/rs/sns/governance/src/cached_upgrade_steps.rs b/rs/sns/governance/src/cached_upgrade_steps.rs index 2c706789393..39c59632dcf 100644 --- a/rs/sns/governance/src/cached_upgrade_steps.rs +++ b/rs/sns/governance/src/cached_upgrade_steps.rs @@ -336,7 +336,22 @@ impl Governance { .await; let upgrade_steps = match upgrade_steps { - Ok(upgrade_steps) => upgrade_steps, + Ok(upgrade_steps) => { + // Check for duplicate versions in the response + let mut seen = std::collections::HashSet::new(); + for version in &upgrade_steps { + if !seen.insert(version) { + log!( + ERROR, + "Cannot refresh cached_upgrade_steps: SNS-W response contains duplicate versions" + ); + return; + } + } + Versions { + versions: upgrade_steps, + } + } Err(err) => { log!( ERROR, @@ -346,9 +361,6 @@ impl Governance { return; } }; - let upgrade_steps = Versions { - versions: upgrade_steps, - }; // Ensure `cached_upgrade_steps` is initialized let cached_upgrade_steps = self diff --git a/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs b/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs index dc4f90c1513..270e9a5814d 100644 --- a/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs +++ b/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs @@ -659,6 +659,72 @@ fn test_upgrade_periodic_task_lock_times_out() { assert!(!gov.acquire_upgrade_periodic_task_lock()); } +#[tokio::test] +async fn test_refresh_cached_upgrade_steps_rejects_duplicate_versions() { + // Step 1: Prepare the world. + let root_canister_id = *TEST_ROOT_CANISTER_ID; + let governance_canister_id = *TEST_GOVERNANCE_CANISTER_ID; + + let mut env = NativeEnvironment::new(Some(governance_canister_id)); + + let version = SnsVersion { + root_wasm_hash: vec![1, 2, 3], + governance_wasm_hash: vec![2, 3, 4], + ledger_wasm_hash: vec![3, 4, 5], + swap_wasm_hash: vec![4, 5, 6], + archive_wasm_hash: vec![5, 6, 7], + index_wasm_hash: vec![6, 7, 8], + }; + + // Set up environment to return upgrade steps with a duplicate version + env.set_call_canister_response( + SNS_WASM_CANISTER_ID, + "list_upgrade_steps", + Encode!(&ListUpgradeStepsRequest { + starting_at: Some(version.clone()), + sns_governance_canister_id: Some(governance_canister_id.into()), + limit: 0, + }) + .unwrap(), + Ok(Encode!(&ListUpgradeStepsResponse { + steps: vec![ + ListUpgradeStep { + version: Some(version.clone()) + }, + ListUpgradeStep { + version: Some(version.clone()) // Duplicate version + }, + ] + }) + .unwrap()), + ); + + let mut governance = Governance::new( + GovernanceProto { + root_canister_id: Some(root_canister_id.get()), + deployed_version: Some(Version::from(version.clone())), + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::new(env), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + + // Step 2: Run code under test. + assert_eq!(governance.proto.cached_upgrade_steps, None); + governance.temporarily_lock_refresh_cached_upgrade_steps(); + governance.refresh_cached_upgrade_steps().await; + + // Step 3: Verify that the cached_upgrade_steps was not updated due to duplicate versions + assert_eq!( + governance.proto.cached_upgrade_steps.unwrap().upgrade_steps, + None + ); +} + #[test] fn test_upgrade_periodic_task_lock_doesnt_get_stuck_during_overflow() { let env = NativeEnvironment::new(Some(*TEST_GOVERNANCE_CANISTER_ID)); diff --git a/rs/sns/governance/tests/governance.rs b/rs/sns/governance/tests/governance.rs index 49277c7982a..dda1fc021c7 100644 --- a/rs/sns/governance/tests/governance.rs +++ b/rs/sns/governance/tests/governance.rs @@ -2909,7 +2909,17 @@ async fn test_refresh_cached_upgrade_steps_noop_if_deployed_version_none() { async fn test_refresh_cached_upgrade_steps() { let mut canister_fixture = GovernanceCanisterFixtureBuilder::new().create(); - let expected_upgrade_steps = vec![Version::default(), Version::default(), Version::default()]; + let v1 = Version::default(); + let v2 = Version { + governance_wasm_hash: vec![1], + ..v1.clone() + }; + let v3 = Version { + governance_wasm_hash: vec![1, 2], + ..v2.clone() + }; + + let expected_upgrade_steps = vec![v1.clone(), v2.clone(), v3.clone()]; // Set up the fixture state { @@ -2927,7 +2937,7 @@ async fn test_refresh_cached_upgrade_steps() { canister_fixture .environment_fixture .push_mocked_canister_reply(ListUpgradeStepsResponse { steps }); - canister_fixture.governance.proto.deployed_version = Some(Version::default()); + canister_fixture.governance.proto.deployed_version = Some(v1); } // Check that the initial state is None