add globalThis.VatData.passStyleOf to liveslots-hosted vat environment #9431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
warner
added
SwingSet
Deploying agoric-sdk with
|
| Latest commit: |
9f0d4b8
|
| Status: | 🚫 Build failed. |
erights
requested changes
May 31, 2024
There was a problem hiding this comment.
This is added as a string-named property, rather than a symbol-named one, which might not be the current plan.
Yeah, this is not the current plan, which is part of why #8051 remained on ice. See
https://github.com/endojs/endo/blob/10edfa0e63bff509e85084cbfbc5f7b255f24469/packages/pass-style/src/passStyleOf.js#L198-L216
export const PassStyleOfEndowmentSymbol = Symbol.for('@endo passStyleOf');
/**
* If there is already a PassStyleOfEndowmentSymbol property on the global,
* then presumably it was endowed for us by liveslots with a `passStyleOf`
* function, so we should use and export that one instead.
* Other software may have left it for us here,
* but it would require write access to our global, or the ability to
* provide endowments to our global, both of which seems adequate as a test of
* whether it is authorized to serve the same role as liveslots.
*
* NOTE HAZARD: This use by liveslots does rely on `passStyleOf` being
* deterministic. If it is not, then in a liveslot-like virtualized
* environment, it can be used to detect GC.
*
* @type {PassStyleOf}
*/
export const passStyleOf =
(globalThis && globalThis[PassStyleOfEndowmentSymbol]) ||Note that it looks up a symbol-named property on the global itself, not on globalThis.VatData. My original design as of #8051 had @endo/pass-style look for it in VatData. But this was rightly objected to as too weird a layering violation, so we moved it into a symbol-named property directly on the global.
Please add a test showing that @endo/pass-style is reexporting this endowment from liveslots, rather than creating its own. That test should fail if liveslots exports the endowment via VatData, or as a string-named property.
|
I believe this will require a change to |
Update the swingset test to look for it there, and compare against a bundle-generated version (they should be different). This currently fails because importBundle() does not respect symbol-named properties of `vatGlobals`. Restored the SwingSet/package.json dependency on `@endo/pass-style` so the vat-under-test can get a bundle-generated version, for comparison. Added a liveslots-local test of the same, which passes because it's only looking at the `vatGlobals` given to `buildVatNamespace()`, and doesn't use `importBundle()`.
|
I pushed a commit which changes liveslots to use the symbol-named property, and update the tests. The swingset test (which uses Since |
What we've done before in these cases is to start with an endo PR. Once approved and merged, either wait for the next version of endo to be released and picked up, or if "urgent", apply the endo change as a patch as you mention. I do recommend that during the endo PR process, the solution is verified through a patch or simulated upgrade of endo in agoric-sdk ( |
erights
reviewed
Jun 3, 2024
| }); | ||
| [PassStyleOfEndowmentSymbol]: passStyleOf, | ||
| }; | ||
| harden(vatGlobals); |
There was a problem hiding this comment.
No objection to doing the harden here. But curious about your motivation for moving it? There's no mutation between the initialization and the harden, so the more traditional harden around the object literal should still be equivalent.
There was a problem hiding this comment.
I don't remember why I moved it.. I'll move it back.
|
@warner I assigned this to you. Is this right? |
mergify bot
pushed a commit
that referenced
this pull request
Aug 17, 2024
…tment (#9874) A variant of #9431 . @warner , feel free to just adopt these changes into #9431 rather than reviewing this alternate. closes: #9781 refs: #9431 endojs/endo#2377 endojs/endo#2408 ## Description The code running under liveslots, i.e., user-level vat code such as contracts, must not be able to sense gc. Thus, liveslots endows them with virtual-storage-aware WeakMap and WeakSet, which treats the virtual object as the weakly held key, whereas the builtin WeakMap and WeakSet would treat the momentary representative as the weakly held key. To achieve this, the virtual-storage-aware WeakMap and WeakSet must impose a comparative storage leak. However, some WeakMaps and WeakSets are used purely as an encapsulated unobservable memo, in the sense that the clients of encapsulating abstraction cannot sense whether the memo hit or missed (modulo timing of course, which we can also deny). `passStyleOf` is such an abstraction. Measurements show that the storage leak it causes is significant. The judgement of `passStyleOf` is only to report the pass-style of its arguments, and all virtual objects that have representative have a pass-style of `'remotable'` every time any of its representatives are tested. To avoid this storage leak, endojs/endo#2377 (merged, released, and synced with agoric-sdk) and endojs/endo#2408 (still in review) together enable liveslots to endow the compartment it unbundles with its own efficient `passStyleOf`, built from the primitive WeakMap which it encapsulates. This PR does two things: - makes the change to liveslots to do this endowing, according to the conventions supported by endojs/endo#2377 and endojs/endo#2408 - because endojs/endo#2408 is not yet synced with agoric-sdk, this PR adds an "equivalent" patch, so that we can depend on it before the next endo sync. ### Security Considerations This design *assumes* that the endowed `passStyleOf` makes the memo hits vs misses unobservable, so its dependence on these does not enable the code using it to observe gc. If there is some way to trick it into exposing the difference between a hit and miss, that would be a security concern. ### Scaling Considerations The point. With this PR, the storage leak caused by the `passStyleOf` memo should go away. For some vats, this should be a big improvement. ### Documentation Considerations For the normal developer, none. ### Testing Considerations Adapts the tests originally written by @warner in #9431 , which seem to demonstrate that this works both for node-based and for XS-based vats. ### Upgrade Considerations I don't believe there are any. When linked with an endo preceding even endojs/endo#2377 , the only consequence should be that the storage leak remains unfixed. Likewise, if an endo with endojs/endo#2377 and even endojs/endo#2408 is linked with an agoric-sdk prior to the PR, the only consequence should be that the storage leak remains unfixed.
erights
added a commit
to endojs/endo
that referenced
this pull request
Aug 22, 2024
…2408) Closes: #2407 Refs: Agoric/agoric-sdk#9874 Agoric/agoric-sdk#9431 Agoric/agoric-sdk#9781 #2377 ## Description While working on Agoric/agoric-sdk#9874 , I found that it still failed to endow `passStyleOf` via a symbol-named property as recently agree. Turns out there was still a lurking `Object.keys` on the compartment endowment pathway that needs to be changed to enumerate all enumerable own properties, whether string-named or symbol-named. ### Security Considerations None ### Scaling Considerations For this PR, none. But it enables liveslots to endow its `passStyleOf` to the compartments it makes, which has the scaling benefits explained at Agoric/agoric-sdk#9781 ### Documentation Considerations For the normal developer, none. Once endowed, `passStyleOf` will have performance that is less surprising. ### Testing Considerations Agoric/agoric-sdk#9874 both patches in the equivalent of this PR, uses it to endow `passStyleOf`, and test that it has done so, across both node and XS. However, after this fix, Agoric/agoric-sdk#9874 works on the Node host. But it still fails on the XS host for undiagnosed reasons, so there may be another bug like #2407 still lurking somewhere. Until diagnosed, it isn't clear if that remaining problem is an endo problem or an agoric-sdk problem. ### Compatibility and Upgrade Considerations Except for code meant only to test this, `passStyleOf` should not have any observable difference. We have not attempted to make an other use of symbol-named global properties, so there should be no compat or upgrade issues.
kriskowal
pushed a commit
that referenced
this pull request
Aug 27, 2024
…tment (#9874) A variant of #9431 . @warner , feel free to just adopt these changes into #9431 rather than reviewing this alternate. closes: #9781 refs: #9431 endojs/endo#2377 endojs/endo#2408 ## Description The code running under liveslots, i.e., user-level vat code such as contracts, must not be able to sense gc. Thus, liveslots endows them with virtual-storage-aware WeakMap and WeakSet, which treats the virtual object as the weakly held key, whereas the builtin WeakMap and WeakSet would treat the momentary representative as the weakly held key. To achieve this, the virtual-storage-aware WeakMap and WeakSet must impose a comparative storage leak. However, some WeakMaps and WeakSets are used purely as an encapsulated unobservable memo, in the sense that the clients of encapsulating abstraction cannot sense whether the memo hit or missed (modulo timing of course, which we can also deny). `passStyleOf` is such an abstraction. Measurements show that the storage leak it causes is significant. The judgement of `passStyleOf` is only to report the pass-style of its arguments, and all virtual objects that have representative have a pass-style of `'remotable'` every time any of its representatives are tested. To avoid this storage leak, endojs/endo#2377 (merged, released, and synced with agoric-sdk) and endojs/endo#2408 (still in review) together enable liveslots to endow the compartment it unbundles with its own efficient `passStyleOf`, built from the primitive WeakMap which it encapsulates. This PR does two things: - makes the change to liveslots to do this endowing, according to the conventions supported by endojs/endo#2377 and endojs/endo#2408 - because endojs/endo#2408 is not yet synced with agoric-sdk, this PR adds an "equivalent" patch, so that we can depend on it before the next endo sync. ### Security Considerations This design *assumes* that the endowed `passStyleOf` makes the memo hits vs misses unobservable, so its dependence on these does not enable the code using it to observe gc. If there is some way to trick it into exposing the difference between a hit and miss, that would be a security concern. ### Scaling Considerations The point. With this PR, the storage leak caused by the `passStyleOf` memo should go away. For some vats, this should be a big improvement. ### Documentation Considerations For the normal developer, none. ### Testing Considerations Adapts the tests originally written by @warner in #9431 , which seem to demonstrate that this works both for node-based and for XS-based vats. ### Upgrade Considerations I don't believe there are any. When linked with an endo preceding even endojs/endo#2377 , the only consequence should be that the storage leak remains unfixed. Likewise, if an endo with endojs/endo#2377 and even endojs/endo#2408 is linked with an agoric-sdk prior to the PR, the only consequence should be that the storage leak remains unfixed.
|
yep closing, subsumed by #9874 , which landed a few weeks ago |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently staged on #8051
Change liveslots to add
passStyleOftoglobalThis.VatDatain the environment provided to real vats.This is added as a string-named property, rather than a symbol-named one, which might not be the current plan.