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

op-supervisor: initialize cross-safe starting point #12841

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

protolambda
Copy link
Contributor

Description

Experimental change to make the op-supervisor require a supervisor_initializeCrossSafe.
This way the op-supervisor cannot accidentally skip prior sync work, and this way we avoid some previous edge-cases around an empty database. (the panic when the parent block of the first newly inserted block doesn't exist).

Tests

Updated existing tests, and added a test-case to check if the initialization part of the interop deriver works.

@protolambda protolambda added this to the Interop: Stable Devnet milestone Nov 6, 2024
@axelKingsley axelKingsley self-requested a review November 6, 2024 17:47
@axelKingsley
Copy link
Contributor

axelKingsley commented Nov 6, 2024

Nice, I like this. Now, when we attempt to push data into the database, it does mean that if the supervisor and op-node start correctly, they'll go through one round of error back-and-forth. But as that's just for seeding the supervisor, no big deal.

I added a commit to fix linting, but there's still issues in the Action tests: the action tests assume you can SyncCrossSafe on the chains before doing any work. This triggers the underlying workflow which causes the uninitialized error. So, I commented out the Sync lines, and put them back below a call to InitializeCrossSafe. However, commenting out SyncCrossSafe causes require calls to fail when checking that status.crossUnsafeL2.ID() is correct.

I find this action test failure very surprising, as I wouldn't expect SyncCrossSafe to have any implications for the unsafe. I'll think on this more, but may leave this PR as-is.

@protolambda I'll pre-approve this PR given that the change is sound, and if you manage to fix the action tests then you can go straight to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants