Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

have the kernel abandon non-durable vat exports during upgrade, not stopVat #6696

Closed
warner opened this issue Dec 19, 2022 · 1 comment · Fixed by #7170
Closed

have the kernel abandon non-durable vat exports during upgrade, not stopVat #6696

warner opened this issue Dec 19, 2022 · 1 comment · Fixed by #7170
Assignees
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE
Milestone

Comments

@warner
Copy link
Member

warner commented Dec 19, 2022

What is the Problem Being Solved?

As listed in #6650, during vat upgrade, one of the last remaining responsiblities of the old vat's stopVat call has been to abandon all non-durable exports. We need to abandon these here, because clients may still have imports that refer to them (Presences), but the implementation of these objects will not be available after the upgrade (not being durable). By calling syscall.abandonExport() on the vrefs, the old vat is telling the kernel to treat those objects as unowned by any vat. They retain their identity, and refcounts will keep the kref alive as usual, but any attempt to send them a message will result in an error being generated by the kernel (aka "message goes splat"), and the objects will be removed from the old exporting vat's c-list.

The task is to move this responsibility into the kernel. During processUpgradeVat, the kernel will enumerate the upgraded vat's c-list for all non-durable exports (which, after #6695, will be the set of kvStore keys from ${vatID}.o+/ through ${vatID}.o+e edit: durables are o+d.., so I think non-durables are o+/-o+:, plus o+v-o+v:). For each one, it will do the same thing as syscall.abandonExports (currently implemented in kernelSyscall.js): a kernelKeeper.orphanKernelObject(koid, vatID). It must also delete the c-list entry (currently performed in vatTranslator.js in the translateAbandonExports() function).

Simultaneously, we should remove the abandonExports() call from src/liveslots/stop-vat.js.

Description of the Design

Security Considerations

Test Plan

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Dec 19, 2022
@warner warner self-assigned this Dec 19, 2022
@ivanlei ivanlei added the vaults_triage DO NOT USE label Dec 20, 2022
@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
@warner
Copy link
Member Author

warner commented Feb 15, 2023

I have a partial branch for this in https://github.com/Agoric/agoric-sdk/tree/6696-kernel-abandons-nondurables . It lays the groundwork, but is not complete. The added comments (both in the code, and the commit comments) enumerate the remaining work. The trickiest part code part is to refactor kernel.js and vat-loader to make the VatSyscallHandler available to kernel.js so that processUpgradeVat can use it to execute the synthesized abandonExports syscalls.

The trickiest conceptual part is whether we need to care about the difference between reachable and recognizable exports. It's possible that we can abandon either form identically, and the kernel-side GC code will take care of both. I don't remember if we have a test for this. The test would prepare one export that is held strongly by a downstream vat (so the export-side c-list entry is marked as "R" for Reachable), then the upstream vat does a syscall.abandonExports on it, and the test asserts that the c-list entry is deleted. The test must also prepare an export that is held weakly by a downstream vat (so the c-list entry is marked as _ for merely-Recognizable), and assert that syscall.abandonExports deletes the c-list entry, and also delivers a dispatch.retireExports to the downstream vat. Look at https://github.com/Agoric/agoric-sdk/blob/f6996583c94e4b1d61160a102f1e92f81cc27bb0/packages/SwingSet/test/test-abandon-export.js .

gibson042 pushed a commit that referenced this issue Mar 4, 2023
This makes room inside processUpgradeVat() for the new cleanup steps
that we want to add.

refs #6650
refs #7001
refs #6694
refs #6696
@warner warner removed their assignment Mar 14, 2023
gibson042 pushed a commit that referenced this issue Mar 16, 2023
This makes room inside processUpgradeVat() for the new cleanup steps
that we want to add.

refs #6650
refs #7001
refs #6694
refs #6696
@mergify mergify bot closed this as completed in #7170 Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants