NNS governance - bug in proposal 136693

Dear all,

We are experiencing a problem as a result of a bad NNS Governance upgrade. This introduced a bug due to which existing proposals no longer have a defined proposal topic. As a result, the NNS dapp provides some unexpected errors, for example when looking at the launchpad.

Moreover, this led to some NNS proposals being garbage collected earlier than usual.
The Governance team is working on a fix. For now, we recommend using the ICP Dashboard for listing proposals.

We are going to provide more information in this thread as soon as we know more.

4 Likes

Proposal #136704 for Governance — Zack | CodeGov

Vote: Adopted

Reason: The build is reproducible and the wasm hash is a match.
The canister id rrkah-fqaaa-aaaaa-aaaaq-cai is correct and the install mode is upgrade.

The fix adds the back_fill_topics function to update the proposals by assigning a topic to all proposals that currently have an Unspecified topic. As can be seen it worked as expected and everything is back to normal.

About CodeGov

CodeGov has a team of developers who review and vote independently on the following proposal topics: IC-OS Version Election, Protocol Canister Management, Subnet Management, Node Admin, and Participant Management. The CodeGov NNS known neuron is configured to follow our reviewers on these technical topics. We also have a group of Followees who vote independently on the Governance and the SNS & Neuron’s Fund topics. We strive to be a credible and reliable Followee option that votes on every proposal and every proposal topic in the NNS. We also support decentralization of SNS projects such as WaterNeuron, KongSwap, and Alice with a known neuron and credible Followees.

Learn more about CodeGov and its mission at codegov.org.

2 Likes

The governance team has submitted two proposals to address the above issue. Since this is a bug in governance that should be fixed as quickly as possible, DFINITY voted on these proposals right away, not waiting the normal 3 days.

These are the proposals:

  • Proposal 136704 to upgrade the NNS Governance. In the post-upgrade, Governance goes through all proposals and fills in the currently missing topic field. This is done with the same logic that has always been used by governance to decide a proposal’s topic.

  • Proposal 136705 to upgrade the NNS frontend dapp. On the launchpad, the NNS dapp used to show the NNS proposal used to create the respective SNS. Since not all of these proposals are available on NNS governance, the NNS dapp points the users to alternative dashboards where they can look at the proposals (the dashboard and vpGeeks). It also shows a link to the forum post here for an explanation of why the proposal is not directly visible in the NNS dapp (as it cannot be fetched from the backend).

4 Likes

proposals - [136704] Cyberowl | CodeGov

Proposals:

136704

Vote: [ADOPT]

Reason & Feedback:

I successfully built and verified the hash. All the commit descriptions match their code changes.

Checks:

Hash Match: [PASS]
Target Canister: [PASS]
Proposer Check: [PASS]

0dc5478d5a
Backfills proposal topics.

1 Like

Adopt 136704

Thanks @aterga for being reactive with this fix!

Proposal

  • The upgrade args correspond to the empty args.
  • The install mode is indeed upgrade.
  • The wasm hash are reproducible.

Code Review

[0dc5478d5a]: Loop through the proposals and fills the missing topic fields.

Proposal 136704 | Tim - CodeGov

image

Vote: Adopt

Build is successful and hashes match. This proposal consists of a single commit that adds a method Governance::back_fill_topics, used within Governance::new_restored to fix a bug in which some proposals did not have an assigned topic after an upgrade and therefore were not displayed in the NNS dapp.

About CodeGov

CodeGov has a team of developers who review and vote independently on the following proposal topics: IC-OS Version Election, Protocol Canister Management, Subnet Management, API Boundary Node Management, Node Admin, and Participant Management. The CodeGov NNS known neuron is configured to follow our reviewers on these technical topics. We also have a group of Followees who vote independently on the Governance and the SNS & Neurons’ Fund topics. We strive to be a credible and reliable Followee option that votes on every proposal and every proposal topic in the NNS. We also support decentralisation of SNS projects such as WaterNeuron, KongSwap, and Alice with a known neuron and credible Followees.

Learn more about CodeGov and its mission at codegov.org.

Do any of the Protocol Canister Management reviewers run the latest release locally and try it out before voting to adopt? (in addition to building and reviewing the code)

I’m just wondering why this wasn’t caught during testing.

2 Likes

I haven’t personally been doing this. I’ve been following the remit in the grants description:

@aterga @lara - Since @Lorimer has asked, what sort of testing process is usually done prior to submitting canister management proposals? Is this the sort of bug that would typically be detected in this way? (And I do appreciate that not everything can be foreseen.) Anything you would like us as reviewers to do differently?

@catthew91 That’s a fair question to ask. I don’t know why all these posts keep getting flagged. Not sure about that figure though. You can check the actual figures and a detailed outline of the purpose of the grants in the same post I’ve linked just above. From the IC-OS side I know there are very extensive tests embedded in the code and a number of different systems involved, but it does take a lot to catch every eventuality.

4 Likes

Thanks @timk11, I’m only asking because I care. I can tell that you do to, and I appreciate your response.

Do you think this is the sort of bug that could be detected this way?

Do you think you should be doing anything differently?


Regardless of CodeGov’s or DFINITY’s stance, CO.DELTA are aiming to raise the bar in this respect.

The IC deserves reviewers who go above and beyond. In any case, testing the code you’re reviewing is normal code review practice. Unlike the IC OS, which has limitations in terms of the practical test surface (though PocketIC can be useful here), I don’t see any reasonable excuse for not running Protocol Canister Management changes locally as part of a typical review, particularly on canisters such as NNS Governance.

1 Like

Same reason bugs are introduced to production on software since the dawn of computers

1 Like

So you’re saying why bother (or are you saying something else)?

Nah just that need to do better… jippity jip jip link update:

1 Like

This is very poor show. You are in an elected position. You are not beyond reproach. This is a forum. Please pull your act together and be civil. If you have time on your hands to write posts like that, why not answer some basic questions.

Have you tried to run this specific release locally yet? What were your observations? Would you have realistically come across the issue had you not known it existed before attempting your testing? I’m curious how you would answer your own question if you run this testing objectively.

This sounds great @Lorimer, but would you please highlight where in that post you demonstrate how you will “raise the bar in this respect”? It’s a post of your announcement of co.delta team members and your intention to apply for grants including protocol canister management, but how does that equate to you raising the bar? Or are you simply stating that raising the bar means having more reviewers? Perhaps with more reviewers someone would have caught this issue.

Thanks @codecustard . That’s a fantastic overview of the level of complexity required for testing an update thoroughly and well worth everyone’s while to read.

@Lorimer I always admire your diligence and attention to detail. In this case my question is more about the extent of testing that the Dfinity team undertakes and the sorts of things that might still slip through (in line with your original question). It certainly behooves a development team to perform extensive testing before releasing an update and I’m sure this is happening here, but it’s impossible to catch everything and I’m interested in the team’s thoughts on this. I don’t want to see too much back and forth in this thread as it tends to take things off-topic and detract from the original issue being discussed.

1 Like

It’s in the name. We don’t just Look, we’re striving to Test and Automate what we practically can.

None of the $20K ICP a month that’s available for Protocol Canister Management reviewers currently comes CO.DELTA’s way. I, as one example, am busy reviewing other topics in my spare time (while juggling a full-time job), and building tooling involving the NNS Governance canister. You can bet your bottom dollar I would have taken a look at running this (and any other PCM proposal) locally. I already have tests that I run against a local instance of NNS Governance (testing canisters I’m building that depend on NNS Governance, rather than testing NNS Governance itself - but the tests essentially serve the same purpose). A test suite is never going to be comprehensive, but it’s best practice to add regression tests on a case by case basis as you go, as needed.

Not principally. The IC needs people spending more time on these reviews. That is what’s needed far more than more people spending less time. There’s a finite amount of funding. You’re not putting it to work effectively (particularly after your and cris’ admin and marketing cuts are taken). One of the many reasons I left CodeGov.

This is an extremely important area. The IC needs to get this right to survive and thrive. The whole system depends on effective governance. This is the only reasons I’m being so vocal, and trying to make people see where we currently are. We have a long way to go, and we need people who are willing to work smarter and harder to help take us there. At least, this is my contention.

2 Likes

I can agree with both POVs from @wpb and @Lorimer. While it is unlikely we will find bugs by testing canister at first, it is important to always improve our automatation and knowledge - after some time it will get more likely, especially since ProtocolCanisterManagement is for single canisters.

5 Likes

I agree too but @lorimer post reads more like an ad than a genuine need to work with everyone in the ecosystem. And this is the main issue I have with this. It has become very political. He makes it seems like CodeGov is Wenzel only and it is NOT.

4 Likes

Hey Jefri, all I’m interested in is raising the bar, both in terms of decentralisation and due diligence. If you think you just read an advert, you’re not appreciating the failings highlighted. I’d happily see all of the grants go to completely different teams if they can demonstrate high quality decentralisation, due diligence, and drive (rather than only striving to meet bare minimum criteria).

1 Like