search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 207 forks source link

Shutdown old EC committee.js instance #10137

Open otoole-brendan opened 2 months ago

otoole-brendan commented 2 months ago

Having the old committee stick around is confusing even though it can't actually impact the governed contracts anymore. We should shut it down.

Design

use an upgraded Zoe adminFacet to support shutdown

Note also alternatives considered below.

### Tasks
- [ ] #9716

Test plan

bootstrap test should suffice

otoole-brendan commented 2 months ago

@rabi-siddique @frazarshad could please estimate this ticket?

rabi-siddique commented 2 months ago

I was able to shutdown the old committee.

Work for core eval script and tests remain.

dckc commented 2 months ago

@Chris-Hibbert suggests a different design in https://github.com/Agoric/agoric-sdk/pull/10105#discussion_r1773808289

dckc commented 2 months ago

Let's not add this now. The Committee is the sole holder of the set of questions that have been voted on, and I'd prefer we find a way to write this to vstorage before killing the vats.

Well, we did write it to vstorage, in latestQuestion and latestAnswer each time. The historical values are available from archive nodes. That's how the History tab of https://econ-gov.inter.trade/ works

image

Chris-Hibbert commented 2 months ago

There have been issues with other contracts that made shutdown problematic. I don't know of any such issues with the econ committee, but it's connected to so many other contracts that I'm not confident yet. If @warner gives us a green light for shutting down this contract, I'll be fine with it. @warner is currently unavailable. Perhaps @mhofman can weigh in?

Also, do note that adding a shutdown method won't help with the current transition. The method would only be available to this and future instances. We'd have to upgrade the existing instances in order to add this method. I don't think the current version of the PR upgrades the existing instances. (I'll add comments in the PR.)

dckc commented 2 months ago

We'd have to upgrade the existing instances in order to add this method.

Yes; that's more explicit in #10136 , but that's the idea behind the .shutdown() design choice.

I don't think the current version of the PR upgrades the existing instances. (I'll add comments in the PR.)

FWIW, I don't expect us to land #10105; I view it as a spike, and I expect it to be broken into separate PRs that match issues such as this one before we land it.

rabi-siddique commented 2 months ago

@dckc @Chris-Hibbert As this work item is currently in the needs-design phase, should I concentrate on setting up builder scripts and core evaluations to facilitate the upcoming changes in the contracts? This way, once the design is finalized, I would only need to make minor adjustments to the core eval code based on our design.

dckc commented 2 months ago

@warner in mainnet, the vat in question is

Number Name
v24 zcf-b1-941ef-economicCommittee
dckc commented 2 months ago

@dckc @Chris-Hibbert As this work item is currently in the needs-design phase, should I concentrate on setting up builder scripts and core evaluations to facilitate the upcoming changes in the contracts?

I prefer that #10134 gets done first.

Chris-Hibbert commented 1 month ago

@warner answered my outstanding question in a slack group. He said it would likely be fine to delete the electorate contract vat.

Unfortunately, as I pointed out, that doesn't immediately give us a solution, since the old electorates don't have a straightforward mechanism for shutting down. I think the right resolution might be in #9716, which he asked me to start working on.

This is a longer term solution, and I think the best choice for the near term may be "don't worry about it". The driving concern for actually shutting down the original electorates is mostly about preventing accidents (which I think are unlikely) and mostly inconsequential anyway. If the EC is in frequent contact, they can just agree to ignore or vote down spurious vote proposals. We'll get rid of the old committees in due course anyway.

dckc commented 1 month ago

yes, #9716 is a good solution, but it's not the only one.

The one we have prototyped is to upgrade the contract to add a shutdown() method.

re "don't worry about it", yes, that's an acknowledged option:

options include:

  • leave their existing voting capability alone ...

but we owed product (@otoole-brendan) at least an estimate; and based on a recent estimate to finish the upgrade option, he currently considers it worth doing.

Chris-Hibbert commented 1 month ago

@dckc challenged me:

do you have something against the "upgrade to add a shutdown method" approach that has been prototyped?

For the instances of the contract that we don't intend to shutdown, we have to think carefully about who has access to the new method. It's probably in the same facet as addQuestion() and getPoserInvitation(), so anything that wants to act as a contract governor has the ability to shutdown its committee, which may be relied on by other contracts. It makes me squeamish. If we could just upgrade the vats that we want to shutdown, that might be okay, but I think we need to be careful that the new bundle doesn't become the standard installation for committee contracts.

Chris-Hibbert commented 1 month ago

@warner added

I'm not a fan of upgrade-to-add-shutdown, it feels like a bunch of work to do something that's more easily managed through the adminNode, and it's work that needs to be done separately for each contract (to find all the Kinds that need redefining), and it introduces new failure modes ("sorry your attempt to shut me down failed because the new one-off version forgot to redefine one Kind, I'll just keep running until you figure out where it lives, hope you remembered to retain the KindHandle"). And because upgrade-to-shutdown is already within the authority of the adminNode-holder, adding just-shut-it-down isn't increasing its authority at all.

dckc commented 1 month ago

IBIS style summary of design space as I currently understand it. @otoole-brendan how about I move it to the description?

See also what to do with outdated EC voting rights? #10137 in Codecider, which renders it like this:

image

text form:

? what to do with outdated EC voting rights? #10137
  : leave their existing voting capability alone
    . as noted above, after replaceElectorate(), it has no effect.
    + low implementation burden
    - risk of accidents
      + unlikely
    - does not provide early detection of errors if/when the wrong contract is used
      + unlikely
  : add E(creatorFacet).shutdown() to committee.js
    - anything that wants to act as a contract governor has the ability to shutdown its committee
  : add a method like stopAddingVotes()
    . sets a flag that blocks use of addQuestion() or getPoserInvitation().
  : use an upgraded Zoe adminFacet to support shutdown
    . based on #9716
    ! needs estimate
dckc commented 1 month ago

@otoole-brendan since we seem to be choosing the "leave it alone" option, I'm re-scoping this back to "shutdown the old committee", and we can postpone it indefinitely (P3).

warner commented 1 month ago

FWIW, given how relatively small the EC vat is, I think it would make a fine candidate to be the first one we terminate under the "slow vat deletion" code (#8928) that will land in upgrade18. We could use that deletion as a final test of the slow-deletion, and could use what we learn from it to tune the parameters before deleting large vats like the price feeds. So I'd be +1 on actually deleting it. Also we have the right adminFacet to do the job.

The old auctioneer (v45) is even smaller, and is already unused (replaced in the "vaults / restart auctioneer" core-eval (gov76, block 16544918, 04-sep-2024), and would thus be an even better candidate, but due to a bug we dropped the zoe adminFacet for it, so triggering its termination may be difficult to arrange.