Proposal to elect new release rc--2024-07-18_01-30--github

Governance
,
1

Hello there!

We are happy to announce that voting is now open for a new IC release.
The NNS proposal is here: IC NNS Proposal 131364.

Here is a summary of the changes since the last release:

Release Notes for release-2024-07-18_01-30–github-base (de29a1a55b589428d173b31cdb8cec0923245657)

Changelog since git revision a3831c87440df4821b435050c8a8fcb3745d86f6

Features:

  • 9a3aa19d7 Consensus: Add backoff and jitter to HostOS upgrades (#395)
  • b518354ed Crypto: add new signature verification package initially supporting canister signatures
  • ecda2039f Crypto,Networking: quinn and rustls upgrade
  • c1787dc74 Execution,Runtime: Print instructions consumed in DTS executions in a more readable form
  • e424cae83 Message Routing: Implement handling reject signals from incoming stream slices.
  • 7e4aea78d Networking: publish https outcalls adapter with http enabled for dfx
  • 7c559a1eb Node: Pull HostOS upgrade file in chunks
  • c7b168f23 Runtime: Adjust max number of cached sandboxes
  • 4d50869f9 Runtime: Reland switch to compiler sandbox for compilation

Bugfixes:

  • ad6197507 General: upgrade the bytes crate since v1.6.0 was yanked due to a bug
  • 7af0814f4 Consensus: ic-replay when DTS is enabled
  • 073c2bc1f Execution,Runtime: Follow up on the reserved cycles limit fix (#383)
  • 9d0d27eb4 Networking: use the Shutdown struct instead of explicitly passing the cancellation token for the sender side of the consensus manager
  • 27ef4655c Runtime: Free SandboxedExecutionController threads (#354)
  • 9d0d27eb4 Runtime: Revert “feat: Switch to compiler sandbox for compilation”

Performance improvements:

  • fe231b385 Crypto: Reduce the size of randomizers during Ed25519 batch verification (#413)
  • 6057ce233 Execution,Runtime,Consensus: Reduce cost of cloning tSchnorr inputs (#344)

Chores:

  • 9f5c513ac Boundary Nodes,Networking,IDX: upgrade rustls
  • 22dd9646e Boundary Nodes,Node,NNS,IDX,pocket-ic: upgrade external crates and use workspace version
  • ebeb49ea8 Consensus: Rename ecdsa modules, EcdsaClient, EcdsaGossip and EcdsaImpl (#367)
  • 24bc0a6a5 Consensus: ic-replay: do not try to verify the certification shares for heights below the CU
  • e6ac79db8 Consensus: Rename EcdsaPreSig*, EcdsaBlock*, EcdsaTranscript*, and EcdsaSig*
  • 9d0d27eb4 Consensus: Rename EcdsaPayload
  • 42a6af85b Crypto: Always optimize the curve25519-dalek crate
  • fac32ae6f Crypto: Remove support for masked kappa in threshold ECDSA (#368)
  • 919057452 Crypto: Implement ZIP25 Ed25519 verification in ic_crypto_ed25519
  • d3b3dce13 Execution,Runtime: Update Wasm benchmarks
  • f940a9f8c Execution,Runtime: Rename iDKG key to threshold key
  • 2e269b77f Interface,Consensus: Remove proto field used to migrate payload layout (#380)
  • 25e7a7f08 Networking: abort artifact download externally if peer set is empty
  • 9d0d27eb4 Networking(ingress-watcher): Add metric to track capacity of the channel from execeution
  • 364fe4f38 Node: firewall counter exporter (#343)
  • 245c20f43 Node: Log HostOS config partition (config.ini and deployment.json)
  • 3bda1a2a2 Node: Update container base images refs [2024-07-12-0623]
  • 7708333b2 Runtime: Derive ParitalEq for all sandbox IPC types (#374)
  • fa1869466 Runtime,Message Routing,Networking(fuzzing): fix clippy warnings for fuzzers

Refactoring:

  • de3425fa6 Crypto: replace ed25519-consensus with ic-crypto-ed25519 in prod (#347)
  • 9d0d27eb4 Networking,Consensus: move the PriorityFn under interfaces and rename the PrioriyFnAndFilterProducer to PriorityFnFactory

Tests:

  • 47c7302c4 Crypto: Re-enable NIDKG cheating dealer solving test
  • b5b9e24b7 Execution,Runtime: Support signing disabled iDKG keys in state_machine_tests
  • 76ef61bc3 Execution,Runtime,IDX: Make system api test to be state machine test (#377)
  • 9d0d27eb4 Message Routing,IDX: check canister queue upgrade/downgrade compatibility against published version
  • 506bbfdc1 Networking: decompress bitcoin data inside tests
  • db73f9385 Runtime(fuzzing): create new test library wasm_fuzzers
  • d1e55daec Runtime,NNS,IDX: move some Bazel rules out of the system test defs

IC-OS Verification

To build and verify the IC-OS disk image, run:

# From https://github.com/dfinity/ic#verifying-releases
sudo apt-get install -y curl && curl --proto '=https' --tlsv1.2 -sSLO https://raw.githubusercontent.com/dfinity/ic/de29a1a55b589428d173b31cdb8cec0923245657/gitlab-ci/tools/repro-check.sh && chmod +x repro-check.sh && ./repro-check.sh -c de29a1a55b589428d173b31cdb8cec0923245657

The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, must be identical, and must match the SHA256 from the payload of the NNS proposal.

3 Likes
2

:pray: Thank you DFINITY for this release. As always I really appreciate the effort and skill that went into it.

However, the way in which this proposal has been presented is inaccurate and misleading, and on that basis I’ll be rejecting this proposal. I invite the community to consider following my vote (see here for some general context) → Do U C What IC? 👀 IC Proposals that mislead... and a 10,000 ICP Giveaway!. I’m planning to upgrade my public neuron to a named neuron soon.

9d0d27eb4 is incorrectly referenced in this proposal. It doesn’t actually form part of this release. In addition it’s been referenced multiple times with commit summaries that are unrelated to that specific commit.

For anyone interested, I believe these are the correct commit hashes that should have been referenced by the proposal summary (instead of 9d0d27eb4):

  • f9c4b9999 Networking: use the Shutdown struct instead of explicitly passing the cancellation token for the sender side of the consensus manager
  • 1d40efc9f Runtime: Revert “feat: Switch to compiler sandbox for compilation”
  • d4517755f Consensus: Rename EcdsaPayload
  • ff9d79077 Networking(ingress-watcher): Add metric to track capacity of the channel from execeution
  • 687010660 Networking,Consensus: move the PriorityFn under interfaces and rename the PrioriyFnAndFilterProducer to PriorityFnFactory
  • 6c9c00c39 Message Routing,IDX: check canister queue upgrade/downgrade compatibility against published version

Even when you account for this, I think there are numerous salient commits that are ignored by this proposal summary:

  • eb775492d chore: firewall counter exporter (#343)
  • 2a530aa8f chore: CON-1257 Rename ecdsa modules, EcdsaClient, EcdsaGossip and EcdsaImpl (#367)
  • 460693f61 perf: CON-1360 Reduce cost of cloning tSchnorr inputs (#344)
  • 1413afe92 refactor(crypto): CRP-2536 replace ed25519-consensus with ic-crypto-ed25519 in prod (#347)
  • dbaa4375c chore(crypto): CRP-2393 Remove support for masked kappa in threshold ECDSA (#368)

I suspect there are more but I’ve already gone over my intended time allocation, and I’m trying to be strict with myself.

On a side note, thanks DFINITY for taking on board my comments regarding 1-commit branches. I gather from the number of commits that are directly on master (instead of explicitly merged in from 1-commit branches) that a change in procedures has been implemented (or this may just be a coincidence :person_shrugging:).

I wouldn’t be surprised if this is the root cause for the worse than normal proposal summary (due to tooling that has to make some assumptions about the repo structure, for practical reasons). Another reason that I think it would be great to plan to get away from the need for this sort of tooling longer term.

2 Likes
3

I also just noticed the unelection component to this proposal (I got wrapped up with the above).

Based on IC-OS election proposal history, there currently appear to be 11 blessed replica versions registered, 5 of which would be unelected by this proposal (if it executes).

I’ve listed these below, ordered by elected date, and crossed out the versions that would be unelected.

  • e3fca54, elected 2024-06-24 (proposal 130727), UNELECTION PROPOSED, running on 0 subnets
  • ae3c4f3, elected 2024-06-24 (proposal 130728), UNELECTION PROPOSED, running on 0 subnets
  • 9c006a5, elected 2024-06-25 (proposal 130748), UNELECTION PROPOSED, running on 0 subnets
  • 2e269c7, elected 2024-07-01 (proposal 130818), UNELECTION PROPOSED, running on 0 subnets
  • b6c3687, elected 2024-07-01 (proposal 130819), UNELECTION PROPOSED, running on 0 subnets
  • e4eeb33, elected 2024-07-08 (proposal 130984), running on 0 subnets
  • 5849c6d, elected 2024-07-08 (proposal 130985), running on 0 subnets
  • 16fabfd, elected 2024-07-11 (proposal 131028), running on 0 subnets
  • 7dee901, elected 2024-07-11 (proposal 131032), running on 1 subnets
  • a3831c8, elected 2024-07-15 (proposal 131054), running on 36 subnets
  • 0d2b396, elected 2024-07-15 (proposal 131055), running on 0 subnets

Can I ask why the e4eeb33, 5849c6d and 16fabfd aren’t being unelected (they’re no longer running on any subnets either)? It’s also interesting that 0d2b396 has never been deployed to a subnet (but was elected earlier in the week). This is just a matter of curiousity :slightly_smiling_face:

1 Like
4

I can’t comment on most of your points, but the two most recent LSMT disabled replica variants were explicitly elected as backup in an abundance of caution for the re-rollout of the feature to the NNS subnet. So they are intentionally on 0 subnets in the absence of problems. As you can see, we are no longer doing that as of this release.

2 Likes
5

:pray: Thanks @stefan.schneider, I really appreciate your explanation. That makes a lot of sense.

1 Like
6

Reviewers for the CodeGov project have completed our review of this replica update.

Proposal ID: 131364
Vote: REJECT
Full report on OpenChat

At the time of this comment on the forum there are still 2 days left in the voting period, which means there is still plenty of time for others to review the proposal and vote independently.

It is important to note that all CodeGov reviewers found significant discrepancies between the changes described in the Summary of the proposal and the actual commits that are included in this release. This means that if this proposal is adopted, there will forever be an inaccurate characterization of the changes that were included in the Summary documentation that can be researched on the dashboard for these changes. These inaccuracies are presumably the details that many voting neurons used as the basis for their vote if they didn’t dive deeper into the GitHub commits or the code. Bad data is worse than no data. It’s worth considering on future proposals if a Summary should be provided at all if it is going to contain inaccurate information. We are aware that DFINITY uses some automated tools to create these reports and there are currently issues with the accuracy of that tooling. It takes time to resolve issues like this, but it’s a fair argument that updates to the IC replica must continue. Hopefully it will be a priority to resolve these issues, but perhaps in the meantime it would be worth removing the Summary section until future proposals can be presented more accurately. Then perhaps it will draw attention to the community that we must dive deeper into the changes in order to properly evaluate the proposal. This is highly undesirable, but it seems even more undesirable to release a proposal where the Summary that is provided is inaccurate.

Below are key points made by each reviewer in their report regarding their reasons for their vote. The CodeGov neuron has voted, but not all reviews have been uploaded yet. This post will be edited as new reports are submitted.

@Lorimer - full report

Build successful and hashes generated on my machine match (CDN and local build), and the GuestOS hash matches the proposal payload.

Rejected due to numerous proposal summary issues. I’ve raised this on the forum.

I’ve also conducted analysis on the proposed unelections, which look safe. However I’m still curious about the policy that’s being employed to determine which versions get unelected. Raised another question on the forum.

@ilbert - full report

For the Execution and Runtime layers, this proposal doesn’t introduce any significant changes.

There are some discrepancies between the proposal summary and the actual commits of this release. Below is what I found.
…
All the commits made between this release and the previous release can be found on GitHub.
Apart from the wrong commits listed, this release doesn’t seem to introduce other commits that are not listed. For this reason and since I was able to find and verify the correct commits in the release, I chose to adopt the release anyway.

@hpeebles - full report

Hashes match but many of the commits in the proposal message were incorrect. Some of them link to commits that are also recent but don’t relate to IC-OS or simply don’t contain the changes mentioned in the proposal message.

Also a few of the mentioned commits linked to large merge commits that seem to be made up of many commits from last week’s release.

Every commit I reviewed looked fine to me, but given these issues I have decided to vote to reject given that I’m fairly sure I haven’t viewed all of the commits contained in the release.

@massimoalbarello - full report

Decided to adopt even if some descriptions in the changelog were wrong as there were no problems i detected in the changes.

Commits 9a3aa19d7 , fac32ae6f , 2e269b77f and 9d0d27eb4 do not correspond to description in changelog. In particular, 9d0d27eb4 is reported multiple times in the changelog with different descriptions…. Though nothing critical, the changelog is pretty random

@Zane - full report

I’ve reviewed all commits for Execution, Runtime, and Message Routing topics. This week’s proposal has inconsistencies in its changelog, namely duplicated commit ids for different code changes and wrong descriptions for some commits. In the past it occurred that some commits were omitted from the changelog due to some oversight made by tooling used to generate them, which is understandable, but the errors made with this proposal are not acceptable as they could have been spotted quite easily and it shouldn’t be up to reviewers to figure out the correct commit ids. For these reasons, even though I haven’t spotted any malicious code changes, I’m going to reject this proposal.

@ZackDS - full report

I built the replica without errors and verified hashes match. I voted to reject due to the inconsistencies in the changelog.

4 Likes
7

These are all part of the same weekly release: dre/release-index.yaml at main · dfinity/dre · GitHub

As long as there’s one active version for that release, we keep all versions. The reasoning is - we can always say automatically that a version that is old and is not deployed to any subnet can safely be removed from elected list. But for the releases from the same week, we cannot determine if these versions will still be useful or not. For example, if we’re releasing a feature, and it gets deployed to all subnets, and “base” version is not deployed anywhere anymore, we still want to be able to quickly revert the feature rollout to some subnet if it experiences issues.

4 Likes
8

DFINITY Foundation voted to reject the proposal because there are too many issues with the release notes. As such, we won’t propose new release for this week.

The main cause of issues is caused by GitHub migration from last week and some changes we did for release notes to handle merge commits a few weeks back. Since we do not have merge commits anymore, the logic for finding merge commits is severely broken and resulted in getting wrong commit hashes. The commit messages however seems to be correctly representing the changes in this release, but the links to commits and commit hashes are completely wrong.

6 Likes
9

Thanks @Luka, I think this helps explain some similar questions I’ve had in the past. Much appreciated :pray:

1 Like