feat: fix execution verification #2433
Conversation
WalkthroughThe changes re-enable and enforce stricter block validation checks in the block manager, ensuring block timestamps are strictly increasing and application hashes match. Corresponding test cases for these checks are also reactivated. Additionally, mutex locking is removed from a section of the EVM execution logic involving forkchoice state creation. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Manav-Aggarwal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the block execution and verification process by introducing explicit execution modes (delayed and immediate). This allows the system to adapt its state management and AppHash validation logic based on the specific behavior of the underlying execution environment, ensuring correctness and flexibility for different types of applications, such as EVM-based ones.
Highlights
- Execution Mode Definition: Introduced an
ExecutionModeenum (ExecutionModeDelayedandExecutionModeImmediate) to explicitly define when an executor applies state transitions relative to block creation. This clarifies the behavior of different execution environments. - Dynamic AppHash Calculation: Modified the block creation logic (
execCreateBlock) to dynamically calculate theAppHashbased on the executor'sExecutionMode. For immediate execution, transactions are executed during block creation to derive the new state root, while for delayed execution, the previous state'sAppHashis used. - Conditional Block Verification: Updated block validation (
execValidate) and application (execApplyBlock) to correctly verify and process blocks according to the determinedExecutionMode. This includes conditionalAppHashchecks and transaction execution based on whether the node is a block creator or a syncing node. - Executor Interface Extension: Extended the
Executorinterface with aGetExecutionMode()method, requiring all executor implementations (e.g.,KVExecutor,DummyExecutor,EngineClient) to declare their supported execution mode.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new ExecutionMode to distinguish between immediate and delayed transaction execution, which is a great feature. The changes are well-implemented across the Executor interface and its implementations. My review includes a couple of suggestions for block/manager.go to improve maintainability by reducing code duplication and to enhance robustness by adding logging for silently handled errors. Overall, this is a solid contribution.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2433 +/- ##
==========================================
+ Coverage 72.71% 72.74% +0.03%
==========================================
Files 67 67
Lines 6395 6403 +8
==========================================
+ Hits 4650 4658 +8
Misses 1347 1347
Partials 398 398
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manav-Aggarwal
force-pushed
the
manav/execution_verification
branch
2 times, most recently
from
0d68d9c to
63c0996
Compare
There was a problem hiding this comment.
Pull Request Overview
The PR restores execution verification in the block manager, updates related tests, and simplifies EVM execution by removing an unnecessary lock.
- Removed redundant mutex around
ForkchoiceStateV1construction in EVM execution. - Re-enabled and updated timestamp and AppHash checks in
Manager.execValidate, adjusting error messages. - Un-commented and modified tests in
manager_test.goto match the new error messages.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| execution/evm/execution.go | Dropped c.mu.Lock()/Unlock() around forkchoice args |
| block/manager.go | Restored timestamp and AppHash validation in execValidate |
| block/manager_test.go | Re-enabled tests for monotonic timestamps and AppHash mismatch |
Comments suppressed due to low confidence (1)
Manav-Aggarwal
force-pushed
the
manav/execution_verification
branch
from
63c0996 to
2e0b538
Compare
| } | ||
|
|
||
| // AppHash should match the last state's AppHash | ||
| if !bytes.Equal(header.AppHash, lastState.AppHash) { |
There was a problem hiding this comment.
While this works we should open an issue so we don't forget to dive deeper
* main: feat: fix execution verification (#2433) build(deps): Bump github.com/spf13/viper from 1.19.0 to 1.20.1 (#2431) build(deps): Bump github.com/libp2p/go-libp2p-kad-dht from 0.29.1 to 0.33.1 (#2432) build(deps): Bump github.com/prometheus/client_golang from 1.21.1 to 1.22.0 (#2430) build(deps): Bump alpine from 3.18.3 to 3.22.0 (#2429) ci: fix dependabot patterns (#2427) feat: migrate logging to ipfs/go-log/v2 (#2416) fix: Consolidate some EVM E2E tests into one (#2423) feat: Support GetHeader in store (#2422) fix: Optimize E2E EVM tests (#2421) fix: getPeers size check (#2417) test: Add comprehensive EVM E2E tests (#2394) feat: migrate logging to ipfs/go-log/v2 (#2411) feat: execution verification (#2377)
Overview
Simpler version of #2377, was reverted in #2394
Summary by CodeRabbit
Bug Fixes
Tests
Refactor