feat(root): New method: create_canister_and_install_code

[daniel-wong-dfinity-org]

Only callable by governance.

As usual, this does the "real work" of a new upcoming proposal type that creates a new canister in a non-NNS subnet (and install code into the new canister).

Prior Work

This new proposal type overlaps a bit with the existing NnsCanisterInstall proposal type. This differs in a couple of key ways:

1. The created canister can live in other subnets.
2. The created canister is NOT designated as an NNS canister (in Registry).

Behavior Details

This uses bounded wait so that NNS (Root and Governance canisters) does not get stuck as a result of an unresponsive subnet (either because it is malicious, or otherwise messed up).

The canister is created using cycles from the Root canister's balance. This is like how when SNS-WASM creates canisters, cycles are taken from its own balance. Keeping Root topped up is beyond the scope of this feature.

Testing

No tests yet. Those will be in a future PR. This is ok, because only Governance is allowed to call this new method, and no code path in Governance calls it yet.

References

Next PR 👉

[jasonz-dfinity]

Is the use case to create canisters in application subnets? If not, then this is not necessary? I don't think it would hurt, since I don't expect anyone to try having root subsidize the creation fee by risking 25 ICP.

[daniel-wong-dfinity-org]

TBH, I'm not really sure which subnets people are hoping to target. Let me ask Björn about this...

In any case, AFAICT, it isn't "actually harmful" to give the canister some initial cycles when it doesn't need them. The only "harm" is that we have to keep Root topped up, but that can be automated very easily (with Cycle Ops), and the cost of the cycles themselves is negligible.

But if canister creation REQUIRES cycles, then, deleting this is harmful, because then, there is NO way to formulate a proposal (that does what the proposer wants). What's nice about this is that it maximizes the possibilities at a "small" cost.

Soap Box/Tangent

In general, I think when we add a feature, we should not limit it to only KNOWN use cases. So many times, people spec-ing out a new feature here have said,

Why would a user ever want to do THAT? Since we cannot imagine it, we conclude that there is no legitimate use for that, so let's explicitly block it.

but then a couple of months or so later, a user asks,

Hey, why are you blocking me from doing this?? WTF??

If a feature can easily be generalized, we should make it as capable as possible. It's not our job to prematurely limit what users do. We should only block an action when it is actually harmful (e.g. due to being very subtle).

And ofc, we shouldn't spend extra time (scope creep) on extending capabilities if there is no good reason to believe that they'll actually be used.

Back to Planet Earth: Add initial_cycles Field

I think what we can maybe do here is make cycles a parameter of the proposal. WDYT?

I would kind of like to make cycles field required, but ofc, UIs can always fill in that field without the user EXPLICITLY telling the UI what value to use. E.g.

type CreateCansterAndInstallCodeRequest = record {
  ...

  initial_cycles: opt nat64;
};

With this, we could reject initial_cycles: null. But then, UIs (like ic-admin) can always just do

let initial_cycles = flags
    .initial_cycles
    .unwrap_or_default(); // NOOOOO!!!
create_canister_and_install_code.initial_cycles = Some(initial_cycles);

to make things more convenient for the user. If a UI does something like that ☝️, my hope that the actual human operator explicitly indicated their desired value (for initial_cycles) is defeated.

[daniel-wong-dfinity-org]

I spoke to Rüdiger, and we kind of agree that we need not support application subnets (the only ones that have a positive canister creation fee?), because people can already do that (i.e. create canister, install code, and make Root the controller) without a proposal. So, that suggests we can delete this line without risking someone in the future being like, "But, I WANT to target that subnet!", because even if they tell us that, we can just say, "You already have the necessary primitives to achieve what you want".

So, the benefit of deleting this line is that it makes things simpler without actually cutting people off from achieving their desired outcomes; it just requires them to go about it in a different (but still acceptable?) way.

[jasonz-dfinity]

NCR. I would go for just "create_canister" and let other proposals take care of installing code and updating settings.

My reasoning:
(1) no partial failure - if the execution fails, we need to determine whether the canister was created in order to decide what to do.
(2) for the last 2 canisters in the NNS subnet, we went for "dummy wasm" initially: (canister skeleton for 135859, and empty wasm for 138487

Neither is a strong reason, so NCR.

[daniel-wong-dfinity-org]

Yeah, I like the simplicity of this idea. It definitely crossed my mind as I was working on this.

TBH, I can't think of a reason that install_canister MUST be included. It just seems like it would be pretty convenient.

Thanks for pointing out those two proposals. They definitely give some indication as to how often install_code would be desired IN ACTUAL PRACTICE.

Personally, it has always boggled my mind that create_canister and install_code are separate. (Meanwhile, upgrade and re-install are just "modes" of install_code, even though upgrade and re-install differ WILDLY in terms of their inherent destructiveness 🤷) I guess what I don't really understand is, "Why would anyone ever want a canister with no code? Isn't the whole point of a canister to DO shit?? With no code, a canister is not doing any actual work. All it is doing is existing...".

[daniel-wong-dfinity-org]

I spoke to Rüdiger, and he said that in his use case, he would want to install code right away.

I guess the only reason you would want to create without a desire to immediately install is if you want everyone to know your canister ID ASAP, before you have any code to deploy.

So, again, the question is, which is more common? Well, Björn thinks we should keep installing code, so I'm going to let that be the "tie breaker".

TANGENT: I think if we could refer to canisters by name, then, the canister need not be created right away; people could just program against the planned name instead of the pre-reserved ID, which cannot be "reliably" predicted. I really don't know why we do not have a canister naming system yet. Don't people remember how shitty it was to refer to hosts by their IP address? Isn't it obvious that the invention of /etc/hosts was inevitable? Does nobody remember why DNS was a very good thing for the Internet?

[github-actions]

This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):

1. Update unreleased_changelog.md (if there are behavior changes, even if they are
   non-breaking).

2. Are there BREAKING changes?

3. Is a data migration needed?

4. Security review?

How to Satisfy This Automatic Review

1. Go to the bottom of the pull request page.

2. Look for where it says this bot is requesting changes.

3. Click the three dots to the right.

4. Select "Dismiss review".

5. In the text entry box, respond to each of the numbered items in the previous
   section, declare one of the following:

• Done.

• $REASON_WHY_NO_NEED. E.g. for unreleased_changelog.md, "No
  canister behavior changes.", or for item 2, "Existing APIs
  behave as before.".

Brief Guide to "Externally Visible" Changes

"Externally visible behavior change" is very often due to some NEW canister API.

Changes to EXISTING APIs are more likely to be "breaking".

If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.

If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.

Reference(s)

For a more comprehensive checklist, see here.

GOVERNANCE_CHECKLIST_REMINDER_DEDUP