diff --git a/rs/sns/governance/src/governance.rs b/rs/sns/governance/src/governance.rs index 33775b55c6a..bf8b5e1bbdf 100644 --- a/rs/sns/governance/src/governance.rs +++ b/rs/sns/governance/src/governance.rs @@ -55,7 +55,7 @@ use crate::{ NeuronPermissionList, NeuronPermissionType, Proposal, ProposalData, ProposalDecisionStatus, ProposalId, ProposalRewardStatus, RegisterDappCanisters, RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeJournal, UpgradeJournalEntry, - UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote, WaitForQuietState, + UpgradeSnsControlledCanister, Vote, WaitForQuietState, }, }, proposal::{ @@ -2434,6 +2434,22 @@ impl Governance { } } + pub fn upgrade_proposals_in_progress(&self) -> BTreeSet { + self.proto + .proposals + .iter() + .filter_map(|(id, proposal_data)| { + if proposal_data.status() == ProposalDecisionStatus::Adopted + && proposal_data.is_upgrade_proposal() + { + Some(*id) + } else { + None + } + }) + .collect::>() + } + /// Executes a UpgradeSnsControlledCanister proposal by calling the root canister /// to upgrade an SNS controlled canister. This does not upgrade "core" SNS canisters /// (i.e. Root, Governance, Ledger, Ledger Archives, or Sale) @@ -2442,7 +2458,7 @@ impl Governance { proposal_id: u64, upgrade: UpgradeSnsControlledCanister, ) -> Result<(), GovernanceError> { - err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?; + self.check_no_other_upgrades_in_progress(proposal_id)?; let sns_canisters = get_all_sns_canisters(&*self.env, self.proto.root_canister_id_or_panic()) @@ -2529,13 +2545,45 @@ impl Governance { }) } + pub fn check_no_other_upgrades_in_progress( + &self, + proposal_id: u64, + ) -> Result<(), GovernanceError> { + let upgrade_proposals_in_progress = self.upgrade_proposals_in_progress(); + if !upgrade_proposals_in_progress.is_empty() + && upgrade_proposals_in_progress != BTreeSet::from([proposal_id]) + { + return Err(GovernanceError::new_with_message( + ErrorType::ResourceExhausted, + format!( + "Another upgrade is currently in progress (proposal IDs {}). \ + Please, try again later.", + upgrade_proposals_in_progress + .into_iter() + .map(|id| id.to_string()) + .collect::>() + .join(", ") + ), + )); + } + + if self.proto.pending_version.is_some() { + return Err(GovernanceError::new_with_message( + ErrorType::ResourceExhausted, + "Upgrade lock currently acquired, not upgrading".to_string(), + )); + } + + Ok(()) + } + /// 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( &mut self, proposal_id: u64, ) -> Result { - err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?; + self.check_no_other_upgrades_in_progress(proposal_id)?; let current_version = self.proto.deployed_version_or_panic(); let root_canister_id = self.proto.root_canister_id_or_panic(); @@ -2735,7 +2783,7 @@ impl Governance { proposal_id: u64, manage_ledger_parameters: ManageLedgerParameters, ) -> Result<(), GovernanceError> { - err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?; + self.check_no_other_upgrades_in_progress(proposal_id)?; let current_version = self.proto.deployed_version_or_panic(); let ledger_canister_id = self.proto.ledger_canister_id_or_panic(); @@ -5745,42 +5793,6 @@ thread_local! { static ATTEMPTED_FIXING_MEMORY_ALLOCATIONS: RefCell = const { RefCell::new(false) }; } -fn err_if_another_upgrade_is_in_progress( - id_to_proposal_data: &BTreeMap, - executing_proposal_id: u64, -) -> Result<(), GovernanceError> { - let upgrade_action_ids: [u64; 3] = [ - (&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into(), - (&Action::UpgradeSnsToNextVersion(UpgradeSnsToNextVersion::default())).into(), - (&Action::ManageLedgerParameters(ManageLedgerParameters::default())).into(), - ]; - - for (other_proposal_id, proposal_data) in id_to_proposal_data { - if *other_proposal_id == executing_proposal_id { - continue; - } - - if !upgrade_action_ids.contains(&proposal_data.action) { - continue; - } - - if proposal_data.status() != ProposalDecisionStatus::Adopted { - continue; - } - - return Err(GovernanceError::new_with_message( - ErrorType::ResourceExhausted, - format!( - "Another upgrade is currently in progress (proposal ID {}). \ - Please, try again later.", - other_proposal_id, - ), - )); - } - - Ok(()) -} - /// Affects the perception of time by users of CanisterEnv (i.e. Governance). /// /// Specifically, the time that Governance sees is the real time + delta. @@ -7070,6 +7082,9 @@ mod tests { Box::new(FakeCmc::new()), ); + let upgrade_proposals_in_progress = governance.upgrade_proposals_in_progress(); + assert_eq!(upgrade_proposals_in_progress, BTreeSet::from([1])); + // Step 2: Execute code under test. governance.process_proposal(2); @@ -7112,15 +7127,20 @@ mod tests { "The second upgrade proposal did not fail. final_proposal_data: {:#?}", final_proposal_data, ); - assert_eq!( + let final_failure_reason = ErrorType::try_from( final_proposal_data .failure_reason .as_ref() .unwrap() .error_type, - ErrorType::ResourceExhausted as i32, - "The second upgrade proposal failed, but failure_reason was not as expected. \ + ) + .unwrap(); + assert_eq!( + final_failure_reason, + ErrorType::ResourceExhausted, + "The second upgrade proposal failed, but failure_reason ({:?}) was not as expected. \ final_proposal_data: {:#?}", + final_failure_reason, final_proposal_data, ); } @@ -8841,14 +8861,24 @@ mod tests { }; assert_eq!(proposal.status(), Status::Adopted); + let governance = Governance::new( + GovernanceProto { + proposals: btreemap! { + proposal_id => proposal, + }, + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::::default(), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + // Step 2: Run code under test. let some_other_proposal_id = 99_u64; - let result = err_if_another_upgrade_is_in_progress( - &btreemap! { - proposal_id => proposal, - }, - some_other_proposal_id, - ); + let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -8876,14 +8906,24 @@ mod tests { }; assert_eq!(proposal.status(), Status::Open); + let governance = Governance::new( + GovernanceProto { + proposals: btreemap! { + proposal_id => proposal, + }, + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::::default(), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + // Step 2: Run code under test. let some_other_proposal_id = 99_u64; - let result = err_if_another_upgrade_is_in_progress( - &btreemap! { - proposal_id => proposal, - }, - some_other_proposal_id, - ); + let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -8913,14 +8953,24 @@ mod tests { }; assert_eq!(proposal.status(), Status::Executed); + let governance = Governance::new( + GovernanceProto { + proposals: btreemap! { + proposal_id => proposal, + }, + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::::default(), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + // Step 2: Run code under test. let some_other_proposal_id = 99_u64; - let result = err_if_another_upgrade_is_in_progress( - &btreemap! { - proposal_id => proposal, - }, - some_other_proposal_id, - ); + let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id); // Step 3: Inspect result. assert!(result.is_ok(), "{:#?}", result); @@ -8949,17 +8999,93 @@ mod tests { }; assert_eq!(proposal.status(), Status::Adopted); - let proposals = btreemap! { - proposal_id => proposal, - }; + let governance = Governance::new( + GovernanceProto { + proposals: btreemap! { + proposal_id => proposal, + }, + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::::default(), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); // Step 2 & 3: Run code under test, and inspect results. - let result = err_if_another_upgrade_is_in_progress(&proposals, proposal_id); + let result = governance.check_no_other_upgrades_in_progress(proposal_id); assert!(result.is_ok(), "{:#?}", result); // Other upgrades should be blocked by proposal 1 though. let some_other_proposal_id = 99_u64; - match err_if_another_upgrade_is_in_progress(&proposals, some_other_proposal_id) { + match governance.check_no_other_upgrades_in_progress(some_other_proposal_id) { + Ok(_) => panic!("Some other upgrade proposal was not blocked."), + Err(err) => assert_eq!( + err.error_type, + ErrorType::ResourceExhausted as i32, + "{:#?}", + err, + ), + } + } + + #[test] + fn test_upgrade_proposals_blocked_by_pending_upgrade() { + // 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 proposal = ProposalData { + action: upgrade_action_id, + id: Some(proposal_id.into()), + decided_timestamp_seconds: 1, + latest_tally: Some(Tally { + yes: 1, + no: 0, + total: 1, + timestamp_seconds: 1, + }), + ..Default::default() + }; + assert_eq!(proposal.status(), Status::Adopted); + + let governance = Governance::new( + GovernanceProto { + proposals: btreemap! { + proposal_id => proposal, + }, + // There's already an upgrade pending + pending_version: Some(UpgradeInProgress { + ..Default::default() + }), + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::::default(), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + + // Step 2 & 3: Run code under test, and inspect results. + match governance.check_no_other_upgrades_in_progress(proposal_id) { + Ok(_) => panic!("Some other upgrade proposal was not blocked."), + Err(err) => assert_eq!( + err.error_type, + ErrorType::ResourceExhausted as i32, + "{:#?}", + err, + ), + } + + let some_other_proposal_id = 99_u64; + match governance.check_no_other_upgrades_in_progress(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/proposal.rs b/rs/sns/governance/src/proposal.rs index 347f38c3d94..d66a803404a 100644 --- a/rs/sns/governance/src/proposal.rs +++ b/rs/sns/governance/src/proposal.rs @@ -2197,6 +2197,30 @@ impl ProposalData { ballots: limited_ballots, } } + + /// "Upgrade proposals" are those that upgrade the SNS or a canister it controls. + pub(crate) fn is_upgrade_proposal(&self) -> bool { + let action_is_upgrade = matches!( + self.proposal, + Some(Proposal { + action: Some( + Action::UpgradeSnsControlledCanister(_) + | Action::UpgradeSnsToNextVersion(_) + | Action::ManageLedgerParameters(_) + ), + .. + }) + ); + // In production, the above condition is exactly what we want. However, in some tests, we only set the action_id + // and not the action. + let upgrade_action_ids: [u64; 3] = [ + (&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into(), + (&Action::UpgradeSnsToNextVersion(UpgradeSnsToNextVersion::default())).into(), + (&Action::ManageLedgerParameters(ManageLedgerParameters::default())).into(), + ]; + let action_id_is_upgrade = upgrade_action_ids.contains(&self.action); + action_is_upgrade || action_id_is_upgrade + } } impl ProposalDecisionStatus {