Conversation
- Add `rollback` command to testapp for rolling back the last block - Implement `RollbackOneBlock` in block manager to revert one block - Add `Rollback` method to store interface and implementation - Add mock for store rollback in tests
|
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 @julienrbrt, 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 introduces a fundamental block rollback capability to the ev-node system. It provides the necessary API within the block management layer and the underlying data store to revert the blockchain state by one block. Additionally, a command-line utility is provided to facilitate testing and usage of this new feature.
Highlights
- Block Rollback API: Introduced a new
RollbackOneBlockmethod within theblock.Managerto enable reverting the node to the state of the previous block. This involves fetching previous block data, deleting current block data from the store, and reapplying the state of the previous block. - Data Store Rollback: Added a
Rollbackmethod to thepkg/store.Storeinterface and itsDefaultStoreimplementation. This method handles the deletion of block headers, data, signatures, and index keys for blocks beyond a specified height, effectively truncating the block history in the data store. - Command-Line Utility: A new
rollbackcommand has been added to thetestappCLI, allowing users to easily trigger a single block rollback operation for testing and debugging purposes. - Mocking Updates: The
test/mocks/store.gofile was updated to include a mock implementation for the newRollbackmethod, ensuring testability of components interacting with theStoreinterface.
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 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 or fill out our survey 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 block rollback support, a valuable feature for chain stability. The implementation adds a RollbackOneBlock method to the block manager, a corresponding Rollback method to the store, and a new rollback command to the testapp.
My review has identified a few critical issues that should be addressed. The core rollback logic in block.Manager appears to be flawed, as it re-applies a block instead of restoring a previous state. The store's Rollback operation is not atomic, which could lead to data inconsistency. Furthermore, the new functionality is missing unit tests, which are essential for ensuring its correctness.
I've provided specific suggestions to fix these issues. Once these points are addressed, this will be a solid contribution to the project.
|
Claude finished @julienrbrt's task —— View job Pull Request Code ReviewI've completed a thorough review of this PR that adds block rollback support to ev-node. Here's my comprehensive analysis: Review Progress:
Overall AssessmentThis PR introduces a solid foundation for block rollback functionality with good test coverage and security considerations. However, there are several areas that need attention before merge. Major Issues1. Store Rollback Logic - Critical Bug in State Update 🚨Location: The current implementation has a critical flaw in how it handles state restoration: targetState, err := s.GetStateAtHeight(ctx, height)
// ...
if err := batch.Put(ctx, ds.NewKey(getStateAtHeightKey(height)), data); err != nil {
return fmt.Errorf("failed to set state at height %d: %w", height, err)
}Problem: The code saves the target state at the same height key, but it should be updating the "current" state to point to the target state. This creates inconsistency between Fix: Should update both the height and the current state key to maintain consistency. 2. Incomplete Rollback of State History
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2499 +/- ##
==========================================
- Coverage 72.77% 72.35% -0.43%
==========================================
Files 72 72
Lines 7310 7394 +84
==========================================
+ Hits 5320 5350 +30
- Misses 1566 1604 +38
- Partials 424 440 +16
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:
|
execution/evm/execution.go
Outdated
| } | ||
|
|
||
| // Rollback rolls back the state to the specified height. | ||
| func (c *EngineClient) Rollback(ctx context.Context, height uint64) error { |
There was a problem hiding this comment.
Should be done in a follow-up by someone with more context with the EVM execution environment. This is enough to unblock me for ev-abci rollback.
There was a problem hiding this comment.
with evm, the system would rollback on replay of the block or a different block.
| return fmt.Errorf("failed to marshal state to protobuf: %w", err) | ||
| } | ||
| return s.db.Put(ctx, ds.NewKey(getStateKey()), data) | ||
| return s.db.Put(ctx, ds.NewKey(getStateAtHeightKey(currentHeight)), data) |
There was a problem hiding this comment.
This change increase the need of coming up with a pruning solution: #2093
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| q := query.Query{} | ||
| results, err := k.db.Query(ctx, q) | ||
| if err != nil { | ||
| fmt.Printf("Error querying DB for key '%s': %v\n", key, err) |
Check failure
Code scanning / CodeQL
Log entries created from user input High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to sanitize the user-provided key before logging it. Since the logs are plain text, the recommended approach is to remove any newline (\n) and carriage return (\r) characters from the key before including it in the log message. This can be done using strings.ReplaceAll. The fix should be applied directly at the point where the log message is constructed, i.e., in apps/testapp/kv/kvexecutor.go at line 59. We need to ensure that the strings package is imported (it already is), so no new imports are needed.
| @@ -56,7 +56,8 @@ | ||
| q := query.Query{} | ||
| results, err := k.db.Query(ctx, q) | ||
| if err != nil { | ||
| fmt.Printf("Error querying DB for key '%s': %v\n", key, err) | ||
| safeKey := strings.ReplaceAll(strings.ReplaceAll(key, "\n", ""), "\r", "") | ||
| fmt.Printf("Error querying DB for key '%s': %v\n", safeKey, err) | ||
| return "", false | ||
| } | ||
| defer results.Close() |
|
|
||
| for result := range results.Next() { | ||
| if result.Error != nil { | ||
| fmt.Printf("Error iterating query results for key '%s': %v\n", key, result.Error) |
Check failure
Code scanning / CodeQL
Log entries created from user input High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to sanitize the user input before logging it. For plain text logs, the recommended approach is to remove any newline (\n) and carriage return (\r) characters from the user input before including it in the log entry. This can be done using strings.ReplaceAll. The fix should be applied directly in the logging statement on line 71 of apps/testapp/kv/kvexecutor.go, ensuring that the sanitized version of key is used in the log message. No changes to existing functionality are required, and no new methods or definitions are needed. The only required import (strings) is already present.
| @@ -68,7 +68,8 @@ | ||
|
|
||
| for result := range results.Next() { | ||
| if result.Error != nil { | ||
| fmt.Printf("Error iterating query results for key '%s': %v\n", key, result.Error) | ||
| safeKey := strings.ReplaceAll(strings.ReplaceAll(key, "\n", ""), "\r", "") | ||
| fmt.Printf("Error iterating query results for key '%s': %v\n", safeKey, result.Error) | ||
| return "", false | ||
| } | ||
|
|
core/execution/execution.go
Outdated
| // | ||
| // Returns: | ||
| // - error: Any errors during rollback | ||
| Rollback(ctx context.Context, height uint64) error |
There was a problem hiding this comment.
since the execution environment needs to implement rollback already for their system, why not wrap our rollback call in their command?
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
i dont think we should extend the execution interface here. the app dev needs to implement rollback anyways then wrap this call. This is equivalent to the execution environment implementing the command
I was thinking of that too. Do you think having a method on the rollkit store and remove the helper from the manager is sufficient? Because when I added it in the manager it felt like adding it to the exécution interface made sense. Then the command will be pure store access. |
|
Note, the rollback method (or its caller -- tbd) must make sure to not revert an height that has already been finalized (ie posted on da). |
Add validation to disallow rollback to a height lower than the DA included height, ensuring finalized heights cannot be rolled back. Add tests for various DA included height scenarios.
implemented in ede5086 |
3 participants
Closes: #2407
Superseded #2446
NOTE: this doesn't implement Rollback for any prod execution environment (evm).