-
Notifications
You must be signed in to change notification settings - Fork 823
refactor(reexcute/c): generic VM reexecution #4451
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
base: master
Are you sure you want to change the base?
Conversation
|
@copilot Can you do a code review similar to the review that you would do if I marked this PR as ready for review? Update: this failed miserably. |
|
@RodrigoVillar I've opened a new pull request, #4452, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Example of metrics working from the latest CI run (ref): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the reexecution test infrastructure to support generic VM reexecution by extracting common logic from the C-Chain specific package into a reusable reexecute package. This enables other VMs (like P-Chain) to leverage the same reexecution testing framework.
Key changes:
- Moves
VMExecutor, block channel creation, and consensus metrics fromtests/reexecute/cto the generictests/reexecutepackage - Introduces
NewMainnetVM()constructor for creating mainnet-configured VMs with standardized initialization - Uplifts metrics server and collector utilities to the shared package
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/reexecute/vm_executor.go | New file containing the generic VMExecutor implementation with block execution logic, mainnet VM initialization, and consensus metrics |
| tests/reexecute/metrics.go | New file with uplifted StartServer and StartCollector functions for Prometheus metrics handling |
| tests/reexecute/export.go | New file with ExportBlockRange function for copying blocks between LevelDB directories |
| tests/reexecute/c/vm_reexecute_test.go | Refactored to use the new generic reexecute package, removing C-Chain specific implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Quality work on this. I've been looking at the C-chain and P-chain test files, and I'm noticing they're mostly identical. Both have the same flag definitions, similar TestMain setup, identical metrics server/collector logic, and the same database initialization pattern. The main differences are really just:
I wonder if could we can reduce the boilerplate? Something like having a single I'm imagining something like: A few questions I'm curious about:
We could eliminate duplication per chain by having one test file, and make adding new chains as simple as implementing the interface and registering it. But I'm also aware there might be reasons to keep things separate. What do you think? |

Why this should be merged
As part of #4108, this PR allows for generic VM reexecution by refactoring the
reexecute/cpackage to only include C-Chain related logic, and migrates all other code to thereexecutepackage.An example of how other VMs can utilize this refactored code for their reexecution tests is #4458, which adds reexecution tests for the P-Chain.
How this works
VMExecutorand extends it by integrating the block channel and consensus metrics into it.NewMainnetVM()constructor to facilitate creating/initializing mainnet VMs (such as the EVM and PlatformVM).StartServer()andStartCollector()ExecuteBlockRange()How this was tested
CI
Need to be documented in RELEASES.md?
No