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

refactor(trin-execution): state import/export don't need TrinExecution #1505

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented Oct 2, 2024

What was wrong?

The import/export state in trin-execution don't need TrinExecution. They only need EvmDb and ExecutionPosition.

No tests that test import/export end to end.

How was it fixed?

Refactor how Importer/Exporter are initialized.

Also added test that:

  • executes fist 1000 blocks
  • exports state to era2 file
  • imports state from era2 file (into new directory)
  • executes another 1000 blocks

To-Do

@morph-dev morph-dev requested a review from KolbyML October 2, 2024 11:24
@morph-dev morph-dev self-assigned this Oct 2, 2024
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

Most of the PR looks good, but I don't think the subcommand's should have a param data_dir, the param from TrinExecutionConfig should be used.

./trin --data-dir="/" import-state --data-dir="/hi"
^ this is a valid commandline in this PR

I think this is very misleading to the user.

./trin --data-dir="/" import-state
^ I think we should have this. As the data-directory for Trin-Execution shouldn't be an param of a subcommand, it should be a param of Trin-Execution itself

trin-execution/src/cli.rs Outdated Show resolved Hide resolved
trin-execution/src/cli.rs Outdated Show resolved Hide resolved
@KolbyML
Copy link
Member

KolbyML commented Oct 2, 2024

Also I think the first commit is a refactor not a feat as no functionality is changed other then the issue I mentioned above

@KolbyML
Copy link
Member

KolbyML commented Oct 2, 2024

Also to note Data dir import export is already supported before this PR using

./trin --data-dir="/" import-state This matches how other execution clients like go-ethereum do it as well for there era1 import/export

@morph-dev morph-dev changed the title Data dir import export refactor(trin-execution): state import/export don't need TrinExecution structure Oct 2, 2024
@morph-dev
Copy link
Collaborator Author

You are right. I was trying:

./target/release/trin-execution export-state --data-dir="/my_dir" --path-to-era2="/era_dir"

and that didn't work. And I interpreted the error that I was getting as that none of the other cli arguments are available if subcommand is selected.
But you are right. It works if you specify the argument before subcommand.

If I knew this, I probably wouldn't even do this PR. But I think it's nice improvement (main is a bit cleaner) and we have a test.

@morph-dev morph-dev force-pushed the data_dir_import_export branch from 8d2f93b to f033731 Compare October 2, 2024 18:48
@morph-dev morph-dev changed the title refactor(trin-execution): state import/export don't need TrinExecution structure refactor(trin-execution): state import/export don't need TrinExecution Oct 2, 2024
@KolbyML
Copy link
Member

KolbyML commented Oct 2, 2024

You are right. I was trying:

./target/release/trin-execution export-state --data-dir="/my_dir" --path-to-era2="/era_dir"

and that didn't work. And I interpreted the error that I was getting as that none of the other cli arguments are available if subcommand is selected. But you are right. It works if you specify the argument before subcommand.

If I knew this, I probably wouldn't even do this PR. But I think it's nice improvement (main is a bit cleaner) and we have a test.

Yeah, I guess we should add a few examples in the readme to document this. You can do this in this PR, or I can make a PR for this after this is merged, or something doesn't matter. But it would be nice to prevent confusion surrounding this in the future

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: everything looks good. Don't forget to

    // Initialize prometheus metrics
    if let Some(addr) = trin_execution_config.enable_metrics_with_url {
        prometheus_exporter::start(addr)?;
    }

To before the sub command code so logs work (ideally it is as high as it can go).

Other then the the PR looks good

trin-execution/src/main.rs Outdated Show resolved Hide resolved
@morph-dev
Copy link
Collaborator Author

Yeah, I guess we should add a few examples in the readme to document this. You can do this in this PR, or I can make a PR for this after this is merged, or something doesn't matter. But it would be nice to prevent confusion surrounding this in the future

Unless I missed it, I don't think we have documentation for import/export at all. I would leave that for another PR.

@morph-dev morph-dev merged commit 1565d84 into ethereum:master Oct 2, 2024
9 checks passed
@morph-dev morph-dev deleted the data_dir_import_export branch October 2, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants