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

feat(xsnap)!: Update Moddable SDK and xsnap-native #6920

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Feb 4, 2023

closes: #6759
supersedes: #6011 #6768

Description

Updates our old fork of the Moddable SDK to the official 3.7.0 version

Updates xsnap-native with the following changes (details at agoric-labs/xsnap-pub#34):

  • handle profiler and instrumentation changes (disabled)
  • updated promise queue behavior

Security Considerations

This update contains a lot of bug fixes in XS found by fuzzing tools. This should be a strict improvement.

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Followed the following process (now described in #6929)

  • Created a "chain transcript" from mainnet using a follower based on the pismoC version of XS (minus agoric-sdk version bump), equivalent to mhofman/pismo-with-replay-tools-improvements-updated-xs~1
  • Replayed "chain transcript" using updated version of xsnap-worker (slightly different revision actually tested, but no effective changes to xsnap-worker), equivalent to branch mhofman/pismo-with-replay-tools-improvements-updated-xs
  • Extracted vat transcripts
  • Replayed vat transcripts by forcing a reload into a new worker after each snapshot being taken, and comparing the execution and snapshot hashes with workers that didn't reload.
  • Observed no anachrophobia nor any snapshot hash differences indicating that no reload based divergences were experience on the mainnet derived data set

(PR #6931 for chain transcript replay tool against master)

@mhofman
Copy link
Member Author

mhofman commented Feb 7, 2023

Apparently something broke, and I'm surprised the testing I didn't uncover these issues.

@warner
Copy link
Member

warner commented Feb 7, 2023

I skimmed the xsnap changes and they look great. I couldn't look at the xs changes.. as usual we rely upon upstream's diligence and your thorough testing.

Not sure what's going on with the test (slogSender something?), but assuming it's something trivial, I think the PR is good to go once it works again.

(hm, I did notice some more file descriptors referenced in the xsnap code.. but I think that only happens if you ask for instrumentation/debugging to be turned on. There's a remote chance that the extra FDs used for the slogsender pipe are getting inherited by the xsnap child process and colliding with something it wants to use for debugging. That would qualify as a non-trivial problem :)

@mhofman
Copy link
Member Author

mhofman commented Feb 7, 2023

Not sure what's going on with the test (slogSender something?), but assuming it's something trivial, I think the PR is good to go once it works again.

(hm, I did notice some more file descriptors referenced in the xsnap code.. but I think that only happens if you ask for instrumentation/debugging to be turned on. There's a remote chance that the extra FDs used for the slogsender pipe are getting inherited by the xsnap child process and colliding with something it wants to use for debugging. That would qualify as a non-trivial problem :)

Oh that's an excellent idea. When I ran my tests I hadn't had time to fully understand the instrumentation changes, so I had ommitted that commit entirely. As described at agoric-labs/xsnap-pub#34, I didn't feel fully confident in them so I disabled them using the build option, thinking the new select would result in a no-op because we're not using these FDs, but maybe that isn't the case. I'll do more testing.

Moddable SDK 3.7.0
xsnap-native:
- handle profiler and instrumentation changes (disabled)
- updated promise queue behavior
@mhofman mhofman force-pushed the mhofman/6759-update-xs branch from 2e5a120 to 4f7703b Compare February 7, 2023 23:47
@mhofman
Copy link
Member Author

mhofman commented Feb 7, 2023

I figured it out: faulty disablement of the new intrument logic, and that code hangs if the fd doesn't exist. More info at agoric-labs/xsnap-pub#34 (comment)

@mhofman
Copy link
Member Author

mhofman commented Feb 8, 2023

@warner, all checks are green, PTAL

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.

recent changes to xsnap/ look good, I think we're go

@mergify mergify bot merged commit ddb745b into master Feb 8, 2023
@mergify mergify bot deleted the mhofman/6759-update-xs branch February 8, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronize with version 3.6.0 or newer of Moddable SDK
2 participants