Skip to content

Commit efc765d

Browse files
anchpopanchpop
authored
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.
1 parent fd070aa commit efc765d

File tree

6 files changed

+101
-25
lines changed

6 files changed

+101
-25
lines changed

‎rs/sns/governance/src/governance.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,10 @@ pub const UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS: u64 = 60 * 60; // 1 ho
166166
/// Past this duration, the lock will be automatically released.
167167
const UPGRADE_PERIODIC_TASK_LOCK_TIMEOUT_SECONDS: u64 = 600;
168168

169+
/// Adopted-but-not-yet-executed upgrade proposals block other upgrade proposals from executing.
170+
/// But this is only true for proposals that are less than 1 day old, to prevent a stuck proposal from blocking all upgrades forever.
171+
const UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS: u64 = 60 * 60 * 24; // 1 day
172+
169173
/// Converts bytes to a subaccountpub fn bytes_to_subaccount(bytes: &[u8]) -> Result<icrc_ledger_types::icrc1::account::Subaccount, GovernanceError> {
170174
pub fn bytes_to_subaccount(
171175
bytes: &[u8],
@@ -2455,8 +2459,14 @@ impl Governance {
24552459
.proposals
24562460
.iter()
24572461
.filter_map(|(id, proposal_data)| {
2462+
let proposal_expiry_time = proposal_data
2463+
.decided_timestamp_seconds
2464+
.checked_add(UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS)
2465+
.unwrap_or_default();
2466+
let proposal_recent_enough = proposal_expiry_time > self.env.now();
24582467
if proposal_data.status() == ProposalDecisionStatus::Adopted
24592468
&& proposal_data.is_upgrade_proposal()
2469+
&& proposal_recent_enough
24602470
{
24612471
Some(*id)
24622472
} else {
@@ -2567,7 +2577,7 @@ impl Governance {
25672577
proposal_id: Option<u64>,
25682578
) -> Result<(), GovernanceError> {
25692579
let upgrade_proposals_in_progress = self.upgrade_proposals_in_progress();
2570-
if upgrade_proposals_in_progress != proposal_id.into_iter().collect() {
2580+
if !upgrade_proposals_in_progress.is_subset(&proposal_id.into_iter().collect()) {
25712581
return Err(GovernanceError::new_with_message(
25722582
ErrorType::ResourceExhausted,
25732583
format!(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ async fn test_initiate_upgrade_blocked_by_upgrade_proposal() {
6161
let proposal = ProposalData {
6262
action: (&action).into(),
6363
id: Some(proposal_id.into()),
64-
decided_timestamp_seconds: 1,
64+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
6565
latest_tally: Some(Tally {
6666
yes: 1,
6767
no: 0,

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

Lines changed: 80 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,7 +1080,7 @@ fn test_disallow_concurrent_upgrade_execution(
10801080
let execution_in_progress_proposal = ProposalData {
10811081
action: proposal_in_progress_action_id,
10821082
id: Some(1_u64.into()),
1083-
decided_timestamp_seconds: 123,
1083+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
10841084
latest_tally: Some(Tally {
10851085
yes: 1,
10861086
no: 0,
@@ -3041,7 +3041,7 @@ fn test_allow_canister_upgrades_while_motion_proposal_execution_is_in_progress()
30413041
let motion_proposal = ProposalData {
30423042
action: motion_action_id,
30433043
id: Some(motion_proposal_id.into()),
3044-
decided_timestamp_seconds: 1,
3044+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
30453045
latest_tally: Some(Tally {
30463046
yes: 1,
30473047
no: 0,
@@ -3056,7 +3056,7 @@ fn test_allow_canister_upgrades_while_motion_proposal_execution_is_in_progress()
30563056
let upgrade_proposal = ProposalData {
30573057
action: upgrade_action_id,
30583058
id: Some(upgrade_proposal_id.into()),
3059-
decided_timestamp_seconds: 1,
3059+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
30603060
latest_tally: Some(Tally {
30613061
yes: 1,
30623062
no: 0,
@@ -3115,7 +3115,7 @@ fn test_allow_canister_upgrades_while_another_upgrade_proposal_is_open() {
31153115
let executing_upgrade_proposal = ProposalData {
31163116
action: upgrade_action_id,
31173117
id: Some(executing_upgrade_proposal_id.into()),
3118-
decided_timestamp_seconds: 1,
3118+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
31193119
latest_tally: Some(Tally {
31203120
yes: 1,
31213121
no: 0,
@@ -3161,8 +3161,8 @@ fn test_allow_canister_upgrades_after_another_upgrade_proposal_has_executed() {
31613161
let previous_upgrade_proposal = ProposalData {
31623162
action: upgrade_action_id,
31633163
id: Some(previous_upgrade_proposal_id.into()),
3164-
decided_timestamp_seconds: 1,
3165-
executed_timestamp_seconds: 1,
3164+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
3165+
executed_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 5,
31663166
latest_tally: Some(Tally {
31673167
yes: 1,
31683168
no: 0,
@@ -3177,7 +3177,7 @@ fn test_allow_canister_upgrades_after_another_upgrade_proposal_has_executed() {
31773177
let upgrade_proposal = ProposalData {
31783178
action: upgrade_action_id,
31793179
id: Some(upgrade_proposal_id.into()),
3180-
decided_timestamp_seconds: 1,
3180+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
31813181
latest_tally: Some(Tally {
31823182
yes: 1,
31833183
no: 0,
@@ -3222,7 +3222,7 @@ fn test_allow_canister_upgrades_proposal_does_not_block_itself_but_does_block_ot
32223222
let proposal = ProposalData {
32233223
action: upgrade_action_id,
32243224
id: Some(proposal_id.into()),
3225-
decided_timestamp_seconds: 1,
3225+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
32263226
latest_tally: Some(Tally {
32273227
yes: 1,
32283228
no: 0,
@@ -3277,7 +3277,7 @@ fn test_upgrade_proposals_blocked_by_pending_upgrade() {
32773277
let proposal = ProposalData {
32783278
action: upgrade_action_id,
32793279
id: Some(proposal_id.into()),
3280-
decided_timestamp_seconds: 1,
3280+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS - 10,
32813281
latest_tally: Some(Tally {
32823282
yes: 1,
32833283
no: 0,
@@ -3330,6 +3330,76 @@ fn test_upgrade_proposals_blocked_by_pending_upgrade() {
33303330
}
33313331
}
33323332

3333+
/// 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
3334+
#[test]
3335+
fn test_upgrade_proposals_not_blocked_by_old_upgrade_proposals() {
3336+
// Step 1: Prepare the world.
3337+
use ProposalDecisionStatus as Status;
3338+
3339+
let upgrade_action_id: u64 =
3340+
(&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into();
3341+
3342+
let proposal_id = 1_u64;
3343+
let some_other_proposal_id = 99_u64;
3344+
let proposal = ProposalData {
3345+
action: upgrade_action_id,
3346+
id: Some(proposal_id.into()),
3347+
decided_timestamp_seconds: NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS
3348+
- crate::governance::UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS
3349+
- 1,
3350+
latest_tally: Some(Tally {
3351+
yes: 1,
3352+
no: 0,
3353+
total: 1,
3354+
timestamp_seconds: 1,
3355+
}),
3356+
..Default::default()
3357+
};
3358+
assert_eq!(proposal.status(), Status::Adopted);
3359+
3360+
let mut governance = Governance::new(
3361+
GovernanceProto {
3362+
proposals: btreemap! {
3363+
proposal_id => proposal,
3364+
},
3365+
..basic_governance_proto()
3366+
}
3367+
.try_into()
3368+
.unwrap(),
3369+
Box::<NativeEnvironment>::default(),
3370+
Box::new(DoNothingLedger {}),
3371+
Box::new(DoNothingLedger {}),
3372+
Box::new(FakeCmc::new()),
3373+
);
3374+
3375+
// Step 2: Check that the proposal is not blocked by an old proposal.
3376+
match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) {
3377+
Ok(_) => {},
3378+
Err(err) => panic!("The proposal should not have gotten blocked by an old proposal. Instead, it was blocked due to: {:#?}", err),
3379+
}
3380+
3381+
// Step 3: Make the proposal newer
3382+
governance
3383+
.proto
3384+
.proposals
3385+
.get_mut(&proposal_id)
3386+
.unwrap()
3387+
.decided_timestamp_seconds = NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS
3388+
- crate::governance::UPGRADE_PROPOSAL_BLOCK_EXPIRY_SECONDS
3389+
+ 1;
3390+
3391+
// Step 4: Check that the proposal is now blocked by an old proposal.
3392+
match governance.check_no_upgrades_in_progress(Some(some_other_proposal_id)) {
3393+
Ok(_) => panic!("The proposal should have gotten blocked by an old proposal"),
3394+
Err(err) => assert_eq!(
3395+
err.error_type,
3396+
ErrorType::ResourceExhausted as i32,
3397+
"{:#?}",
3398+
err,
3399+
),
3400+
}
3401+
}
3402+
33333403
#[test]
33343404
fn test_add_generic_nervous_system_function_succeeds() {
33353405
let root_canister_id = *TEST_ROOT_CANISTER_ID;
@@ -3626,10 +3696,7 @@ fn test_move_staked_maturity_on_dissolved_neurons_works() {
36263696
let neuron_id_2 = test_neuron_id(controller_2);
36273697
let regular_maturity: u64 = 1000000;
36283698
let staked_maturity: u64 = 424242;
3629-
let now = std::time::SystemTime::now()
3630-
.duration_since(std::time::UNIX_EPOCH)
3631-
.unwrap()
3632-
.as_secs();
3699+
let now = NativeEnvironment::DEFAULT_TEST_START_TIMESTAMP_SECONDS;
36333700
// Dissolved neuron.
36343701
let neuron_1 = Neuron {
36353702
id: Some(neuron_id_1.clone()),

‎rs/sns/governance/src/types.rs‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2677,25 +2677,21 @@ pub mod test_helpers {
26772677
default_canister_call_response: Ok(vec![]),
26782678
required_canister_call_invocations: Arc::new(RwLock::new(vec![])),
26792679
// This needs to be non-zero
2680-
now: std::time::SystemTime::now()
2681-
.duration_since(std::time::UNIX_EPOCH)
2682-
.unwrap()
2683-
.as_secs(),
2680+
now: Self::DEFAULT_TEST_START_TIMESTAMP_SECONDS,
26842681
}
26852682
}
26862683
}
26872684

26882685
impl NativeEnvironment {
2686+
pub const DEFAULT_TEST_START_TIMESTAMP_SECONDS: u64 = 999_111_000_u64;
2687+
26892688
pub fn new(local_canister_id: Option<CanisterId>) -> Self {
26902689
Self {
26912690
local_canister_id,
26922691
canister_calls_map: Default::default(),
26932692
default_canister_call_response: Ok(vec![]),
26942693
required_canister_call_invocations: Arc::new(RwLock::new(vec![])),
2695-
now: std::time::SystemTime::now()
2696-
.duration_since(std::time::UNIX_EPOCH)
2697-
.unwrap()
2698-
.as_secs(),
2694+
now: Self::DEFAULT_TEST_START_TIMESTAMP_SECONDS,
26992695
}
27002696
}
27012697

‎rs/sns/integration_tests/src/neuron.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,10 @@ fn test_neuron_action_is_not_authorized() {
724724
// This is a bit hacky and fragile, as it depends on how `SnsCanisters` is setup.
725725
// TODO(NNS1-1892): expose SnsCanisters' current time via API.
726726
fn get_sns_canisters_now_seconds() -> i64 {
727-
(NativeEnvironment::default().now() as i64)
727+
std::time::SystemTime::now()
728+
.duration_since(std::time::UNIX_EPOCH)
729+
.unwrap()
730+
.as_secs() as i64
728731
+ (NervousSystemParameters::with_default_values()
729732
.initial_voting_period_seconds
730733
.unwrap() as i64)

‎rs/sns/test_utils/src/itest_helpers.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1281,7 +1281,7 @@ impl SnsCanisters<'_> {
12811281
);
12821282
return;
12831283
}
1284-
tokio::time::sleep(Duration::from_millis(100)).await;
1284+
self.governance.runtime().tick().await;
12851285
}
12861286

12871287
panic!(

0 commit comments

Comments
 (0)