feat(sns): Enable upgrading SNS-controlled canisters using chunked WASMs #3287
Conversation
aterga
force-pushed
the
arshavir/upgrade-sns-controlled-canister-with-chunked-wasm
branch
from
534ea16 to
49f5888
Compare
aterga
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
|
TODO: Extend the rs/nervous_system/integration_tests/tests/upgrade_sns_controlled_canister_with_large_wasm.rs test to submit a proposal that would tirgger the upgrade. Done |
|
I think you need to modify the second example in your PR description? It lists chunks, but IIUC, the idea is that "embedded Wasm" means that there are no chunks. |
daniel-wong-dfinity-org
dismissed
github-actions[bot]’s stale review
changelog files have not been added for SNS yet, but that will happen very soon. If that goes in before this, then, yes, let's update those. Otherwise, this shouldn't be a blocker.
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
| @@ -212,7 +179,7 @@ async fn run_test(store_same_as_target: bool) { | |||
| app_subnet, | |||
| vec![], | |||
| None, | |||
| vec![sns.root.canister_id], | |||
| vec![sns.governance.canister_id, sns.root.canister_id], | |||
There was a problem hiding this comment.
Why does governance have to be a controller?
| @@ -64,7 +64,7 @@ pub struct ChangeCanisterRequest { | |||
| #[serde(with = "serde_bytes")] | |||
| pub wasm_module: Vec<u8>, | |||
|
|
|||
| /// If the entire WASM does not into the 2 MiB ingress limit, then `new_canister_wasm` | |||
| /// If the entire WASM does not fit into the 2 MiB ingress limit, then `new_canister_wasm` | |||
There was a problem hiding this comment.
I don't see a field called "new_canister_wasm" - should that say "wasm_module"?
|
|
||
| // Generate final report. | ||
| if !defects.is_empty() { | ||
| let tip = if upgrade.chunked_canister_wasm.is_some() { | ||
| format!( | ||
| "\nPlease make sure that both Governance ({}) and Root ({}) are controllers of \ |
There was a problem hiding this comment.
This comment made me think... if the canister is malicious, does root or governance need to make a request to that store canister? That seems like a risk we don't want.
| }; | ||
|
|
||
| let stored_chunks_response = env | ||
| .call_canister(CanisterId::ic_00(), "stored_chunks", arg) |
There was a problem hiding this comment.
I think we should cut this validation of stored_chunks out of the code.
The controller of that canister can change the chunks at any point. Maybe the proposals should simply have instructions to verify the overall wasm hash via the build instructions?
Can users submit text with the UpgradeSnsControlledCanister? If yes, then they should say how to verify it. The overall hash is all that needs to be verified by the voters. The chunks are either here or not, and if they're here during this part of the process that doesn't guarantee they're there later, since this canister can be controlled by whoever....
I guess this is helpful for the proposer, but it adds quite a bit of extra complexity to the code. I'm not sure it's really worth it.
This PR extends the existing proposal type
UpgradeSnsControllerCanisterwith a new fieldchunked_canister_wasmthat can be used for specifying an upgrade to a large WASM module (over 2 MiB) priorly uploaded to some store canister.Example proposal render for chunked Wasm:
Example proposal render for embedded Wasm: