feat: execution verification #2377
Conversation
WalkthroughThe changes introduce explicit support for "execution modes" (immediate vs. delayed) throughout the block manager, executor interfaces, and related implementations. The Manager now caches the execution mode, and block validation, creation, and application logic are updated to handle these modes correctly. Timestamp and AppHash validation are reinstated in Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Executor
participant State
Note over Manager: Block Validation (execValidate)
Manager->>Executor: GetExecutionMode()
alt Delayed Execution
Manager->>State: Compare AppHash with header (previous block)
else Immediate Execution
Manager->>State: Skip AppHash check (AppHash reflects post-execution)
end
Manager->>State: Validate timestamp is increasing
sequenceDiagram
participant Manager
participant Executor
participant State
Note over Manager: Block Creation (execCreateBlock)
Manager->>Executor: GetExecutionMode()
alt Immediate Execution and non-empty block
Manager->>Executor: ExecuteTxs()
Executor-->>Manager: New state root (AppHash)
else
Manager->>State: Use last state's AppHash
end
Manager->>Manager: Set header fields (version, chain ID, etc.)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
📄 Source: CodeRabbit Inference Engine (CLAUDE.md) List of files the instruction was applied to:
🧠 Learnings (2)📓 Common learningsblock/manager.go (4)⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (5)
✨ 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).
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2377 +/- ##
==========================================
- Coverage 73.05% 72.48% -0.58%
==========================================
Files 67 67
Lines 6377 6510 +133
==========================================
+ Hits 4659 4719 +60
- Misses 1320 1391 +71
- Partials 398 400 +2
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
block/manager.go (1)
819-850: Review execution mode handling in block creation.The execution mode logic appears correct:
- For immediate mode with transactions: executes transactions during creation to compute AppHash
- For delayed mode or empty blocks: uses AppHash from last state
However, there's a potential issue with the empty block check condition on line 834.
Consider clarifying the condition logic:
- if m.executionMode == coreexecutor.ExecutionModeImmediate && !isEmpty { + if m.executionMode == coreexecutor.ExecutionModeImmediate && !isEmpty {While the current logic is correct, you might want to add a comment explaining why empty blocks always use the last state's AppHash regardless of execution mode:
// Determine AppHash based on execution mode var appHash []byte + // Note: Empty blocks always use last state AppHash regardless of execution mode + // since there are no transactions to execute if m.executionMode == coreexecutor.ExecutionModeImmediate && !isEmpty {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
block/manager.go(7 hunks)block/manager_test.go(1 hunks)block/publish_block_p2p_test.go(2 hunks)da/internal/mocks/da.go(1 hunks)test/mocks/da.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- da/internal/mocks/da.go
- test/mocks/da.go
🚧 Files skipped from review as they are similar to previous changes (1)
- block/manager_test.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (8)
block/publish_block_p2p_test.go (2)
23-23: LGTM: Appropriate import addition.The import of the core execution package is necessary to support the new execution mode functionality.
240-242: LGTM: Correct interface implementation.The mock executor correctly implements the new
GetExecutionMode()method, returningExecutionModeDelayedwhich is consistent with other test executors and supports the execution mode feature.block/manager.go (6)
156-157: LGTM: Proper execution mode caching.The execution mode is correctly cached from the executor during manager initialization and stored in the struct for efficient access during block operations.
Also applies to: 362-362, 393-393
775-789: LGTM: Execution mode-aware validation logic.The validation correctly:
- Re-enables timestamp validation for strictly increasing block times
- Implements execution mode-specific AppHash validation
- In delayed mode, validates AppHash matches last state (since transactions haven't been executed yet)
- Appropriately skips AppHash validation in immediate mode during this phase
The logic aligns with the execution mode semantics.
794-794: LGTM: Added lastState parameter for block creation.The addition of the
lastStateparameter enables the block creation logic to access version and chain ID information, which is necessary for proper block header construction.
854-866: LGTM: Proper usage of lastState for header fields.The header construction correctly uses
lastStatefor version, chainID, and computed AppHash, ensuring consistency with the current blockchain state.
917-917: LGTM: Proper context passing for signed header.The addition of the signed header to the context enables executors to access header information during transaction execution, which may be necessary for certain validation or state computations.
Also applies to: 936-936
886-942: Verify the block creator detection logic.The execution logic for immediate mode is sophisticated but potentially fragile. The block creator detection relies on:
- Address matching between signer and proposer
- Height comparison with store height + 1
This could have edge cases during concurrent operations or rapid block production.
The logic assumes that if we're the proposer and the height matches
currentHeight + 1, then we created this block. However, this could be problematic in scenarios where:
- Multiple blocks are being processed concurrently
- There's a race condition between block creation and application
- Store height updates happen asynchronously
Consider adding additional verification or making the block creator detection more robust.
#!/bin/bash # Search for concurrent access patterns and potential race conditions rg -A 5 -B 5 "store\.Height|SetHeight" --type go ast-grep --pattern 'go func() { $$$ SetHeight($$$) $$$ }'
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
execution/evm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
block/manager.go(7 hunks)block/manager_test.go(1 hunks)execution/evm/execution.go(1 hunks)execution/evm/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- execution/evm/go.mod
- execution/evm/execution.go
- block/manager_test.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Go Mod Tidy Check
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (10)
block/manager.go (10)
159-160: LGTM! Clean addition of execution mode caching.The addition of the
executionModefield to cache the executor's execution mode is well-documented and follows good practice by avoiding repeated calls toGetExecutionMode().
365-365: LGTM! Proper execution mode retrieval.Getting the execution mode from the executor during manager initialization is the correct approach.
396-396: LGTM! Execution mode properly stored in manager.The execution mode is correctly stored in the manager struct for later use.
817-832: LGTM! Improved validation logic with execution mode awareness.The timestamp validation re-enablement and conditional AppHash validation based on execution mode is well-implemented:
- Timestamp monotonicity validation is correctly restored
- AppHash validation is appropriately skipped for immediate mode since the AppHash represents the state after executing the current block's transactions
- The error messages are clear and informative
836-836: LGTM! Updated method signature with lastState parameter.The addition of the
lastStateparameter toexecCreateBlockis necessary for the execution mode logic that follows.
861-893: LGTM! Well-structured execution mode handling in block creation.The logic correctly handles both execution modes:
- For immediate mode with non-empty blocks: executes transactions to compute the new state root
- For delayed mode or empty blocks: uses the last state's AppHash
- Transaction conversion is properly handled
- Error handling is appropriate
The implementation follows the execution semantics correctly.
897-908: LGTM! Proper header construction with state-derived values.The header construction correctly uses values from
lastState:
- Version information is properly copied
- ChainID is set from the last state
- AppHash is set based on the execution mode logic above
918-924: LGTM! Correct DataHash handling for empty vs non-empty blocks.The DataHash is properly set:
- For non-empty blocks: uses the block data's DA commitment
- For empty blocks: uses the predefined hash for empty transactions
948-984: LGTM! Sound execution logic for immediate mode.The execution logic properly handles both cases:
- Block creator: trusts the header's AppHash (since transactions were already executed during creation)
- Syncing node: executes transactions to verify the AppHash matches
The AppHash verification and error handling are correct.
971-984: LGTM! Consistent delayed mode execution.The delayed mode logic correctly always executes transactions regardless of whether the node is the block creator, which aligns with the delayed execution semantics.
| var newStateRoot []byte | ||
|
|
||
| // Check if we need to execute transactions | ||
| if m.executionMode == coreexecutor.ExecutionModeImmediate { | ||
| // For immediate mode, the aggregator already executed during block creation | ||
| // But syncing nodes need to execute to verify the AppHash | ||
| // Check if we're the block creator by verifying we signed this block | ||
| isBlockCreator := false | ||
| if m.signer != nil { | ||
| myAddress, err := m.signer.GetAddress() | ||
| if err == nil && bytes.Equal(header.ProposerAddress, myAddress) { | ||
| // Also verify this is the current height we're producing | ||
| currentHeight, err := m.store.Height(ctx) | ||
| if err == nil && header.Height() == currentHeight+1 { | ||
| isBlockCreator = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | ||
| newStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | ||
| if err != nil { | ||
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | ||
| if isBlockCreator { | ||
| // We created this block, so we already executed transactions in execCreateBlock | ||
| // Just use the AppHash from the header | ||
| newStateRoot = header.AppHash | ||
| } else { | ||
| // We're syncing this block, need to execute to verify AppHash | ||
| rawTxs := make([][]byte, len(data.Txs)) | ||
| for i := range data.Txs { | ||
| rawTxs[i] = data.Txs[i] | ||
| } | ||
|
|
||
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | ||
| computedStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | ||
| if err != nil { | ||
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | ||
| } | ||
|
|
||
| // Verify the AppHash matches | ||
| if !bytes.Equal(header.AppHash, computedStateRoot) { | ||
| return types.State{}, fmt.Errorf("AppHash mismatch in immediate mode: expected %x, got %x", computedStateRoot, header.AppHash) | ||
| } | ||
| newStateRoot = computedStateRoot | ||
| } | ||
| } else { | ||
| // For delayed mode, always execute transactions | ||
| rawTxs := make([][]byte, len(data.Txs)) | ||
| for i := range data.Txs { | ||
| rawTxs[i] = data.Txs[i] | ||
| } | ||
|
|
||
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | ||
| var err error | ||
| newStateRoot, _, err = m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | ||
| if err != nil { | ||
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential logic issue in block creator detection.
The block creator detection logic has a potential race condition or timing issue. The check header.Height() == currentHeight+1 might not be reliable in all scenarios, especially during concurrent operations or if there are delays in processing.
Consider simplifying the block creator detection or adding additional safeguards.
- // Check if we're the block creator by verifying we signed this block
- isBlockCreator := false
- if m.signer != nil {
- myAddress, err := m.signer.GetAddress()
- if err == nil && bytes.Equal(header.ProposerAddress, myAddress) {
- // Also verify this is the current height we're producing
- currentHeight, err := m.store.Height(ctx)
- if err == nil && header.Height() == currentHeight+1 {
- isBlockCreator = true
- }
- }
- }
+ // Check if we're the block creator by verifying we signed this block
+ isBlockCreator := false
+ if m.signer != nil {
+ myAddress, err := m.signer.GetAddress()
+ if err == nil && bytes.Equal(header.ProposerAddress, myAddress) {
+ isBlockCreator = true
+ }
+ }The height check might be unnecessary since the proposer address check should be sufficient to determine if we created the block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var newStateRoot []byte | |
| // Check if we need to execute transactions | |
| if m.executionMode == coreexecutor.ExecutionModeImmediate { | |
| // For immediate mode, the aggregator already executed during block creation | |
| // But syncing nodes need to execute to verify the AppHash | |
| // Check if we're the block creator by verifying we signed this block | |
| isBlockCreator := false | |
| if m.signer != nil { | |
| myAddress, err := m.signer.GetAddress() | |
| if err == nil && bytes.Equal(header.ProposerAddress, myAddress) { | |
| // Also verify this is the current height we're producing | |
| currentHeight, err := m.store.Height(ctx) | |
| if err == nil && header.Height() == currentHeight+1 { | |
| isBlockCreator = true | |
| } | |
| } | |
| } | |
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | |
| newStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | |
| if err != nil { | |
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | |
| if isBlockCreator { | |
| // We created this block, so we already executed transactions in execCreateBlock | |
| // Just use the AppHash from the header | |
| newStateRoot = header.AppHash | |
| } else { | |
| // We're syncing this block, need to execute to verify AppHash | |
| rawTxs := make([][]byte, len(data.Txs)) | |
| for i := range data.Txs { | |
| rawTxs[i] = data.Txs[i] | |
| } | |
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | |
| computedStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | |
| if err != nil { | |
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | |
| } | |
| // Verify the AppHash matches | |
| if !bytes.Equal(header.AppHash, computedStateRoot) { | |
| return types.State{}, fmt.Errorf("AppHash mismatch in immediate mode: expected %x, got %x", computedStateRoot, header.AppHash) | |
| } | |
| newStateRoot = computedStateRoot | |
| } | |
| } else { | |
| // For delayed mode, always execute transactions | |
| rawTxs := make([][]byte, len(data.Txs)) | |
| for i := range data.Txs { | |
| rawTxs[i] = data.Txs[i] | |
| } | |
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | |
| var err error | |
| newStateRoot, _, err = m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | |
| if err != nil { | |
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | |
| } | |
| } | |
| var newStateRoot []byte | |
| // Check if we need to execute transactions | |
| if m.executionMode == coreexecutor.ExecutionModeImmediate { | |
| // For immediate mode, the aggregator already executed during block creation | |
| // But syncing nodes need to execute to verify the AppHash | |
| // Check if we're the block creator by verifying we signed this block | |
| isBlockCreator := false | |
| if m.signer != nil { | |
| myAddress, err := m.signer.GetAddress() | |
| if err == nil && bytes.Equal(header.ProposerAddress, myAddress) { | |
| isBlockCreator = true | |
| } | |
| } | |
| if isBlockCreator { | |
| // We created this block, so we already executed transactions in execCreateBlock | |
| // Just use the AppHash from the header | |
| newStateRoot = header.AppHash | |
| } else { | |
| // We're syncing this block, need to execute to verify AppHash | |
| rawTxs := make([][]byte, len(data.Txs)) | |
| for i := range data.Txs { | |
| rawTxs[i] = data.Txs[i] | |
| } | |
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | |
| computedStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | |
| if err != nil { | |
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | |
| } | |
| // Verify the AppHash matches | |
| if !bytes.Equal(header.AppHash, computedStateRoot) { | |
| return types.State{}, fmt.Errorf("AppHash mismatch in immediate mode: expected %x, got %x", computedStateRoot, header.AppHash) | |
| } | |
| newStateRoot = computedStateRoot | |
| } | |
| } else { | |
| // For delayed mode, always execute transactions | |
| rawTxs := make([][]byte, len(data.Txs)) | |
| for i := range data.Txs { | |
| rawTxs[i] = data.Txs[i] | |
| } | |
| ctx = context.WithValue(ctx, types.SignedHeaderContextKey, header) | |
| var err error | |
| newStateRoot, _, err = m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash) | |
| if err != nil { | |
| return types.State{}, fmt.Errorf("failed to execute transactions: %w", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In block/manager.go around lines 929 to 984, the block creator detection logic
uses a height check that may cause race conditions or timing issues. To fix
this, remove the height comparison and rely solely on verifying if the signer’s
address matches the header’s proposer address to determine block creator status.
This simplifies the logic and avoids potential concurrency problems.
|
merging this as the follow up evm tests will test the desired behaviour |
This reverts commit 1ca8457.
|
|
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview Simpler version of #2377, was reverted in #2394 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. Ex: Closes #<issue number> --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved block validation to enforce stricter checks on block timestamp and application hash consistency. * Enhanced error messages for validation failures to provide clearer feedback. * **Tests** * Re-enabled tests to verify block timestamp and application hash validation during block processing. * **Refactor** * Simplified transaction execution logic for improved performance by removing unnecessary locking during forkchoice state creation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
* 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
fixes #2250
this pr adds apphash verification based on the execution environment.
Summary by CodeRabbit