Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 1m 58s —— View job Code Review CompleteI've reviewed the replay logic fix. This PR addresses a critical bug in the app hash verification during block replay. The fix is fundamentally correct and well-tested. Tasks:
Overall AssessmentVerdict: Approved with minor suggestions This is a solid fix for a subtle but critical bug in the replay logic. The core issue was that the code was comparing the execution result against Detailed Findings1. Code Quality & Architecture ✅Strengths:
Observations:
2. Correctness Analysis ✅The Bug: if !bytes.Equal(newAppHash, header.AppHash) // WRONG!This was incorrect because:
The Fix: expectedState, err := s.store.GetStateAtHeight(ctx, height)
if err == nil {
if !bytes.Equal(newAppHash, expectedState.AppHash) // CORRECT!This correctly compares the execution result against the stored state at the current height, which contains the expected app hash. Critical Insight (replay.go:163-164): Graceful Degradation (replay.go:214-220): 3. Error Handling & Resilience ✅Strengths:
Minor Suggestion: expectedState, err := s.store.GetStateAtHeight(ctx, height)
if err == nil {
// verification logic...
} else {
s.logger.Debug().
Uint64("height", height).
Err(err). // Add this
Str("app_hash", hex.EncodeToString(newAppHash)).
Msg("replayBlock: ExecuteTxs completed (no stored state to verify against)")
}4. Security ✅Observations:
Note on Trust Model: 5. Testing & Reliability ✅Test Coverage - Excellent:
Test Quality:
Key Test Update (replay_test.go:50, 72-73): AppHash: []byte("app-hash-99"), // This is the input app hash (from state 99)
// ...
mockExec.On("ExecuteTxs", ..., []byte("app-hash-99")).
Return([]byte("app-hash-100"), nil)This correctly shows that block 100 takes 6. Performance & Resource Efficiency ✅Observations:
Minor Optimization Opportunity: 7. Documentation & Developer Experience ✅Strengths:
CHANGELOG Entry: - Fix replay logic leading to app hashes by verifying against the wrong block [#3053]Minor suggestion: Could be more specific: "Fix replay logic app hash mismatch by verifying execution result against correct stored state instead of input header hash" Specific Code Commentsblock/internal/common/replay.goLine 159 ✅ Good comment clarifying genesis app hash is input Lines 163-165 ✅ Excellent comment explaining Lines 166-168 ✅ Good simplification - removed redundant Lines 188-190 ✅ Critical comment explaining header.AppHash vs expected output Lines 193-220 ✅ Excellent verification logic with graceful degradation block/internal/common/replay_test.goLine 5 ✅ Added Lines 42-73 ✅ Properly updated mocks to reflect input/output distinction Lines 76-84 ✅ New verification mock with correct expected output hash Lines 329 ✅ Added missing Lines 445-516 ✅ New test case for missing state - excellent edge case coverage Potential Issues & SuggestionsMinor Issues:
No Breaking Changes:The fix maintains the same external interface and behavior, only correcting the internal verification logic. Recommendations
SummaryThis PR fixes a critical bug where replay logic was comparing execution output against the wrong hash (input instead of expected output). The fix:
The fact that Claude suggested this solution in "one shot" speaks to the clarity of the problem once the input/output hash distinction was understood. Excellent work! 👏🏾 |
Summary of ChangesHello @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 addresses a critical bug in the block replay mechanism where the application hash (AppHash) logic was flawed, potentially causing incorrect state synchronization. The changes ensure that the correct AppHash is used as input for block execution and that the verification process accurately compares the execution output against the expected state, thereby stabilizing the replay process and preventing discrepancies in the application state. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a critical fix to the block replay logic. The previous implementation incorrectly determined the input AppHash for transaction execution and performed an invalid verification of the resulting AppHash. The changes correct this by using the state from height-1 to get the input AppHash and verifying the new AppHash against the stored state at the current height. The test suite has been updated accordingly to reflect these logic changes and includes new test cases for app hash mismatches and scenarios where state for verification is not yet present. The fix appears correct and robust. I have one suggestion to improve the error handling during the state verification step to make it more resilient.
| if err == nil { | ||
| // State exists, verify the app hash matches | ||
| if !bytes.Equal(newAppHash, expectedState.AppHash) { | ||
| err := fmt.Errorf("app hash mismatch at height %d: expected %s got %s", | ||
| height, | ||
| hex.EncodeToString(expectedState.AppHash), | ||
| hex.EncodeToString(newAppHash), | ||
| ) | ||
| s.logger.Error(). | ||
| Str("expected", hex.EncodeToString(expectedState.AppHash)). | ||
| Str("got", hex.EncodeToString(newAppHash)). | ||
| Uint64("height", height). | ||
| Err(err). | ||
| Msg("app hash mismatch during replay") | ||
| return err | ||
| } | ||
| s.logger.Debug(). | ||
| Uint64("height", height). | ||
| Str("app_hash", hex.EncodeToString(newAppHash)). | ||
| Msg("replayBlock: app hash verified against stored state") | ||
| } else { | ||
| // State doesn't exist yet, we trust the execution result since we're replaying validated blocks. | ||
| s.logger.Debug(). | ||
| Uint64("height", height). | ||
| Err(err). | ||
| Msg("app hash mismatch during replay") | ||
| return err | ||
| Str("app_hash", hex.EncodeToString(newAppHash)). | ||
| Msg("replayBlock: ExecuteTxs completed (no stored state to verify against)") | ||
| } |
There was a problem hiding this comment.
This logic handles errors from s.store.GetStateAtHeight by assuming any error means the state was not found, and then proceeds without verification. This is risky because it could mask other critical errors from the store, such as data corruption, disk I/O issues, or unmarshalling problems. In such cases, the replayer would silently proceed and potentially persist an incorrect state.
It would be safer to explicitly check for a 'not found' error and treat all other errors as fatal for the replay of this block.
A more robust implementation would be to check for the specific 'not found' error. For example:
expectedState, err := s.store.GetStateAtHeight(ctx, height)
if err != nil {
// It's important to distinguish 'not found' from other errors.
// This might require exporting an error from the store package or using errors.Is with the underlying datastore error.
if !isNotFoundError(err) { // isNotFoundError is a placeholder for the actual check
return fmt.Errorf("failed to get expected state for verification at height %d: %w", height, err)
}
// State not found, proceed without verification (as intended)
s.logger.Debug().
Uint64("height", height).
Str("app_hash", hex.EncodeToString(newAppHash)).
Msg("replayBlock: ExecuteTxs completed (no stored state to verify against)")
} else {
// State exists, perform verification
if !bytes.Equal(newAppHash, expectedState.AppHash) {
// ... error handling for mismatch
}
}Please consider refining this error handling to make the replay process more resilient to unexpected storage issues.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3053 +/- ##
==========================================
+ Coverage 56.56% 56.62% +0.06%
==========================================
Files 118 118
Lines 12151 12152 +1
==========================================
+ Hits 6873 6881 +8
+ Misses 4536 4530 -6
+ Partials 742 741 -1
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:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
The replay logic was wrong and leading to app hashes.
This PR fixes it. The correct solution was one shot by Claude 👏🏾