-
Notifications
You must be signed in to change notification settings - Fork 215
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(swingset-tools): Expand replay tool for anachrophobia diagnosis #6723
feat(swingset-tools): Expand replay tool for anachrophobia diagnosis #6723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modulo those two questions, this looks good, just make sure it doesn't break local replay on the non-testing path (if we temporarily break debugging replay for local workers, that's not a big deal)
I'm sure you do, but please make sure you've got a plan in mind for getting this landed on trunk sooner or later.
}) | ||
); | ||
|
||
const snapStore = argv.useCustomSnapStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep an eye on #6755, it might change the snapStore API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make sure it doesn't break local replay on the non-testing path (if we temporarily break debugging replay for local workers, that's not a big deal)
I know xs-worker
definitely works for normal execution as I've been running multiple following nodes including this change and the ones in #6724 on mainnet. I don't believe we have any local
manager type in production use. Also I don't think this PR changes anything in the production paths besides types, adding a couple exports, and providing an extra argument to compareSyscalls
.
I definitely want to merge this in trunk as well once this is approved, but we still need to be ok with the mitigation in #6664 landing as well. It's possible to merge the tooling changes without the replay behavioral change, but we'd lose a lot of the functionality. |
c112b39
to
cafe0e8
Compare
a163f96
to
c068619
Compare
Store VC metadata syscalls results for handling missing from transcript Options to configure syscall mismatch
With different load points
Add keep worker at specific delivery nums
…orkers Fix concurrent snapshot save for custom snapStore
c068619
to
9a9ea0b
Compare
refs: #6588
This PR is targeting
release-pismo
as this is where I've done most of the work, but it should be safe to merge into master, which I plan on doing after approval.I spent a lot of time cleaning up the changes to make the implementation cleaner and the individual commits relevant. At this point it's almost a rewrite of the
replay-transcript.js
tool, so reviewing commit-by-commit may be easier but not necessary.Description
In the past month I've massively expanded the capabilities of the replay tool to support my investigation of the anachrophobia issue experienced by validators on mainnet (#6588).
This PR adds the following features:
const
based config to command line options, including ability to load options from a config fileSecurity Considerations
None, this is a debug tooling.
Documentation Considerations
The new command line options could be better documented (with CLI based documentation), but I didn't feel like dealing with the quirks of the full
yargs
at this point.Testing Considerations
Main features were used extensively over the last month. Refactors/rebase to clean up was quickly tested again.