Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sns): Automatically advance SNS target version upon opt-in #3119

Merged
merged 6 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ async fn test_get_upgrade_journal() {

expected_upgrade_journal_entries.push(Event::TargetVersionSet(TargetVersionSet::new(
None,
Some(new_sns_version_2.clone()),
new_sns_version_2.clone(),
false,
)));

sns::governance::assert_upgrade_journal(
Expand Down
4 changes: 4 additions & 0 deletions rs/sns/governance/api/src/ic_sns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1076,6 +1076,9 @@ pub struct NervousSystemParameters {
/// that the PB default (bool fields are false) and our application default
/// (enabled) agree.
pub maturity_modulation_disabled: Option<bool>,
/// Whether to automatically advance the SNS target version after a new upgrade is published
/// by the NNS. If not specified, defaults to false for backward compatibility.
pub automatically_advance_target_version: Option<bool>,
}
#[derive(Default, candid::CandidType, candid::Deserialize, Debug, Clone, Copy, PartialEq)]
pub struct VotingRewardsParameters {
Expand Down Expand Up @@ -2236,6 +2239,7 @@ pub mod upgrade_journal_entry {
pub struct TargetVersionSet {
pub old_target_version: Option<super::governance::Version>,
pub new_target_version: Option<super::governance::Version>,
pub is_advanced_automatically: Option<bool>,
}
#[derive(
candid::CandidType, candid::Deserialize, Debug, serde::Serialize, Clone, PartialEq,
Expand Down
2 changes: 2 additions & 0 deletions rs/sns/governance/canister/governance.did
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ type NervousSystemParameters = record {
voting_rewards_parameters : opt VotingRewardsParameters;
maturity_modulation_disabled : opt bool;
max_number_of_principals_per_neuron : opt nat64;
automatically_advance_target_version : opt bool;
};

type Neuron = record {
Expand Down Expand Up @@ -784,6 +785,7 @@ type UpgradeStepsReset = record {
type TargetVersionSet = record {
new_target_version : opt Version;
old_target_version : opt Version;
is_advanced_automatically : opt bool;
};

type TargetVersionReset = record {
Expand Down
2 changes: 2 additions & 0 deletions rs/sns/governance/canister/governance_test.did
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ type NervousSystemParameters = record {
voting_rewards_parameters : opt VotingRewardsParameters;
maturity_modulation_disabled : opt bool;
max_number_of_principals_per_neuron : opt nat64;
automatically_advance_target_version : opt bool;
};

type Neuron = record {
Expand Down Expand Up @@ -798,6 +799,7 @@ type UpgradeStepsReset = record {
type TargetVersionSet = record {
new_target_version : opt Version;
old_target_version : opt Version;
is_advanced_automatically : opt bool;
};

type TargetVersionReset = record {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,10 @@ message NervousSystemParameters {
// that the PB default (bool fields are false) and our application default
// (enabled) agree.
optional bool maturity_modulation_disabled = 22;

// Whether to automatically advance the SNS target version after a new upgrade is published
// by the NNS. If not specified, defaults to false for backward compatibility.
optional bool automatically_advance_target_version = 23;
}

message VotingRewardsParameters {
Expand Down Expand Up @@ -2244,6 +2248,7 @@ message UpgradeJournalEntry {
message TargetVersionSet {
optional Governance.Version old_target_version = 1;
optional Governance.Version new_target_version = 2;
optional bool is_advanced_automatically = 3;
}

message TargetVersionReset {
Expand Down
23 changes: 22 additions & 1 deletion rs/sns/governance/src/cached_upgrade_steps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,8 @@ impl Governance {
true
}

/// Refreshes the cached_upgrade_steps field
/// Attempts to refresh the cached_upgrade_steps field and (if this SNS wants automatic
/// deployment of upgrades), also the target_version.
pub async fn refresh_cached_upgrade_steps(&mut self, deployed_version: Version) {
let sns_governance_canister_id = self.env.canister_id().get();

Expand All @@ -495,6 +496,26 @@ impl Governance {
}
};

if self.should_automatically_advance_target_version()
&& upgrade_steps.has_pending_upgrades()
{
let new_target = upgrade_steps.last().clone();

{
let old_version = self.proto.target_version.clone();
let new_target = new_target.clone();
if old_version.as_ref() != Some(&new_target) {
self.push_to_upgrade_journal(upgrade_journal_entry::TargetVersionSet::new(
old_version,
new_target,
true,
));
}
}

self.proto.target_version.replace(new_target);
}

// This copy of the data would go to the upgrade journal for auditability.
let versions = upgrade_steps.clone().into_iter().collect();

Expand Down
6 changes: 6 additions & 0 deletions rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1473,6 +1473,10 @@ pub struct NervousSystemParameters {
/// (enabled) agree.
#[prost(bool, optional, tag = "22")]
pub maturity_modulation_disabled: ::core::option::Option<bool>,
/// Whether to automatically advance the SNS target version after a new upgrade is published
/// by the NNS. If not specified, defaults to false for backward compatibility.
#[prost(bool, optional, tag = "23")]
pub automatically_advance_target_version: ::core::option::Option<bool>,
}
#[derive(
candid::CandidType,
Expand Down Expand Up @@ -3547,6 +3551,8 @@ pub mod upgrade_journal_entry {
pub old_target_version: ::core::option::Option<super::governance::Version>,
#[prost(message, optional, tag = "2")]
pub new_target_version: ::core::option::Option<super::governance::Version>,
#[prost(bool, optional, tag = "3")]
pub is_advanced_automatically: ::core::option::Option<bool>,
}
#[derive(
candid::CandidType,
Expand Down
17 changes: 15 additions & 2 deletions rs/sns/governance/src/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3106,11 +3106,14 @@ impl Governance {
let (_, target_version) = self
.proto
.validate_new_target_version(Some(new_target))
.map_err(|err| GovernanceError::new_with_message(ErrorType::InvalidProposal, err))?;
.map_err(|err: String| {
GovernanceError::new_with_message(ErrorType::InvalidProposal, err)
})?;

self.push_to_upgrade_journal(upgrade_journal_entry::TargetVersionSet::new(
self.proto.target_version.clone(),
Some(target_version.clone()),
target_version.clone(),
false,
));

self.proto.target_version = Some(target_version);
Expand All @@ -3123,6 +3126,16 @@ impl Governance {
self.proto.parameters.as_ref()
}

pub fn should_automatically_advance_target_version(&self) -> bool {
self.nervous_system_parameters()
.map(|nervous_system_parameters| {
nervous_system_parameters
.automatically_advance_target_version
.unwrap_or_default()
})
.unwrap_or_default()
}

/// Returns the NervousSystemParameters or panics
fn nervous_system_parameters_or_panic(&self) -> &NervousSystemParameters {
self.nervous_system_parameters()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,84 @@ fn get_or_reset_upgrade_steps_leads_to_should_refresh_cached_upgrade_steps() {
assert!(gov.should_refresh_cached_upgrade_steps());
}

#[tokio::test]
async fn test_refresh_cached_upgrade_steps_advances_target_version_automatically_is_required() {
let version_a = Version {
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],
};
let mut version_b = version_a.clone();
version_b.root_wasm_hash = vec![9, 9, 9];

// Smoke test.
assert_ne!(version_a, version_b);

let make_gov = || -> Governance {
let mut env = NativeEnvironment::new(Some(*TEST_GOVERNANCE_CANISTER_ID));
add_environment_mock_list_upgrade_steps_call(
&mut env,
vec![
SnsVersion::from(version_a.clone()),
SnsVersion::from(version_b.clone()),
],
);
Governance::new(
GovernanceProto {
deployed_version: Some(version_a.clone()),
cached_upgrade_steps: None,
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::new(env),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
)
};

for (automatically_advance_target_version, expected_target_version) in [
(None, None),
(Some(false), None),
(Some(true), Some(version_b.clone())),
] {
let mut gov = make_gov();

if let Some(parameters) = gov.proto.parameters.as_mut() {
parameters.automatically_advance_target_version = automatically_advance_target_version;
};

// Precondition
assert_eq!(gov.proto.cached_upgrade_steps, None);
assert_eq!(gov.proto.target_version, None);

// Run code under test and assert intermediate conditions.
let deployed_version = gov
.try_temporarily_lock_refresh_cached_upgrade_steps()
.unwrap();
gov.refresh_cached_upgrade_steps(deployed_version).await;

// Intermediate postcondition.
assert_eq!(
gov.proto
.cached_upgrade_steps
.clone()
.unwrap()
.upgrade_steps
.unwrap()
.versions,
vec![version_a.clone(), version_b.clone(),]
);

// Main postcondition.
assert_eq!(gov.proto.target_version, expected_target_version);
}
}

fn add_environment_mock_calls_for_initiate_upgrade(
env: &mut NativeEnvironment,
expected_wasm_hash_requested: Vec<u8>,
Expand Down
4 changes: 4 additions & 0 deletions rs/sns/governance/src/pb/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,7 @@ impl From<pb::NervousSystemParameters> for pb_api::NervousSystemParameters {
max_dissolve_delay_bonus_percentage: item.max_dissolve_delay_bonus_percentage,
max_age_bonus_percentage: item.max_age_bonus_percentage,
maturity_modulation_disabled: item.maturity_modulation_disabled,
automatically_advance_target_version: item.automatically_advance_target_version,
}
}
}
Expand Down Expand Up @@ -1150,6 +1151,7 @@ impl From<pb_api::NervousSystemParameters> for pb::NervousSystemParameters {
max_dissolve_delay_bonus_percentage: item.max_dissolve_delay_bonus_percentage,
max_age_bonus_percentage: item.max_age_bonus_percentage,
maturity_modulation_disabled: item.maturity_modulation_disabled,
automatically_advance_target_version: item.automatically_advance_target_version,
}
}
}
Expand Down Expand Up @@ -3222,6 +3224,7 @@ impl From<pb::upgrade_journal_entry::TargetVersionSet>
Self {
old_target_version: item.old_target_version.map(|x| x.into()),
new_target_version: item.new_target_version.map(|x| x.into()),
is_advanced_automatically: item.is_advanced_automatically,
}
}
}
Expand All @@ -3232,6 +3235,7 @@ impl From<pb_api::upgrade_journal_entry::TargetVersionSet>
Self {
old_target_version: item.old_target_version.map(|x| x.into()),
new_target_version: item.new_target_version.map(|x| x.into()),
is_advanced_automatically: item.is_advanced_automatically,
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions rs/sns/governance/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ impl NervousSystemParameters {
max_dissolve_delay_bonus_percentage: Some(100),
max_age_bonus_percentage: Some(25),
maturity_modulation_disabled: Some(false),
automatically_advance_target_version: Some(false),
}
}

Expand Down Expand Up @@ -525,6 +526,9 @@ impl NervousSystemParameters {
maturity_modulation_disabled: self
.maturity_modulation_disabled
.or(base.maturity_modulation_disabled),
automatically_advance_target_version: self
.automatically_advance_target_version
.or(base.automatically_advance_target_version),
}
}

Expand Down
9 changes: 7 additions & 2 deletions rs/sns/governance/src/upgrade_journal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,15 @@ impl upgrade_journal_entry::UpgradeStepsReset {

impl upgrade_journal_entry::TargetVersionSet {
/// Creates a new TargetVersionSet event with old and new versions
pub fn new(old_version: Option<Version>, new_version: Option<Version>) -> Self {
pub fn new(
old_version: Option<Version>,
new_version: Version,
is_advanced_automatically: bool,
) -> Self {
Self {
old_target_version: old_version,
new_target_version: new_version,
new_target_version: Some(new_version),
is_advanced_automatically: Some(is_advanced_automatically),
}
}
}
Expand Down
35 changes: 31 additions & 4 deletions rs/sns/governance/unreleased_changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,40 @@ on the process that this file is part of, see

## Added

Enable upgrading SNS-controlled canisters using chunked WASMs. This is implemented as an extension
* Enable upgrading SNS-controlled canisters using chunked WASMs. This is implemented as an extension
of the existing `UpgradeSnsControllerCanister` proposal type with new field `chunked_canister_wasm`.
This field can be used for specifying an upgrade of an SNS-controlled *target* canister using
a potentially large WASM module (over 2 MiB) uploaded to some *store* canister, which:
* must be installed on the same subnet as target.
* must have SNS Root as one of its controllers.
* must have enough cycles for performing the upgrade.
* must be installed on the same subnet as target.
* must have SNS Root as one of its controllers.
* must have enough cycles for performing the upgrade.

* Enable SNSs to opt in for
[automatically advancing its target version](https://forum.dfinity.org/t/proposal-opt-in-mechanism-for-automatic-sns-target-version-advancement/39874)
to the newest version blessed by the NNS. To do so, please submit a `ManageNervousSystemParameters`
proposal, e.g.:

```bash
dfx canister --network ic call $SNS_GOVERNANCE_CANISTER_ID manage_neuron '(
record {
subaccount = blob "'${PROPOSER_SNS_NEURON_SUBACCOUNT}'";
command = opt variant {
MakeProposal = record {
url = "https://forum.dfinity.org/t/proposal-opt-in-mechanism-for-automatic-sns-target-version-advancement/39874";
title = "Opt for automatic advancement of SNS target versions";
action = opt variant {
ManageNervousSystemParameters = record {
automatically_advance_target_version = opt true;
}
};
summary = "Enable automatically advancing the target version \
of this SNS to speed up the delivery of SNS framework \
upgrades that were already blessed by the NNS.";
}
};
},
)'
```

## Changed

Expand Down
Loading