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.

Sign up for GitHub

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

Jump to bottom

Conductor and sequencer p2p refactoring #11455

Merged
merged 10 commits into from
Aug 29, 2024
Merged

Conductor and sequencer p2p refactoring #11455

merged 10 commits into from
Aug 29, 2024

Conversation

anacrolix
Copy link
Contributor

@anacrolix anacrolix commented Aug 13, 2024 •
edited
Loading

Addresses #10936 (comment).

  • Stops the sequencer before stopping p2p so it can communicate any last work.
  • Reworks the p2p enabled logic so it's behaving like p2p is disabled when it's only in a partially shutdown state.
  • Increases logging around sequencing management in conductor.
  • Tidies up context use in various places to avoid the conductor sleeping during shutdown or getting stuck in API calls.
  • Removes a check for host that's no longer optional.

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

very nice

@anacrolix anacrolix force-pushed the anacrolix/p2p-shutdown branch 2 times, most recently from 6affbf5 to 48045a6 Compare August 19, 2024 11:11
semgrep-app[bot]
semgrep-app bot reviewed Aug 19, 2024
View reviewed changes
@anacrolix anacrolix force-pushed the anacrolix/p2p-shutdown branch from 48045a6 to 0a3bb21 Compare August 19, 2024 11:15
@anacrolix anacrolix force-pushed the anacrolix/p2p-shutdown branch from 0a3bb21 to 6bf15e7 Compare August 19, 2024 11:31
@anacrolix anacrolix marked this pull request as ready for review August 20, 2024 06:07
@anacrolix anacrolix requested review from 0x00101010, zhwrd, mslipper and a team as code owners August 20, 2024 06:07
@anacrolix anacrolix requested a review from sebastianst August 20, 2024 06:07
sebastianst
sebastianst reviewed Aug 20, 2024
View reviewed changes
@anacrolix anacrolix changed the title op-node: P2P shutdown Conductor and sequencer p2p refactoring Aug 20, 2024
@anacrolix anacrolix force-pushed the anacrolix/p2p-shutdown branch from d5e3b4c to 571cbec Compare August 20, 2024 10:42
@tynes tynes mentioned this pull request Aug 22, 2024
Open
@anacrolix anacrolix requested a review from sebastianst August 26, 2024 23:09
@anacrolix anacrolix requested a review from tynes August 26, 2024 23:17
@tynes
Copy link
Contributor

tynes commented Aug 27, 2024

I am not sure how extensive the test coverage of this code is. Do we have unit tests? Is the best way to test just syncing a node?

@anacrolix
Copy link
Contributor Author

I'll check tests and fix conflicts now.

@anacrolix
Copy link
Contributor Author

I don't think the test failures are related to this code

@anacrolix anacrolix enabled auto-merge August 28, 2024 09:24
@anacrolix anacrolix linked an issue Aug 28, 2024 that may be closed by this pull request
op-node: Improve shutdown behavior #10936
Closed
@anacrolix anacrolix self-assigned this Aug 28, 2024
@anacrolix
Copy link
Contributor Author

Looks like I need a code owners approval?

sebastianst
sebastianst reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

LGTM, effectively the only change in behavior in this PR is to call n.l2Driver.StopSequencer(ctx) inside OpNode.Stop. Everything else is just clean up work.

Will approve once the open conversations are resolved.

@anacrolix anacrolix requested a review from sebastianst August 29, 2024 04:39
sebastianst
sebastianst approved these changes Aug 29, 2024
View reviewed changes
@anacrolix anacrolix added this pull request to the merge queue Aug 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 29, 2024
@sebastianst sebastianst added this pull request to the merge queue Aug 29, 2024
Merged via the queue into develop with commit c7b91ab Aug 29, 2024
57 checks passed
@sebastianst sebastianst deleted the anacrolix/p2p-shutdown branch August 29, 2024 16:17
@anacrolix
Copy link
Contributor Author

🎉

samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
@anacrolix @sebastianst @samlaf
Conductor and sequencer p2p refactoring (ethereum-optimism#11455)
55d20e0 
* Shutdown sequencer before stopping p2p

* Check p2p isn't also disabled

Co-authored-by: Sebastian Stammler <[email protected]>

* Remove missed time.Sleep

* Fix up use of SetupP2P.Disabled

* Revert error check after RPC boundary

* Add comment about context for StopSequencer

* Add Config.p2pEnabled

* op-node: Make Config.P2PEnabled public

---------

Co-authored-by: Sebastian Stammler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@zhwrd zhwrd zhwrd approved these changes

@sebastianst sebastianst sebastianst approved these changes

@0x00101010 0x00101010 Awaiting requested review from 0x00101010

@mslipper mslipper Awaiting requested review from mslipper

@tynes tynes Awaiting requested review from tynes

Assignees

@anacrolix anacrolix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

op-node: Improve shutdown behavior
4 participants
@anacrolix @tynes @zhwrd @sebastianst