Record instances that will be replaced so we can manage them #10680

Chris-Hibbert · 2024-12-11T19:17:23Z

closes: #10669

Description

Record instances of contracts that will be replaced so we can find them later and be able to manage them.

Security Considerations

We've lost the handle to some vats before, and it's a hassle.

Scaling Considerations

The problem gets worse as we upgrade more vats.

Documentation Considerations

None.

Testing Considerations

Verified that Auctions and Committee are present. The priceFeeds vary too much across test environments to be worth checking.

Upgrade Considerations

Yes..

cloudflare-workers-and-pages · 2024-12-11T19:18:04Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1581127`
Status:	✅ Deploy successful!
Preview URL:	https://2f7a3e79.agoric-sdk.pages.dev
Branch Preview URL:	https://10669-saveinstances.agoric-sdk.pages.dev

View logs

mhofman

My first reaction is that the explicit naming per "upgrade action" feels error prone. Is there a reason we cannot make this retired instance collection a SetStore and just push into it? Maybe have the retiredContractInstances be a MapStore<string, SetStore<Instance>> ?

warner · 2024-12-12T00:49:50Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+  );
+  const econeconomicCommitteeOriginal = await economicCommitteeOriginalP;
+  contractInstanceMap.init('electorate-v24', econeconomicCommitteeOriginal);
+  retiredContractInstances.resolve(contractInstanceMap);


What's the frequency (or "per-ness", as Chip would say) of this resolution? Relatedly, how frequently does this replaceElectorate.js proposal get run? I don't see a provide pattern here, so I'm worried that this might work correctly during upgrade18, but the next time this proposal is run, we'll create a lame-duck constractInstanceMap, put something important in it, and then the retiredContractInstances.resolve() will be ignored because that key is already in the promise-space. That would lose a reference.

I don't know how to square a provide-pattern with the promise space. Maybe there should be a separate (effectively idempotent) function whose only job is to create contractInstanceMap and resolve it into the promise space. Then the electorate replacement function would only consume (and not produce), and we wouldn't run the risk of this ignored-duplicate-resolve problem.

Whatever next time is, it'll have to reset() in order to re-assign the Map. This is brittle, and will need to be modified the next time it's used, but that isn't the failure mode.

Then the electorate replacement function would only consume (and not produce), and we wouldn't run the risk of this ignored-duplicate-resolve problem.

I'd be happier if I knew how to do that, but I couldn't think of a way. If that's more important than getting this in tonight, I'll talk to @dckc about how to do that tomorrow.

@dckc reminded me of the standard pattern for this. Each intended writer creates a MapStore and attempts to save it as retiredContractInstances. One of them will win, the others will be ignored. Each then consumes retiredContractInstances, and adds a value to it. They'll all consume the same thing, and they won't know or care whether they were the one that won the first race.

I'll defer to his advice, and I also don't know how to code it the way I suggested (it's in the "looks easy because I haven't tried" category :), but that pattern doesn't sound great to me. "The others will be ignored" might be reliable, but it relies upon non-obvious behavior of the promise-space resolve, which makes it less legible and an ongoing source of questions ("why read this back out of consume when you just put it in there on the previous line?" and "why doesn't this duplicate resolve throw?").

"The others will be ignored" might be reliable, but it relies upon non-obvious behavior of the promise-space resolve, which makes it less legible and an ongoing source of questions ("why read this back out of consume when you just put it in there on the previous line?" and "why doesn't this duplicate resolve throw?").

I agree that it's less than obvious, but it's reliable.

If we ever change our mind about "silently ignore attempts to assign values that are already set", we'll have to change a large number of other things.

Chris-Hibbert · 2024-12-12T00:51:17Z

My first reaction is that the explicit naming per "upgrade action" feels error prone. Is there a reason we cannot make this retired instance collection a SetStore and just push into it? Maybe have the retiredContractInstances be a MapStore<string, SetStore> ?

We definitely could do that, but it feels to me like that would make it hard to distinguish the various instances of any contract. I want them to have labels, though I admit I don't see a way to automate it that makes me happy.

warner · 2024-12-12T00:51:33Z

packages/inter-protocol/src/proposals/deploy-price-feeds.js

+  retiredContractInstances.init(
+    // XXX tail of label needs to vary with upgrade. BundleId would be different
+    // from previous, but is not necessarily unique.
+    `priceFeed-${AGORIC_INSTANCE_NAME}-u18`,


Yeah the -u18 suffix is probably the best we can do, short of using a list and appending to it (which would make future proposals that need to reference a specific one harder to write and review). At least if someone re-uses this proposal and forgets to update the suffix, that .init() will throw and we should catch it during testing.

boardID works better and can be automated.

warner · 2024-12-12T00:52:07Z

packages/inter-protocol/src/proposals/add-auction.js

+  // save the auctioneer instance so we can manage it later
+  const retiredContractInstances = await retiredContractInstancesP;
+  const legacyInstance = legacyKit.instance;
+  retiredContractInstances.init('auction-vat157', legacyInstance);


maybe add a note that this suffix needs to be changed each time this proposal is re-used

@dckc suggests using the boardId instead.I think that would work.

warner · 2024-12-12T20:24:44Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+  retiredInstanceWriter.resolve(contractInstanceMap);
+
+  // get the actual retiredContractInstances
+  const retiredInstancesWriter = await retiredInstancesP;


nit: having both retiredInstanceWriter and retiredInstancesWriter (it took me a few minutes to spot the extra s in the second one) is confusing. Could the second one simply be called retiredInstances?

yes, definitelly. I don't know how that happened. The latter is not even a writer.

warner · 2024-12-12T21:07:43Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+  // get the actual retiredContractInstances
+  const retiredInstances = await retiredInstancesP;
+  // Record the retired electorate vat so we can manage it later.
+  const econeconomicCommitteeOriginal = await economicCommitteeOriginalP;


nit: spelling

turadg · 2024-12-12T21:24:01Z

packages/builders/scripts/testing/recorded-retired-instances.js

+  },
+}) => {
+  trace('Start');
+  const retiredContractInstances = await retiredContractInstancesP;


we do this a lot. I wonder if it would be better not to destructure consume in the params so we could do,

const { contractKits, retiredContractInstances } = deeplyFulfilled(consume);

totally optional

deeplyFulfilled(consume) sounds bad. At best, it would try to grab everything in the space. But I suspect the properties are not even enumerable.

oh right. we'd need something like await space(consume, ['retiredContractInstances')

packages/builders/scripts/testing/recorded-retired-instances.js

packages/inter-protocol/src/proposals/add-auction.js

packages/inter-protocol/src/proposals/deploy-price-feeds.js

turadg · 2024-12-12T21:34:48Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+  // if retiredContractInstances doesn't exist, create it.
+  const contractInstanceMap = makeScalarBigMapStore(
+    'retiredContractInstances',
+    { durable: true },
+  );
+  retiredInstanceWriter.resolve(contractInstanceMap);


please extract a function that documents why it can be called in multiple places at any time.

it should also document the naming scheme: {agoricNames.instance key}-{boardID}

The most natural form is generic creation of a Map in promiseSpace, but this doesn't leave a place to document the usage of retiredContractInstances. Would you rather have a shared function called createRetiredContractInstancesMap, which could document the pattern?

f4c67ec is okay but consider something more direct: 515f772

turadg · 2024-12-13T01:45:31Z

a3p-integration/proposals/z:acceptance/recorded-retired.test.js

@@ -5,6 +5,7 @@ import { evalBundles } from '@agoric/synthetic-chain';
 const SUBMISSION_DIR = 'recorded-instances-submission';

 test(`recorded instances in u18`, async t => {
-  await evalBundles(SUBMISSION_DIR);
+  const result = await evalBundles(SUBMISSION_DIR);
+  console.log('recorded retired instance result:', result);


I forgot to mention this and it's not worth a rev: if you tested the result you wouldn't need a t.pass. It would also help document what's expected

Chris-Hibbert · 2024-12-13T04:56:07Z

packages/builders/scripts/testing/recorded-retired-instances.js

+  // I don't know why it's neither in governedContractKits nor contractKits
+  // assert(await E(governedContractKits).get(auctionInstance));


I draw your attention to this inability, but I'm going to merge it anyway. The old auction instance has been stored, but I haven't been able to look it up in in governedContractKits or contractKits. the economicCommitttee instance is in contractKits.

kits typically make it into governedContractKits by way of startGovernedUpgradable, but startAuctioneer pre-dates that. It calls E(zoe).startInstance(contractGovernorInstallation, ...) directly.

mergify · 2024-12-13T06:48:58Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

dckc

to the robots: I'm done reviewing this; please take it off my queue.

closes: #10669 ## Description Record instances of contracts that will be replaced so we can find them later and be able to manage them. ### Security Considerations We've lost the handle to some vats before, and it's a hassle. ### Scaling Considerations The problem gets worse as we upgrade more vats. ### Documentation Considerations None. ### Testing Considerations Verified that Auctions and Committee are present. The priceFeeds vary too much across test environments to be worth checking. ### Upgrade Considerations Yes..

### Description Cherry-picks the following commits from master: - #10672 (3b478fb) - #10668 (a74161c) - #10680 (c883c39, 1581127) No new upgrade name has been added. Done partially via git cherry-pick and via the following rebase-todo: ``` # PR #10680 Branch Record-instances-that-will-be-replaced-so-we-can-manage-them-10680- label base-Record-instances-that-will-be-replaced-so-we-can-manage-them-10680- pick c883c39 feat: record instances that will be replaced so we can manage them pick 1581127 refactor: provideRetiredInstances label pr-10680--Record-instances-that-will-be-replaced-so-we-can-manage-them-10680- reset base-Record-instances-that-will-be-replaced-so-we-can-manage-them-10680- merge -C d35659b pr-10680--Record-instances-that-will-be-replaced-so-we-can-manage-them-10680- # Record instances that will be replaced so we can manage them (#10680) ```

refs: #10680 ## Description We misplaced the adminFacets that would allow us to manage Auctions after they've been replaced and were about to do it again. The auctions were [last upgraded](https://github.com/Agoric/agoric-sdk/blob/43345a561fbdf7621c369abb15e6839f7c696565/packages/inter-protocol/src/proposals/add-auction.js#L157) in `agoric-upgrade-16av`. That code fails to save the instance's adminFacet, and only stores the contractKit in bootstrap promise space under the name `auctioneerKit`, where it will be overwritten on upgrade. Our other contracts now save a copy of the `contractKit` in either `contractKits` or `governedContractKits`, indexed by the instance, so the facets will hang around. This saves the old auctioneer during upgrade so we can manage it later (upgrade, terminate, change parameters). ### Security Considerations Losing our last handle for vats is a problem. ### Scaling Considerations We're upgrading vats to deal with scaling. ### Documentation Considerations None. ### Testing Considerations there was a test in #10680 which looked for this kit in `governedContractKits`, but I commented it out when it didn't succeed. It succeeds now. ### Upgrade Considerations Yes.

Chris-Hibbert added deployment Chain deployment mechanism (e.g. testnet) code-style defensive correctness patterns; readability thru consistency Inter-protocol Overarching Inter Protocol contract-upgrade next-release about next agoric-sdk or endo release labels Dec 11, 2024

Chris-Hibbert self-assigned this Dec 11, 2024

Chris-Hibbert requested a review from a team as a code owner December 11, 2024 19:17

Chris-Hibbert requested a review from AgoricTriage December 11, 2024 19:17

Chris-Hibbert added the force:integration Force integration tests to run on PR label Dec 11, 2024

Chris-Hibbert mentioned this pull request Dec 11, 2024

chore: uprade 18 cherrypicks round 5 #10676

Merged

Chris-Hibbert force-pushed the 10669-saveInstances branch 2 times, most recently from 941288e to 64d75a1 Compare December 11, 2024 20:07

toliaqat requested review from warner and mhofman December 11, 2024 23:19

Chris-Hibbert requested review from mhofman and removed request for mhofman and AgoricTriage December 12, 2024 00:18

mhofman reviewed Dec 12, 2024

View reviewed changes

warner reviewed Dec 12, 2024

View reviewed changes

Chris-Hibbert requested review from dckc and turadg December 12, 2024 00:59

warner reviewed Dec 12, 2024

View reviewed changes

turadg reviewed Dec 12, 2024

View reviewed changes

turadg approved these changes Dec 12, 2024

View reviewed changes

turadg reviewed Dec 13, 2024

View reviewed changes

Chris-Hibbert force-pushed the 10669-saveInstances branch from 06e77b4 to c6b5308 Compare December 13, 2024 04:52

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Dec 13, 2024

Chris-Hibbert commented Dec 13, 2024

View reviewed changes

Chris-Hibbert force-pushed the 10669-saveInstances branch 2 times, most recently from a1ad58f to 616fdfe Compare December 13, 2024 06:30

dckc reviewed Dec 13, 2024

View reviewed changes

Chris-Hibbert and others added 2 commits December 13, 2024 15:53

feat: record instances that will be replaced so we can manage them

c883c39

refactor: provideRetiredInstances

1581127

Chris-Hibbert force-pushed the 10669-saveInstances branch from 616fdfe to 1581127 Compare December 13, 2024 15:53

mergify bot merged commit d35659b into master Dec 13, 2024
81 checks passed

mergify bot deleted the 10669-saveInstances branch December 13, 2024 16:44

Chris-Hibbert mentioned this pull request Dec 14, 2024

Save the contractKit for the auctioneer before overwriting it #10694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record instances that will be replaced so we can manage them #10680

Record instances that will be replaced so we can manage them #10680

Chris-Hibbert commented Dec 11, 2024

cloudflare-workers-and-pages bot commented Dec 11, 2024 •

edited

Loading

mhofman left a comment

warner Dec 12, 2024

Chris-Hibbert Dec 12, 2024

Chris-Hibbert Dec 12, 2024

warner Dec 12, 2024

Chris-Hibbert Dec 12, 2024

Chris-Hibbert commented Dec 12, 2024

warner Dec 12, 2024

Chris-Hibbert Dec 13, 2024

warner Dec 12, 2024

Chris-Hibbert Dec 12, 2024

Chris-Hibbert Dec 12, 2024

warner Dec 12, 2024

Chris-Hibbert Dec 12, 2024

warner Dec 12, 2024

Chris-Hibbert Dec 12, 2024

turadg Dec 12, 2024

dckc Dec 12, 2024

turadg Dec 12, 2024

turadg Dec 12, 2024

Chris-Hibbert Dec 12, 2024

turadg Dec 12, 2024

turadg Dec 13, 2024

Chris-Hibbert Dec 13, 2024

dckc Dec 13, 2024

mergify bot commented Dec 13, 2024

dckc left a comment

		// I don't know why it's neither in governedContractKits nor contractKits
		// assert(await E(governedContractKits).get(auctionInstance));

Record instances that will be replaced so we can manage them #10680

Record instances that will be replaced so we can manage them #10680

Conversation

Chris-Hibbert commented Dec 11, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 11, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 13, 2024

dckc left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Dec 11, 2024 •

edited

Loading