NNS Updates 2024-10-04

[msumme]

The NNS Team submitted the following upgrade proposals today, 2024-10-04. DFINITY plans to vote on these proposals the following Monday.

Proposals to be Submitted

# Upgrade the Cycles-minting Canister to Commit ca88475

__Proposer__: maximilian.summe at dfinity.org\
__Source Code__: [ca8847547d327ce8a3bd81d25a590e01da1a3af5][new-commit]

[new-commit]: https://github.com/dfinity/ic/tree/ca8847547d327ce8a3bd81d25a590e01da1a3af5

[How to verify] this proposal (and others like it).

[How to verify]: https://github.com/dfinity/ic/tree/master/rs/nervous_system/docs/proposal_verification.md

## New Commits

```
$ git log --format="%C(auto) %h %s" 77f48ae63af09b6538b1bf33d3accc3bc74d14f8..ca8847547d327ce8a3bd81d25a590e01da1a3af5 --  ./rs/nns/cmc
 3bbabefb70 chore(Ledger-Suite): FI-1502 move icp and icrc ledger suites (#1682)
 ebb4dc57d9 feat(cmc): get default subnets query (#892)
 df1dbfc8a2 chore: Add file extension to globs (#1543)
 bbb8a51524 fix(cmc): Fix the data certification for the `get_average_icp_xdr_conversion_rate` endpoint (#1423)
 3aa43520c4 chore(cmc): Clean up the CMC state migration code (#1424)
 c2b4a0aaf4 fix(cmc): Merging CMC hotfix back to master (#1368)
 137faea9ee chore(cmc): Simplify get_next_multiply_of (#1236)
 4d09678d23 chore: sort rust derive traits (#1241)
```


## Upgrade Arguments

```candid
()
```


## Current Version

- Current Git Hash: 77f48ae63af09b6538b1bf33d3accc3bc74d14f8
- Current Wasm Hash: 3260e795bd3e446a189539ce89d44cb29f7d196b92cdd2e2c75571c062ef1e50

# Upgrade the Governance Canister to Commit ca88475

__Proposer__: maximilian.summe at dfinity.org\
__Source Code__: [ca8847547d327ce8a3bd81d25a590e01da1a3af5][new-commit]

[new-commit]: https://github.com/dfinity/ic/tree/ca8847547d327ce8a3bd81d25a590e01da1a3af5

[How to verify] this proposal (and others like it).

[How to verify]: https://github.com/dfinity/ic/tree/master/rs/nervous_system/docs/proposal_verification.md

## New Commits

```
$ git log --format="%C(auto) %h %s" 87343a880050ca72b1361138535211f5770dd52e..ca8847547d327ce8a3bd81d25a590e01da1a3af5 --  ./rs/nns/governance ./rs/sns/init
 3bbabefb70 chore(Ledger-Suite): FI-1502 move icp and icrc ledger suites (#1682)
 e2cb3d6389 chore: upgrade prost and tonic crates (#1738)
 e9647a7f3e feat(nns): More secure random numbers. (#1633)
 e7ad4e2a9c refactor(nns): Remove heap_neurons as it references the internal storage directly (#1728)
 f7791372e9 chore: remove old hyper and bump prost and tonic versions (#1597)
 09e7929d55 refactor(nervous-system): Remove ic-sns-governance and ic-sns-init's test_feature configurations (#1606)
 5610c60491 refactor(sns): Remove different sns init behavior when cfg(feature = "test") (#1609)
 f3d13ef565 feat(nns): Lower probability of panicking deprecated methods (#1659)
 285a5db07d refactor(nns): Migrate nns governance from dfn_core -> ic_cdk (#1565)
```


## Current Version

- Current Git Hash: 87343a880050ca72b1361138535211f5770dd52e
- Current Wasm Hash: 8665830c50c9a0dddd996008e537d060a380ac7b6c22237679bd0cecc4ee1044

Edit:
Proposal Links are included below:

https://dashboard.internetcomputer.org/proposal/133311
https://dashboard.internetcomputer.org/proposal/133312

[timk11]

I have voted to reject proposals 133311 and 133312.

Proposal 133311 contains the full set of commits from the previously rejected proposal 133088 plus an additional one. Proposal 133312 contains the new commit from 133311 in addition to a set of commits that has not previously been seen in the Protocol Canister Management topic. In my earlier review of proposal 133088 I noted that arg_hash didn’t match but figured this was a mistake in the writing of the proposal and didn’t consider it grounds for rejection.

In the two new proposals, however, the instructions for verifying the wasm module and payload arguments have not been included in the proposals themselves but have instead have been replaced by a generic link that is the same for each proposal. The verification instructions in the generic link are not written in a way that would be clear for new users of the platform.

@daniel-wong @msumme Please let’s not go down this path. Specific instructions for verifying the module (and arguments, if desired) can easily be modified from the examples in numerous previous proposals. The generic link does contain some helpful information and additional background but should not replace a more succinct and specific explanation for each individual proposal. Please be mindful of the distinction between arg_hex and arg_hash, which has been an issue in some past proposals. In particular, please assume whenever technical instructions are given that there could always be someone out there doing this for the first time.

Additionally, there was no link within the proposal itself to this forum topic. The proposal numbers were not mentioned in this thread and so I did not find it when I attempted to search for it. I only found it by accident in a Google search while I was looking for something else. There are posts in the forum here from @lara and here from @cryptoschindler about the new process that has been requested for posting about NNS proposals in the forum.

My comments are made with full respect and appreciation for the work that you are doing, but I do feel quite strongly about ensuring that things are made easier for new users rather than harder. I will still go through the new commits either in the shorter term or at such time as the proposals are resubmitted (if they are rejected), but I did want to get in and make this point early in the voting process.

[timk11]

@0rions @ZackDS @Zane @cyberowl @LaCosta @wpb

[cyberowl]

I agree with @timk11 that the generic instructions provided are not sufficiently clear, particularly for new users of the platform. While those familiar with the process may find them acceptable, new users could struggle to get started.

Additionally, I believe there is a critical need for explicit argument verification, given the past issues we’ve encountered in this area. The arg_hash has caused problems before. While proposal 133311 does provide a clear hash (0fee102bd16b053022b69f2c65fd5e2f41d150ce9c214ac8731cfaf496ebda4e), proposal 133312 does not.

For these reasons, I will be voting to REJECT both proposals.

[ZackDS]

Proposal 133311 CMC

Hash and argument match so do the listed commits. Obviously they are the same form the last proposal that got rejected, so they were reviewed already, with the only addition of 3bbabefb70 that is a part of a subdir restructure explained in great detail.
Voted to adopt.

Proposal 133312 Governance

Hash and commits match (they were reviewed previously). No arguments to check.
Voted to adopt.

P.S. I like the idea of a link in the summary. This way new users actually end up learning something by reading and doing it logically instead of having a long list of commands in the proposal summary that just needs to be copy pasta’d in the terminal. No offence to anyone this is just my opinion.
Also just for the record I didn’t actually check that LINK yet so not sure how hard it is to follow but anyways unless one is unable to read docs or understand the process just follow another neuron.

[Zane]

Proposal 133311

Both build and args hashes match and all commits have already been verified. Voted to adopt.

Proposal 133312

Build is successfull and all commits match. Voted to adopt.

[3bbabefb70] Moved icp and icrc ledgers to their own dedicated ledger_suites directory, no functionality changes.

[e2cb3d6389] Bump crate version for prost and tonic.

[e9647a7f3e] Regenerate seed for randomness every hour instead of only when the canister is initialized.

[e7ad4e2a9c] Made heap_neurons no longer accessible directly outside of NeuronStore to better abstract internal storage.

[f7791372e9] Removed old hyper and bump prost and tonic versions.

[09e7929d55] Removed test_feature configurations for ic-sns-governance and ic-sns-init.

[5610c60491] Deleted code to init ledger with token balances to a specified principal in test mode.

[f3d13ef565] Set probability of panicking for deprecated methods back to 0.1.

[285a5db07d] Replaced dfn_core with ic_cdk.

[0rions]

Cycles-minting Canister

The commit-id given for the previous proposal is not accessible from github: GitHub - dfinity/ic at 77f48ae63af09b6538b1bf33d3accc3bc74d14f8 This commit does not belong to any branch on this repository and may belong to a fork outside of the repository.

The wasm hash looks good, and the args match.

57f2cbbdbc2d8943173acc6ebd55aca9b9c12e2140b38ff5f20aea7b101ea570 *cycles-minting-canister.wasm.gz

Governance Canister

Changes are mainly refactoring the code.

Wasm hash looks good and no args to be reviewed.

eb6b506617dca22c8d72ab1279e08f0391a7f3bf1827bc23c9d21d9bb9d0afa9 *governance-canister.wasm.gz

Regarding the proposal content comments, having an external link to instructions can be a bit weak as the link’s content can change, I would be in favor of adding more details to the proposal. On top of that, it would be great to have the motivations for making a proposal, e.g https://dashboard.internetcomputer.org/proposal/132134

[daniel-wong]

Regarding verification instructions:

I can re-introduce instructions into future proposal summaries.

As for proposals 133311, and 133312, let me suggest that the issue of verification instructions not block these specific proposals. IMHO, let us instead focus on three core issues:

1. Do these proposals use commit ca88475 (as claimed in the summary)?
2. In the case of Cycles Minting, is () the upgrade args (as claimed in the summary)?
3. Are there any objections to any of the code changes (since the last successful upgrade proposal)?

AFAICT, there are no objections based on these three issues.

Assuming no additional issues come to light, DFINITY will vote in favor of these proposals.

[daniel-wong]

re the current CMC commit: Good catch! The last executed proposal to upgrade CMC (132262) was a hotfix. I think we intended to push the code to the public ic repo, but we either forgot, or our attempts didn’t work, and we didn’t notice. Either way, I’m working to correct this.

[daniel-wong]

Once this pull request gets merged, instructions will again be part of future proposals.

[msumme]

The code for the last CMC hotfix upgrade is now published.

Apologies for this oversight. Some of our procedures changed during the Gitlab → Github migration.

[msumme]

Thanks for your helpful feedback, and pointing out some improvements that can be made here.

Most of the internal proposal process is completely automated, but the forum posts are not (yet), which leaves some room for human error. Apologies for these oversights. We will make sure they’re addressed, and try to make the process smoother.

As Daniel mentioned, we’re already incorporating some feedback back into the proposal process.

[LaCosta]

Firstly I do agree somewhat with @timk11 in the terms that the process of the proposal verification should be made accessible to anyone and since including it should be fairly easy I don’t see any reason not to. The link with the instructions on how to verify is a good idea and should be kept so people can understand what the commands are doing and how the process works. I also think that when making the forum topic it should identify the proposal in question in order to contain discussions in one forum topic and making it easy to search for it on the forum. On the other hand I don’t see this in any way as grounds for rejecting proposals since I think the content of the proposal is much more relevant and if everything is fine with it, the proposal should be accepted, specially in the case of proposal 133311, since it is a continuation of a previously rejected proposal. There is no need for it to be delayed any further.

Proposal 133311

Vote: ADOPT

This proposal as already been reviewed here but for easy access I will provide it here aswell.

Build successful and hashes match.

[3bbabefb70]: Moves icp and icrc1 ledger suites from the rosetta-api/ subdirectory into the ledger_suite/ subdirectory as part of a restructure of the rosetta-api/.

[ebb4dc57d9]: Adds a new query method get_default_subnets that returns a list of default subnets ids.

[df1dbfc8a2]: Changes the globs used to select source files to be more specific by adding file extensions ( .rs for Rust or .proto for protocol files), ensuring only relevant files are included. This solves an issue where when building the compiler would complain about Vim .swp files.

[bbb8a51524]: Adds two new tests test_get_icp_xdr_conversion_rate_certification and test_get_average_icp_xdr_conversion_rate_certification. It solves the problem of using the wrong certification for the get_average_icp_xdr_conversion_rate endpoint which now uses the correct lable LABEL_AVERAGE_ICP_XDR_CONVERSION_RATE. Lastly it standardizes the version of the crates ic-certificate-verification, ic-certification and ic-http-certification to 2.6.0.

[3aa43520c4]: Removes no longer needed migrate function since the migration was performed a long time ago. Also replaces functionality in old state function warning users to perform the migration

[c2b4a0aaf4]: Merges CMC hotfix back to master, refactors code, adds a new validation ensuring that deposit memos don’t exceed 32 characters with const MAX_MEMO_LENGTH

[137faea9ee]: Simplifies the function get_next_multiply_of and adds comments

[4d09678d23]: Sorts Rust #[derive(...)] traits to maintain a consistent style

Proposal 133312

Vote: ADOPT

Build successful and hashes match.

[3bbabefb70]: Moves icp and icrc1 ledger suites from the rosetta-api/ subdirectory into the ledger_suite/ subdirectory as part of a restructure of the rosetta-api/

[e2cb3d6389]: Updates the version of the prost and tonic crates

[e9647a7f3e]: The new method schedule_seeding sets an interval with the variable SEEDING_INTERVAL of 1 hour for reseeding the Random Number Generator.

[e7ad4e2a9c]: Removes the heap_neurons method that returned a reference to the BTreeMap that stored neurons in heap memory. The use cases are now replaced with active_neurons_iter and active_neurons_range methods that encapsulate how neurons are accessed.

[f7791372e9]: Removes hyper and bumps prost and tonic versions

[09e7929d55]: Removes the SNS Governance’s and SNS Init’s --test-feature target and moves this logic to the canister crate

[5610c60491]: Removes the code used when #[cfg(not(feature = “test”))] was true. This code was used to set up a ledger with a principal with a ton of tokens but it’s no longer needed.

[f3d13ef565]: Lowers the probability of deprecated methods panicking from 0.7 to 0.1

[285a5db07d]: Migrates the nns governance from using dfn_core to using ic_cdk. The commit explains really well the changes. It makes small identation adjustments and sorting dependencies. This changes were made possible using a ic_cdk function is_recovering_from_trap that locks the state of a neuron after a panic when calling the ledger. This also means that we can replace Rt::call_without_cleanup with Rt::call_with_cleanup since we no longer rely on a lack of cleaning to prevent that the neuron isn’t locked.