fix(vm-config): `vats/init-core.js` relies on econCommittee #11056

michaelfig · 2025-02-26T17:08:34Z

Description

A catch-22 was introduced when vats/init-core.js and inter-protocol/init-core.js were separated into different coreProposal steps:

startWalletFactory would not advance to the next step until provisionPool was instantiated with governance, but
the governance needed by provisionPool was established in a later step.

Moving them to the same step was the expedient approach taken by #11058, but breaking the deadlock by decoupling the completion of startWalletFactory from the provisionPool governance seems cleaner, and less dependent upon subtle @agoric/vm-config steps.

Breaking the deadlock was crucial for solving #11053 , so that smart wallets can be properly provisioned even if inter-protocol governance occurs in a later step than vats/init-core.js.

Also in this PR:

when producer.resolve(x) is called, immediately nameHubAdmin.reserve the key, even if x settles much later
don't call onReset unless producer.reset() actually modified something
add the key name to the rejection reason propagated to a nameHub's consumer caused by resetting a promise-space key.

Security Considerations

n/a

Scaling Considerations

No additional resource consumption.

Documentation Considerations

n/a

Testing Considerations

Manually tested as specified in #11053 , but it would be good to test in CI if it can be done without significantly affecting CI duration.

To test this in a testnet, just boot it from genesis, and verify that smart wallets can be successfully provisioned via the faucet (don't forget to send some funds to the provisionPool or uist to the faucet account!).

Upgrade Considerations

The bootstrap vat JS code is not upgradable for existing chains, so much of the changes in this PR will not affect those chains. Besides that, the agoricNames vat and provisioning vat host nameHubs, so the Value for ${key} has been deleted diagnostic will continue to lack the ${key} until they are upgraded.

Future chains booted from genesis will benefit the most from these changes.

cloudflare-workers-and-pages · 2025-02-26T17:10:58Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3dbddef`
Status:	✅ Deploy successful!
Preview URL:	https://e1191434.agoric-sdk.pages.dev
Branch Preview URL:	https://mfig-devnet-config-steps.agoric-sdk.pages.dev

View logs

dckc

I think some unit tests are worthwhile for the fixes.

And I think there's more to say about Upgrade Considerations.

dckc · 2025-02-27T06:05:44Z

packages/vats/src/core/promise-space.js

@@ -50,10 +50,10 @@ export const makeStoreHooks = (store, log = noop) => {
      return;
    }
    if (store.has(name)) {
-      console.warn('cannot save duplicate:', name);
-      return;
+      store.set(name, value);


how would this change get to mainnet?

"n/a" doesn't seem to suffice for upgrade considerations.

How about adding a test in test-promise-space.js for this fix?

"How do we validate this change in a testnet?" will eventually be asked. If the answer is "it's not cost-effective" then that should go in Testing Considerations.

dckc · 2025-02-27T06:07:31Z

packages/vats/src/core/promise-space.js

+      if (!nameToState.has(name)) {
+        return;
+      }


How about adding a test in test-promise-space.js for this fix too?

packages/vats/src/core/startWalletFactory.js

dckc · 2025-02-27T06:19:05Z

packages/vats/src/core/utils.js

+            if (present) {
+              void E.sendOnly(nameAdmin).delete(name);


seems worth a unit test in test-name-hub-published.js

but again... how do we deploy this change to mainnet?

Maybe the answer is "we don't" but that merits something explicit in upgrade considerations.

This change is outdated since I simplified the utils.js change to just one line.

packages/vats/src/nameHub.js

refs: #11053 #11056 ## Description This PR incorporates the original #11053 fix as installed on agoricdev-25 of [the Agoric devnet](https://devnet.agoric.net). #11056 contains a cleaner fix to be merged with master. ### Security Considerations n/a ### Scaling Considerations Bootstrap only: should not affect resource consumption. ### Documentation Considerations n/a ### Testing Considerations Tested manually and on Agoric devnet. ### Upgrade Considerations Will only affect new devnet chains and followers.

michaelfig · 2025-02-28T04:49:32Z

@dckc, I still owe you a few little tests, but now that I've simplified the changes and adequately written up the PR description, I'd appreciate your rereview so I can address all the comments at once during the next push.

dckc · 2025-02-28T07:36:42Z

packages/vats/src/core/utils.js

@@ -221,6 +221,7 @@ export const makePromiseSpaceForNameHub = (nameAdmin, log = noop) => {
        logHooks.onAddKey(name);
      },
      onResolve: (name, valueP) => {
+        void E(nameAdmin).reserve(name);


interesting solution

dckc

upgrade considerations and such look good

I approve presuming the unit tests as discussed.

michaelfig requested a review from a team as a code owner February 26, 2025 17:08

michaelfig requested a review from AgoricTriage February 26, 2025 17:08

michaelfig force-pushed the mfig-devnet-config-steps branch from 8e33f9c to 4c363ab Compare February 26, 2025 17:56

michaelfig changed the title ~~fix(vm-config): init-core.js relies on inter-protocol~~ fix(vm-config): vats/init-core.js relies on econCommittee Feb 26, 2025

michaelfig requested a review from dckc February 26, 2025 18:38

michaelfig mentioned this pull request Feb 26, 2025

fix(vm-config): incorporate devnet config from agoricdev-25 #11058

Merged

michaelfig self-assigned this Feb 26, 2025

michaelfig force-pushed the mfig-devnet-config-steps branch from 4c363ab to ef30b2a Compare February 27, 2025 04:26

michaelfig mentioned this pull request Feb 27, 2025

agd throws panic: counter cannot decrease in value #11062

Open

michaelfig force-pushed the mfig-devnet-config-steps branch from ef30b2a to 272bd32 Compare February 27, 2025 05:48

dckc reviewed Feb 27, 2025

View reviewed changes

michaelfig force-pushed the mfig-devnet-config-steps branch 2 times, most recently from e7b8cf6 to cab5376 Compare February 28, 2025 03:08

michaelfig added 3 commits February 27, 2025 21:21

fix(vats): avoid needless mutation on producer.reset()

c4f64f8

fix(vats): on resolve, immediately reserve PromiseSpaceForNameHub

3a320a6

feat(vats): decouple walletFactory from inter-protocol

3dbddef

michaelfig force-pushed the mfig-devnet-config-steps branch from cab5376 to 3dbddef Compare February 28, 2025 03:23

michaelfig requested a review from dckc February 28, 2025 04:49

dckc reviewed Feb 28, 2025

View reviewed changes

dckc approved these changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(vm-config): `vats/init-core.js` relies on econCommittee #11056

fix(vm-config): `vats/init-core.js` relies on econCommittee #11056

michaelfig commented Feb 26, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 26, 2025 •

edited

Loading

dckc left a comment

dckc Feb 27, 2025

dckc Feb 27, 2025

dckc Feb 27, 2025

dckc Feb 27, 2025

dckc Feb 27, 2025

dckc Feb 27, 2025

michaelfig Feb 28, 2025

michaelfig commented Feb 28, 2025

dckc Feb 28, 2025

dckc left a comment

fix(vm-config): vats/init-core.js relies on econCommittee #11056

Are you sure you want to change the base?

fix(vm-config): vats/init-core.js relies on econCommittee #11056

Conversation

michaelfig commented Feb 26, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Feb 26, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Feb 28, 2025

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

fix(vm-config): `vats/init-core.js` relies on econCommittee #11056

fix(vm-config): `vats/init-core.js` relies on econCommittee #11056

michaelfig commented Feb 26, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 26, 2025 •

edited

Loading