fix: add a way to verify current apphashes #2921
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 1m 51s —— View job Comprehensive Code ReviewI've completed a thorough review of this PR focusing on AppHash verification improvements. The changes successfully address a critical issue around ensuring block headers contain correct post-execution AppHashes. Here's my detailed analysis: 1. Code Quality & Architecture ✅Strengths:
Observations:
Minor Suggestion:
switch {
case bytes.Equal(header.AppHash, newState.AppHash):
// already matches
case len(header.AppHash) == 0, bytes.Equal(header.AppHash, currentState.AppHash):
header.AppHash = bytes.Clone(newState.AppHash)
headerModified = true
default:
return fmt.Errorf("header app hash mismatch...")
}2. Error Handling & Resilience ✅Strengths:
Concerns:
Recommendation: // Validate header's AppHash against execution result (if header has an AppHash set)
// Empty AppHash is tolerated for backwards compatibility with blocks produced
// before this validation was added.
if len(header.AppHash) != 0 && !bytes.Equal(header.AppHash, newState.AppHash) {
return fmt.Errorf("header app hash mismatch - got: %x, want: %x", header.AppHash, newState.AppHash)
}3. Security 🔒Strengths:
Critical Security Note:
No security vulnerabilities identified ✅ 4. Performance & Resource Efficiency ⚡Strengths:
Observations:
Recommendation: func (c *EngineClient) tryHashCandidates(
ctx context.Context,
candidates []common.Hash,
operation string,
fn func(common.Hash) error,
) error {
var lastErr error
for idx, candidate := range candidates {
err := fn(candidate)
if err == nil {
return nil
}
lastErr = err
if errors.Is(err, ErrInvalidPayloadStatus) && idx+1 < len(candidates) {
c.logger.Warn().
Str("blockHash", candidate.Hex()).
Msg(operation + ": execution engine rejected hash candidate, trying alternate")
continue
}
return err
}
return lastErr
}5. Testing & Reliability 🧪Strengths:
Coverage Gap (from Codecov):
Recommendations:
Example test to add: func TestExecutor_produceBlock_AppHashMismatch(t *testing.T) {
// Setup executor with mock that returns unexpected AppHash
// Expect error: "header app hash mismatch"
}
func TestSyncer_AppHashMismatchRejection(t *testing.T) {
// Create block with incorrect post-exec AppHash
// Verify syncer rejects it with proper error
}6. Documentation & Developer Experience 📚Strengths:
Areas for Improvement:
Recommendation: ## Overview
This PR adds robust AppHash verification to ensure block headers contain the correct post-execution state root.
### Problem
Previously, blocks could be produced/synced with incorrect or missing AppHash values, leading to state divergence.
### Solution
- **Executor**: Updates header AppHash after execution if empty or stale, rejects mismatches
- **Syncer**: Validates synced block headers have correct post-execution AppHash
- **Execution Client**: Tries multiple hash candidates for resilience to varying client behaviors
### Testing
- Added comprehensive tests for hash validation logic
- Updated existing tests to use correct post-execution AppHash valuesSummary & Verdict✅ Approve with Minor SuggestionsThis is a solid, necessary improvement to ensure state consistency across the network. The core logic is sound and the implementation is clean. Priority Improvements:
Code Quality: 8.5/10
Test Coverage: 7/10
Security: 10/10
Overall: 8/10 - Ready to merge with follow-up improvementsAction Items for Author:
|
Summary of ChangesHello @tac0turtle, 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 significantly improves the system's ability to correctly handle and verify application hashes, which are crucial for maintaining state consistency. By introducing flexible hash candidate selection during interactions with the execution engine and centralizing app hash validation, the changes ensure that blocks are processed and synced reliably, even when the execution environment presents slightly different hash values for the same state. This reduces potential points of failure and enhances the overall stability of the blockchain. Highlights
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 introduces a more robust mechanism for verifying and handling application hashes (AppHashes) from the execution client. The changes ensure that block headers contain the correct post-execution AppHash, both during block production and synchronization. Key changes include:
- In
block/internal/executing/executor.go, logic is added to update the header's AppHash after block execution, handling cases where it might be empty or contain a pre-execution hash. - In
block/internal/syncing/syncer.go, validation is added to ensure that a synced block's header AppHash matches the post-execution AppHash. - In
execution/evm/execution.go, the client now attempts multiple hash candidates (block hash and state root) for genesis and parent blocks when interacting with the execution engine, increasing compatibility. - The now-incorrect AppHash validation in
types/state.gohas been removed.
The changes are well-implemented and improve the node's resilience. I have a couple of suggestions for minor refactoring to improve code clarity and reduce duplication.
| switch { | ||
| case len(header.Header.AppHash) == 0: | ||
| header.Header.AppHash = bytes.Clone(newState.AppHash) | ||
| headerModified = true | ||
| case bytes.Equal(header.Header.AppHash, newState.AppHash): | ||
| // already matches expected state root | ||
| case bytes.Equal(header.Header.AppHash, currentState.AppHash): | ||
| // header still carries previous state's apphash; update it to the new post-state value | ||
| header.Header.AppHash = bytes.Clone(newState.AppHash) | ||
| headerModified = true | ||
| default: | ||
| return fmt.Errorf("header app hash mismatch - got: %x, want: %x", header.Header.AppHash, newState.AppHash) | ||
| } |
There was a problem hiding this comment.
The logic in this switch statement can be simplified by combining the cases that result in the same action. The cases for an empty AppHash and an AppHash matching the currentState both update the header. Grouping them improves readability and reduces redundancy.
switch {
case bytes.Equal(header.Header.AppHash, newState.AppHash):
// already matches expected state root
case len(header.Header.AppHash) == 0, bytes.Equal(header.Header.AppHash, currentState.AppHash):
// header has no apphash or previous state's apphash; update it to the new post-state value
header.Header.AppHash = bytes.Clone(newState.AppHash)
headerModified = true
default:
return fmt.Errorf("header app hash mismatch - got: %x, want: %x", header.Header.AppHash, newState.AppHash)
}| for idx, candidate := range candidates { | ||
| args := engine.ForkchoiceStateV1{ | ||
| HeadBlockHash: candidate, | ||
| SafeBlockHash: candidate, | ||
| FinalizedBlockHash: candidate, | ||
| } | ||
|
|
||
| // Validate payload status | ||
| if err := validatePayloadStatus(forkchoiceResult.PayloadStatus); err != nil { | ||
| err = retryWithBackoffOnPayloadStatus(ctx, func() error { | ||
| var forkchoiceResult engine.ForkChoiceResponse | ||
| err := c.engineClient.CallContext(ctx, &forkchoiceResult, "engine_forkchoiceUpdatedV3", args, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("engine_forkchoiceUpdatedV3 failed: %w", err) | ||
| } | ||
|
|
||
| if err := validatePayloadStatus(forkchoiceResult.PayloadStatus); err != nil { | ||
| c.logger.Warn(). | ||
| Str("status", forkchoiceResult.PayloadStatus.Status). | ||
| Str("latestValidHash", forkchoiceResult.PayloadStatus.LatestValidHash.Hex()). | ||
| Interface("validationError", forkchoiceResult.PayloadStatus.ValidationError). | ||
| Msg("InitChain: engine_forkchoiceUpdatedV3 returned non-VALID status") | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| }, MaxPayloadStatusRetries, InitialRetryBackoff, "InitChain") | ||
|
|
||
| if err == nil { | ||
| selectedGenesisHash = candidate | ||
| break | ||
| } | ||
|
|
||
| if errors.Is(err, ErrInvalidPayloadStatus) && idx+1 < len(candidates) { | ||
| c.logger.Warn(). | ||
| Str("status", forkchoiceResult.PayloadStatus.Status). | ||
| Str("latestValidHash", forkchoiceResult.PayloadStatus.LatestValidHash.Hex()). | ||
| Interface("validationError", forkchoiceResult.PayloadStatus.ValidationError). | ||
| Msg("InitChain: engine_forkchoiceUpdatedV3 returned non-VALID status") | ||
| return err | ||
| Str("blockHash", candidate.Hex()). | ||
| Msg("InitChain: execution engine rejected hash candidate, trying alternate") | ||
| continue | ||
| } | ||
|
|
||
| return nil | ||
| }, MaxPayloadStatusRetries, InitialRetryBackoff, "InitChain") | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } |
There was a problem hiding this comment.
The logic for iterating through hash candidates and retrying engine_forkchoiceUpdatedV3 is duplicated here and in the ExecuteTxs function (lines 369-426). This duplication increases maintenance overhead. Consider extracting this loop into a private helper method to promote code reuse and simplify both functions. The helper could accept the list of candidates and a function to perform the specific forkchoice update logic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2921 +/- ##
==========================================
- Coverage 60.14% 60.10% -0.04%
==========================================
Files 88 88
Lines 8427 8440 +13
==========================================
+ Hits 5068 5073 +5
- Misses 2787 2794 +7
- Partials 572 573 +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:
|
Overview