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

The beginnings of SwingSet consensus mode #3450

Merged
merged 6 commits into from
Jul 14, 2021
Merged

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jul 3, 2021

Refs #2519

This PR plumbs through a consensusMode kernel option that, when true, tries to eliminate observable nondeterminism from the perspective of user code in vats. For this first cut, that means that console logging is stubbed out as close to the vat code as possible.

Consensus mode is the default when running ag-chain-cosmos without setting $DEBUG. It is a dynamic setting, so to get debugging information, you can take an existing initdir and run DEBUG=agoric ag-chain-cosmos start to reenable debug output. Note that this may result in writing nondeterministic effects to disk, so be sure to take a copy of the initdir so that you can continue the original in consensus mode.

To facilitate debugging, ag-solo does not use consensus mode, nor does its embedded sim-chain (which is also solo, not quorum nor genuine chain).

@michaelfig michaelfig added SwingSet package: SwingSet cosmic-swingset package: cosmic-swingset labels Jul 3, 2021
@michaelfig michaelfig self-assigned this Jul 3, 2021
@dckc
Copy link
Member

dckc commented Jul 6, 2021

add "refs #2519"?

@michaelfig michaelfig requested a review from warner July 8, 2021 04:43
@@ -85,6 +85,7 @@ export function makeVatLoader(stuff) {
}

const allowedDynamicOptions = [
'consensusMode',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We certainly want to be able to control this mode for replay purposes, but I don't think it should be controllable through the vatAdminVat facet. A vat that is under consensus mode should not be able to acquire the power to break consensus by virtue of creating a new vat that is not. So I kind of think this shouldn't be set on a vat-per-vat basis. It should exist in managerOptions, but not be permitted in allowedDynamicOptions/allowedStaticOptions.

I'm not sure what to recommend exactly. All settings that could affect the consensus-visible activity of the system ought to be "sticky" (provided to initializeKernel and stored in the DB somehow) so that the kernel/vat's behavior will be consistent from one run to the next. But this control is obviously useful to override during a specialized replay action, where you know you're breaking that state vector for the benefit of debugging something. The verb to override it should maybe be distinct from the control used to set it initially.

(we need to reorganize/rethink the large variety of options we have, and the places that can provide them.. adding an option requires touching far too many files, among other problems)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We certainly want to be able to control this mode for replay purposes, but I don't think it should be controllable through the vatAdminVat facet.

Ah, I overlooked that. Thanks for pointing it out.

But this control is obviously useful to override during a specialized replay action, where you know you're breaking that state vector for the benefit of debugging something.

I think that is a clear description of what the $DEBUG environment variable is for.

@@ -58,6 +59,8 @@ export const makeLRU = max => {
*/
export function makeVatWarehouse(kernelKeeper, vatLoader, policyOptions) {
const {
// Whether to eliminate all vat nodeterminism.
consensusMode = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warehousePolicy was kind of meant to define the RAM-vs-time tradeoffs of the warehouse itself, rather than to be a way to control the execution of vats.. it feels like consensusMode wants to be part of some other options object (which the VatWarehouse may need to be aware of, to instruct VatManagers how to work). I'm not sure I have a better recommendation, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new kernel runtime option called overrideVatManagerOptions, which does as advertised. consensusMode now lives in there, instead of in the VatWarehouse.

'function',
X`Invalid VatConsole wrapper value ${wrapper}`,
);
return [level, (...args) => wrapper(backingLog, args)];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels funny here, why have a three-way switch here (always-enabled, never-enabled, switchable) when supervisor-subprocess-xsnap.js also has a switch? This feels like maybe we could omit the switching code here and have the supervisors manage the disabling instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The identity of the console is permanently fixed in the case of snapshots, so we need this switching code to be in the same place as where the console is first defined. I could just always enable the switching code and not use the always/never modes (which you maybe consider to be premature optimisation).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think I prefer the simplicity of having one mode, over the nominal performance improvement of sometimes omitting the switch

@michaelfig michaelfig force-pushed the mfig-consensus-mode branch from 1ec304d to 3943498 Compare July 12, 2021 16:22
@michaelfig michaelfig requested a review from warner July 12, 2021 16:43
@michaelfig michaelfig force-pushed the mfig-consensus-mode branch from 3943498 to 8d5d0ff Compare July 12, 2021 16:59
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok,those fixes look good. If it's easy, please simplify the trio of options in supervisor-helper.js makeVatConsole() into one (recognizing that we don't need consensus mode on non-xsnap workers), and then feel free to land.

@michaelfig michaelfig force-pushed the mfig-consensus-mode branch from 8d5d0ff to dc0839b Compare July 14, 2021 19:29
@michaelfig michaelfig enabled auto-merge July 14, 2021 19:29
@michaelfig michaelfig merged commit ae5330a into master Jul 14, 2021
@michaelfig michaelfig deleted the mfig-consensus-mode branch July 14, 2021 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants