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(cli): adds load and state cli options #502

Merged
merged 19 commits into from
Dec 20, 2024

Conversation

dutterbutter
Copy link
Collaborator

@dutterbutter dutterbutter commented Dec 18, 2024

What 💻

Why ✋

  • Anvil parity
  • Fixes load state from forked dumped state

Evidence 📷

Include screenshots, screen recordings, or console output here demonstrating that your changes work as intended

Run the following command to first easily create a state.json file:

./target/release/anvil-zksync --dump-state state.json --state-interval 1 run
  • make some transactions if you would like

Then we can use that state to start the node using --load-state

./target/release/anvil-zksync --load-state state.json --state-interval 1 run

Forking:

./target/release/anvil-zksync --dump-state state.json fork --fork-url mainnet

Then we can use that state to start the node using --load-state

./target/release/anvil-zksync --load-state state.json --state-interval 1 fork --fork-url mainnet

@dutterbutter dutterbutter added the do not merge 🛑 Do not merge this PR until label is removed label Dec 18, 2024
@dutterbutter dutterbutter changed the title feat: adds load and state cli options feat(cli): adds load and state cli options Dec 18, 2024
Base automatically changed from db/add-state-cli-options to main December 19, 2024 02:30
@dutterbutter dutterbutter removed the do not merge 🛑 Do not merge this PR until label is removed label Dec 19, 2024
@dutterbutter dutterbutter force-pushed the db/adds-load-state-cli-option branch from d62332d to 9c0e73d Compare December 19, 2024 03:50

// Allow some time for the state to be dumped
sleep(Duration::from_secs(2));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggest keeping sleep here, as sometimes the json file is not dumped in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add a sigterm handler to enforce dumping. I will do some experiments later

@dutterbutter dutterbutter marked this pull request as ready for review December 19, 2024 04:01
@dutterbutter dutterbutter requested a review from a team as a code owner December 19, 2024 04:01
Comment on lines +748 to +756
let new_provider = init_testing_provider(move |node| {
node.path(get_node_binary_path())
.arg("--state-interval")
.arg("1")
.arg("--load-state")
.arg(dump_path_clone.to_str().unwrap())
.fork("sepolia-testnet")
})
.await?;
Copy link
Collaborator Author

@dutterbutter dutterbutter Dec 19, 2024

Choose a reason for hiding this comment

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

@itegulov I've noticed some flakiness with the fork tests timing out during initialization (e.g. test has been running for longer then x).

Any recommendations to harden?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I had a feeling this would be an issue. I think we should not do fork tests in this form right now. To minimize flakiness we should set up a real local ZKsync enviornment and use as forking source in all of the fork e2e tests. For example we could spin up dockerized zksync-era node and then seal some blocks.

In any case I think it's for the better to delete these for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Skipping the fork tests for now, and created issue: #508

@dutterbutter dutterbutter added the needs review 👓 PR requires a review label Dec 19, 2024
itegulov
itegulov previously approved these changes Dec 19, 2024
Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit

crates/cli/Cargo.toml Outdated Show resolved Hide resolved
@@ -548,6 +548,7 @@ impl ForkDetails {
)
})?;
let l1_batch_number = block_details.l1_batch_number;
block.l1_batch_number = Some(l1_batch_number.0.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I have feeling this might lead to some unexpected behaviour because that block might technically not be sealed in that batch yet (this is what l1BatchNumber == null means). But anyway, the whole forking logic needs some rethinking in general so I am fine with keeping as is

…s/anvil-zksync into db/adds-load-state-cli-option
@dutterbutter dutterbutter merged commit 070c569 into main Dec 20, 2024
9 checks passed
@dutterbutter dutterbutter deleted the db/adds-load-state-cli-option branch December 20, 2024 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review 👓 PR requires a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support anvil's state-related CLI options
2 participants