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: spawn/kill anvil-zksync with PGID #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itegulov
Copy link
Collaborator

@itegulov itegulov commented Feb 3, 2025

Encountered an issue with the WIP version of anvil-zksync that now spawns an L1 anvil subprocess. Killing anvil-zksync by PID turns anvil subprocess a zombie.

There isn't much we can do on anvil-zksync side as std doesn't bother with SIGTERM and sends SIGKILL right off the bat. Thus we can't register a proper handler.

Note: this is UNIX-specific, no idea what proper solution for Windows would look like

Copy link
Owner

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

Not sure if I agree with this approach.
I'd expect it to be possible to run anvil externally to anvil-zksync, e.g. something like:

anvil &
# Make `anvil-zksync` assume that `anvil` already runs, don't run it automatically
anvil-zksync --l1-instance external

(it sounds helpful on its own so that you can see what's going on easily)

With that, you can just change how we spawn it:
One process for anvil, and then one process for anvil-zksync. On drop, kill them in reverse.

This is cross-platform, and this is more explicit. WDYT?

@itegulov
Copy link
Collaborator Author

itegulov commented Feb 3, 2025

I thought we agreed that the first iteration would provide a built-in anvil instance instead of relying on user to supply their own?

Generally I agree that we need to support externally run anvil. But, the way I see it right now, the process will become more manual and error-prone for users. We either have to ask users to start anvil with --no-request-size-limit so we can inject the state (the state's size is over anvil's default limit) or to start it with --load-state <state_json_path> (we unpack state file and interactively print the path I guess?). There is also a question of backwards compatibility: there are some side effects of loading so much state that affect even the version I have been working with this whole time - anvil-0.3.0 (e.g. its fee mechanism becomes messed up until you submit at least one tx with huge gas price). It is impossible to predict what can go wrong with any other version.

To sum it up, if we don't want to spawn anything from anvil-zksync at all then I am fine with closing this PR, otherwise we need to figure something out.

@popzxc
Copy link
Owner

popzxc commented Feb 3, 2025

Nah, I meant that by default we will spawn anvil from anvil-zksync, but it's possible to run it externally if you know exactly what you're doing.
E.g.

anvil-zksync # Runs `anvil` for you
anvil-zksync --l1-instance external # Expects you to run `anvil` with all the required settings prepared

Although based on your feedback it looks complicated, so now I'm much less sure.

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.

2 participants