Skip to content

Conversation

netrome
Copy link
Collaborator

@netrome netrome commented Oct 5, 2025

Closes #1224

This PR solves the problem of the attestation generating logic happening outside of a tokio runtime by creating the root runtime first thing in StartCmd::run and calling .enter() to enter the runtime context and keep it for the remainder of the run() function.

Forgive me, team, for I have sinned. I have opened this PR without adding test coverage of the change. While the change is trivial and should be easy to review, I did not find any easy way to test this. And since this bug currently breaks the node, it would be nice to get the fix through before refactoring the code to make this testable.

I have created a follow-up (#1227) to ensure this part of the code is easier to test, but I fear that will be a significant effort to refactor and we might need to push it until after the soft launch. Unless we're hitting many more bugs during E2E testing at which point we can reconsider prioritizing this refactor.

Also this bug is a deja-vu. We fixed a similar issue in #612 but for some reason, the runtime guard introduced there is not around anymore. This also shows why it's important to have tests to guard against regressions. @DSharifi - was there any reason to kill the runtime guard that you recall?

@netrome netrome linked an issue Oct 5, 2025 that may be closed by this pull request
Copy link
Contributor

@pbeza pbeza left a comment

Choose a reason for hiding this comment

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

LGTM, but yeah — it’d be nice to find a proper way to test it so we don’t run into the same kind of bug for the third time.

@netrome
Copy link
Collaborator Author

netrome commented Oct 6, 2025

LGTM, but yeah — it’d be nice to find a proper way to test it so we don’t run into the same kind of bug for the third time.

Amen to that. As much as I'd love to do #1227, if you have any ideas of lower-hanging fruits or quick solutions in the meantime I'd love to hear them.

@netrome netrome added this pull request to the merge queue Oct 6, 2025
Merged via the queue into main with commit a820a92 Oct 6, 2025
11 checks passed
@netrome netrome deleted the 1224-mpc-node-panics-on-startup-in-tee branch October 6, 2025 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPC node panics on startup in TEE
2 participants