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

liveSlots consensus mode and better verbosity switches #4364

Merged
merged 8 commits into from
Jan 25, 2022

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jan 24, 2022

refs: #2519

Description

This properly routes liveSlots output to a disabled console if in consensus mode. Also, updates the use of $DEBUG to be more coherent and not as bizarre (where an empty $DEBUG would log less than a missing $DEBUG).

Also improve a longstanding wart where XS console logging displayed quote's hodgepodge of JSON and deeper inspection, just by replacing the whole thing with an NPM package based on Node.js's util.inspect rendering.

At last, all of agoric start, agoric start -v, and agoric start -vv look right, in order of increasing verbosity. Notably, when running an actual chain, no vat-level errors are displayed.

Security Considerations

Documentation Considerations

Testing Considerations

@michaelfig michaelfig added SwingSet package: SwingSet agoric-cli package: agoric-cli labels Jan 24, 2022
@michaelfig michaelfig self-assigned this Jan 24, 2022
@michaelfig michaelfig changed the title Mfig liveslots consensus mode liveSlots consensus mode and better verbosity switches Jan 24, 2022
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I looked at all the code and I don't see any problems, but I don't clearly understand the motivation and I'm not confident about the non-determinism guarantees, so I defer to @warner .

@michaelfig michaelfig force-pushed the mfig-liveslots-consensus-mode branch 7 times, most recently from 8e69eec to 75e3982 Compare January 25, 2022 01:27
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, that looks good. I would have loved to see a unit test that shows the console being disabled when consensusMode is true, but I certainly couldn't whip one up without a couple of hours, so I can't expect anyone else to.

r+

globalThis.print = portSendingPrinter;
printAll(...args);
} finally {
globalThis.print = savePrinter;
Copy link
Member

Choose a reason for hiding this comment

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

ok, when printAll() causes some random object's toString to be invoked and that does more console.log-ing, this structure should unwind the reentrancy properly, nice

@michaelfig michaelfig force-pushed the mfig-liveslots-consensus-mode branch 3 times, most recently from 6be9b40 to 3411f24 Compare January 25, 2022 05:12
@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jan 25, 2022
@mergify mergify bot merged commit 80f0076 into master Jan 25, 2022
@mergify mergify bot deleted the mfig-liveslots-consensus-mode branch January 25, 2022 05:44
Comment on lines +12 to +13
# copied from upstream
/packages/xsnap/lib/object-inspect.js
Copy link
Member

Choose a reason for hiding this comment

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

@michaelfig why is this copied from upstream instead of sourced from the object-inspect dependency in package.json?

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 could have commented that better. It's not a verbatim copy, it's the object-inspect.js with an unnecessary CommonJS require('./util.inspect') removed since it doesn't work under XS.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. What's the reason to prevent prettying it? If it's to make it easier to update from upstream, WDYT of using patch and to make it work under XS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. What's the reason to prevent prettying it? If it's to make it easier to update from upstream, WDYT of using patch and to make it work under XS?

That's fine with me, if you're bored and want something to do. 😄 Tests cover it, so it's a safe thing to try.

Copy link
Member Author

Choose a reason for hiding this comment

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

patch-package can't be used, though, because people might get this code directly via NPM's @agoric/xsnap.

@dckc
Copy link
Member

dckc commented Feb 8, 2022

I notice now, after the fact, that object-inspect.js is a significant chunk of code that doesn't follow our style. And it goes right at the bottom of our TCB. I'm surprised it didn't get a mention in Security Considerations like other PRs where I've seen us take on a new dependency. @warner were you conscious of this in your review?

And I see logging tests in test-boot-lockdown.js changed. While the overall issue of console API compatibility for XS (#2146) remains open, we (@kriskowal @erights and I) did decide to use assert.quote from SES in #3856 and I am just now realizing this PR means reconsidering this decision. I better double-check that they're OK with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cli package: agoric-cli automerge:no-update (expert!) Automatically merge without updates SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants