fix(sns): Don't let very old upgrade proposals block future upgrades (#…

…3294)

For those who don't know, upgrade proposals (e.g.
UpgradeSnsToNextVersion) block all other upgrade actions while they're
adopted until they're done executing. If a proposal were to get stuck,
this would be a major problem, since the SNS would be un-upgradable.
This obviously is a pretty bad scenario, so this PR makes it so only
upgrade proposals decided within the last day can block other upgrades.

rs/sns/governance/src/governance.rs

@@ -166,6 +166,10 @@ pub const UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS: u64 = 60 * 60; // 1 ho
 /// Past this duration, the lock will be automatically released.
 const UPGRADE_PERIODIC_TASK_LOCK_TIMEOUT_SECONDS: u64 = 600;
 
+/// Adopted-but-not-yet-executed upgrade proposals block other upgrade proposals from executing.
+/// But this is only true for proposals that are less than 1 day old, to prevent a stuck proposal from blocking all upgrades forever.
+const UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS: u64 = 60 * 60 * 24; // 1 day
+
 /// Converts bytes to a subaccountpub fn bytes_to_subaccount(bytes: &[u8]) -> Result<icrc_ledger_types::icrc1::account::Subaccount, GovernanceError> {
 pub fn bytes_to_subaccount(
     bytes: &[u8],
@@ -2455,8 +2459,14 @@ impl Governance {
             .proposals
             .iter()
             .filter_map(|(id, proposal_data)| {
+                let proposal_expiry_time = proposal_data
+                    .decided_timestamp_seconds
+                    .checked_add(UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS)
+                    .unwrap_or_default();
+                let proposal_recent_enough = proposal_expiry_time > self.env.now();
                 if proposal_data.status() == ProposalDecisionStatus::Adopted
                     && proposal_data.is_upgrade_proposal()
+                    && proposal_recent_enough
                 {
                     Some(*id)
                 } else {
@@ -2567,7 +2577,7 @@ impl Governance {
         proposal_id: Option<u64>,
     ) -> Result<(), GovernanceError> {
         let upgrade_proposals_in_progress = self.upgrade_proposals_in_progress();
-        if upgrade_proposals_in_progress != proposal_id.into_iter().collect() {
+        if !upgrade_proposals_in_progress.is_subset(&proposal_id.into_iter().collect()) {
             return Err(GovernanceError::new_with_message(
                 ErrorType::ResourceExhausted,
                 format!(

rs/sns/governance/src/governance/advance_target_sns_version_tests.rs

@@ -61,7 +61,7 @@ async fn test_initiate_upgrade_blocked_by_upgrade_proposal() {
     let proposal = ProposalData {
         action: (&action).into(),
         id: Some(proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,

rs/sns/governance/src/governance/assorted_governance_tests.rs

@@ -1080,7 +1080,7 @@ fn test_disallow_concurrent_upgrade_execution(
     let execution_in_progress_proposal = ProposalData {
         action: proposal_in_progress_action_id,
         id: Some(1_u64.into()),
-        decided_timestamp_seconds: 123,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3041,7 +3041,7 @@ fn test_allow_canister_upgrades_while_motion_proposal_execution_is_in_progress()
     let motion_proposal = ProposalData {
         action: motion_action_id,
         id: Some(motion_proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3056,7 +3056,7 @@ fn test_allow_canister_upgrades_while_motion_proposal_execution_is_in_progress()
     let upgrade_proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(upgrade_proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3115,7 +3115,7 @@ fn test_allow_canister_upgrades_while_another_upgrade_proposal_is_open() {
     let executing_upgrade_proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(executing_upgrade_proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3161,8 +3161,8 @@ fn test_allow_canister_upgrades_after_another_upgrade_proposal_has_executed() {
     let previous_upgrade_proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(previous_upgrade_proposal_id.into()),
-        decided_timestamp_seconds: 1,
-        executed_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
+        executed_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 5,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3177,7 +3177,7 @@ fn test_allow_canister_upgrades_after_another_upgrade_proposal_has_executed() {
     let upgrade_proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(upgrade_proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3222,7 +3222,7 @@ fn test_allow_canister_upgrades_proposal_does_not_block_itself_but_does_block_ot
     let proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3277,7 +3277,7 @@ fn test_upgrade_proposals_blocked_by_pending_upgrade() {
     let proposal = ProposalData {
         action: upgrade_action_id,
         id: Some(proposal_id.into()),
-        decided_timestamp_seconds: 1,
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
         latest_tally: Some(Tally {
             yes: 1,
             no: 0,
@@ -3330,6 +3330,76 @@ fn test_upgrade_proposals_blocked_by_pending_upgrade() {
     }
 }
 
+/// Ugrade proposals (e.g. UpgradeSnsToNextVersion) block all other upgrade actions while they're adopted until they're done executing, unless they're too old. This test checks the "unless they're too old" part
+#[test]
+fn test_upgrade_proposals_not_blocked_by_old_upgrade_proposals() {
+    // Step 1: Prepare the world.
+    use ProposalDecisionStatus as Status;
+
+    let upgrade_action_id: u64 =
+        (&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into();
+
+    let proposal_id = 1_u64;
+    let some_other_proposal_id = 99_u64;
+    let proposal = ProposalData {
+        action: upgrade_action_id,
+        id: Some(proposal_id.into()),
+        decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS
+            - crate::governance::UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS
+            - 1,
+        latest_tally: Some(Tally {
+            yes: 1,
+            no: 0,
+            total: 1,
+            timestamp_seconds: 1,
+        }),
+        ..Default::default()
+    };
+    assert_eq!(proposal.status(), Status::Adopted);
+
+    let mut governance = Governance::new(
+        GovernanceProto {
+            proposals: btreemap! {
+                proposal_id => proposal,
+            },
+            ..basic_governance_proto()
+        }
+        .try_into()
+        .unwrap(),
+        Box::<NativeEnvironment>::default(),
+        Box::new(DoNothingLedger {}),
+        Box::new(DoNothingLedger {}),
+        Box::new(FakeCmc::new()),
+    );
+
+    // Step 2: Check that the proposal is not blocked by an old proposal.
+    match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) {
+        Ok(_) => {},
+        Err(err) => panic!("The proposal should not have gotten blocked by an old proposal. Instead, it was blocked due to: {:#?}", err),
+    }
+
+    // Step 3: Make the proposal newer
+    governance
+        .proto
+        .proposals
+        .get_mut(&proposal_id)
+        .unwrap()
+        .decided_timestamp_seconds = NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS
+        - crate::governance::UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS
+        + 1;
+
+    // Step 4: Check that the proposal is now blocked by an old proposal.
+    match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) {
+        Ok(_) => panic!("The proposal should have gotten blocked by an old proposal"),
+        Err(err) => assert_eq!(
+            err.error_type,
+            ErrorType::ResourceExhausted as i32,
+            "{:#?}",
+            err,
+        ),
+    }
+}
+
 #[test]
 fn test_add_generic_nervous_system_function_succeeds() {
     let root_canister_id = *TEST_ROOT_CANISTER_ID;
@@ -3626,10 +3696,7 @@ fn test_move_staked_maturity_on_dissolved_neurons_works() {
     let neuron_id_2 = test_neuron_id(controller_2);
     let regular_maturity: u64 = 1000000;
     let staked_maturity: u64 = 424242;
-    let now = std::time::SystemTime::now()
-        .duration_since(std::time::UNIX_EPOCH)
-        .unwrap()
-        .as_secs();
+    let now = NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS;
     // Dissolved neuron.
     let neuron_1 = Neuron {
         id: Some(neuron_id_1.clone()),

rs/sns/governance/src/types.rs

@@ -2677,25 +2677,21 @@ pub mod test_helpers {
                 default_canister_call_response: Ok(vec![]),
                 required_canister_call_invocations: Arc::new(RwLock::new(vec![])),
                 // This needs to be non-zero
-                now: std::time::SystemTime::now()
-                    .duration_since(std::time::UNIX_EPOCH)
-                    .unwrap()
-                    .as_secs(),
+                now: Self::DEFAULT_TEST_START_TIMESTAMP_SECONDS,
             }
         }
     }
 
     impl NativeEnvironment {
+        pub const DEFAULT_TEST_START_TIMESTAMP_SECONDS: u64 = 999_111_000_u64;
+
         pub fn new(local_canister_id: Option<CanisterId>) -> Self {
             Self {
                 local_canister_id,
                 canister_calls_map: Default::default(),
                 default_canister_call_response: Ok(vec![]),
                 required_canister_call_invocations: Arc::new(RwLock::new(vec![])),
-                now: std::time::SystemTime::now()
-                    .duration_since(std::time::UNIX_EPOCH)
-                    .unwrap()
-                    .as_secs(),
+                now: Self::DEFAULT_TEST_START_TIMESTAMP_SECONDS,
             }
         }
 

rs/sns/integration_tests/src/neuron.rs

@@ -724,7 +724,10 @@ fn test_neuron_action_is_not_authorized() {
 // This is a bit hacky and fragile, as it depends on how `SnsCanisters` is setup.
 // TODO(NNS1-1892): expose SnsCanisters' current time via API.
 fn get_sns_canisters_now_seconds() -> i64 {
-    (NativeEnvironment::default().now() as i64)
+    std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .unwrap()
+        .as_secs() as i64
         + (NervousSystemParameters::with_default_values()
             .initial_voting_period_seconds
             .unwrap() as i64)

rs/sns/test_utils/src/itest_helpers.rs

@@ -1281,7 +1281,7 @@ impl SnsCanisters<'_> {
                 );
                 return;
             }
-            tokio::time::sleep(Duration::from_millis(100)).await;
+            self.governance.runtime().tick().await;
         }
 
         panic!(