Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 276
feat: execution verification#2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b0a9eae6d79622a96fbfa21ed31e56d751e6153b003868e4b0ea18236322e1e073b11e1e6f42fe622dd5a65c5aa5680544a6981be1a8a6b5708a41b1aecb50f7ae532e9ae4feda041dec2775b36d3697a4b52a2aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| version: "2" | ||
| run: | ||
| modules-download-mode: readonly | ||
| build-tags: | ||
| - evm | ||
| - e2e | ||
| - docker | ||
| linters: | ||
| enable: | ||
| - errorlint | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -147,6 +147,8 @@ | ||
| metrics *Metrics | ||
| exec coreexecutor.Executor | ||
| // executionMode caches the execution mode from the executor | ||
| executionMode coreexecutor.ExecutionMode | ||
| // daIncludedHeight is rollkit height at which all blocks have been included | ||
| // in the DA | ||
| @@ -351,6 +353,8 @@ | ||
| daH := atomic.Uint64{} | ||
| daH.Store(s.DAHeight) | ||
| executionMode := exec.GetExecutionMode() | ||
| m := &Manager{ | ||
| signer: signer, | ||
| config: config, | ||
| @@ -380,6 +384,7 @@ | ||
| metrics: seqMetrics, | ||
| sequencer: sequencer, | ||
| exec: exec, | ||
| executionMode: executionMode, | ||
| da: da, | ||
| gasPrice: gasPrice, | ||
| gasMultiplier: gasMultiplier, | ||
| @@ -800,22 +805,26 @@ | ||
| } | ||
| // // Verify that the header's timestamp is strictly greater than the last block's time | ||
| // headerTime := header.Time() | ||
| // if header.Height() > 1 && lastState.LastBlockTime.After(headerTime) { | ||
| // return fmt.Errorf("block time must be strictly increasing: got %v, last block time was %v", | ||
| // headerTime.UnixNano(), lastState.LastBlockTime) | ||
| // } | ||
| // // Validate that the header's AppHash matches the lastState's AppHash | ||
| // // Note: Assumes deferred execution | ||
| // if !bytes.Equal(header.AppHash, lastState.AppHash) { | ||
| // return fmt.Errorf("app hash mismatch: expected %x, got %x", lastState.AppHash, header.AppHash) | ||
| // } | ||
| headerTime := header.Time() | ||
| if header.Height() > 1 && lastState.LastBlockTime.After(headerTime) { | ||
| return fmt.Errorf("block time must be strictly increasing: got %v, last block time was %v", | ||
| headerTime.UnixNano(), lastState.LastBlockTime) | ||
| } | ||
| // Validate AppHash based on execution mode | ||
| if m.executionMode == coreexecutor.ExecutionModeDelayed { | ||
| // In delayed mode, AppHash should match the last state's AppHash | ||
| if !bytes.Equal(header.AppHash, lastState.AppHash) { | ||
| return fmt.Errorf("appHash mismatch in delayed mode: expected %x, got %x", lastState.AppHash, header.AppHash) | ||
| } | ||
| } | ||
| // Note: For immediate mode, we can't validate AppHash against lastState because | ||
| // it represents the state after executing the current block's transactions | ||
| return nil | ||
| } | ||
| func (m *Manager) execCreateBlock(_ context.Context, height uint64, lastSignature *types.Signature, lastHeaderHash types.Hash, _ types.State, batchData *BatchData) (*types.SignedHeader, *types.Data, error) { | ||
| func (m *Manager) execCreateBlock(ctx context.Context, height uint64, lastSignature *types.Signature, lastHeaderHash types.Hash, lastState types.State, batchData *BatchData) (*types.SignedHeader, *types.Data, error) { | ||
| // Use when batchData is set to data IDs from the DA layer | ||
| // batchDataIDs := convertBatchDataToBytes(batchData.Data) | ||
| @@ -840,21 +849,36 @@ | ||
| // Determine if this is an empty block | ||
| isEmpty := batchData.Batch == nil || len(batchData.Transactions) == 0 | ||
| // Determine AppHash based on execution mode | ||
| var appHash []byte | ||
| if m.executionMode == coreexecutor.ExecutionModeImmediate { | ||
| // Execute transactions | ||
| newStateRoot, _, err := m.exec.ExecuteTxs(ctx, batchData.Transactions, height, batchData.Time, lastState.AppHash) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to execute transactions for immediate mode: %w", err) | ||
| } | ||
| appHash = newStateRoot | ||
| } else { | ||
| // For delayed execution, use the app hash from last state | ||
| appHash = lastState.AppHash | ||
| } | ||
| header := &types.SignedHeader{ | ||
| Header: types.Header{ | ||
| Version: types.Version{ | ||
| Block: m.lastState.Version.Block, | ||
| App: m.lastState.Version.App, | ||
| Block: lastState.Version.Block, | ||
| App: lastState.Version.App, | ||
| }, | ||
| BaseHeader: types.BaseHeader{ | ||
| ChainID: m.lastState.ChainID, | ||
| ChainID: lastState.ChainID, | ||
| Height: height, | ||
| Time: uint64(batchData.UnixNano()), //nolint:gosec // why is time unix? (tac0turtle) | ||
| }, | ||
| LastHeaderHash: lastHeaderHash, | ||
| // DataHash is set at the end of the function | ||
| // DataHash is set below | ||
| ConsensusHash: make(types.Hash, 32), | ||
| AppHash: m.lastState.AppHash, | ||
| AppHash: appHash, | ||
| ProposerAddress: m.genesis.ProposerAddress, | ||
| }, | ||
| Signature: *lastSignature, | ||
| @@ -869,7 +893,7 @@ | ||
| Txs: make(types.Txs, 0), // Start with empty transaction list | ||
| } | ||
| // Only add transactions if this is not an empty block | ||
| // Set DataHash | ||
| if !isEmpty { | ||
| blockData.Txs = make(types.Txs, len(batchData.Transactions)) | ||
| for i := range batchData.Transactions { | ||
| @@ -884,15 +908,61 @@ | ||
| } | ||
| func (m *Manager) execApplyBlock(ctx context.Context, lastState types.State, header *types.SignedHeader, data *types.Data) (types.State, error) { | ||
| rawTxs := make([][]byte, len(data.Txs)) | ||
| for i := range data.Txs { | ||
| rawTxs[i] = data.Txs[i] | ||
| } | ||
| 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 | ||
| } | ||
Comment on lines
+915
to
+952
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainComplex branching logic needs careful testing. The logic for determining whether to execute transactions in immediate mode is complex with multiple conditions. Consider adding unit tests to cover all branches, especially:
Based on the static analysis, these execution paths lack test coverage. Would you like me to help generate comprehensive unit tests for the immediate execution mode? 🏁 Script executed: #!/bin/bash# Search for unit tests covering the immediate execution mode branching in block/manager.go# 1. Tests mentioning "immediate mode"
rg -n "immediate mode" --glob "*_test.go"# 2. Tests invoking ExecuteTxs
rg -n "ExecuteTxs" --glob "*_test.go"# 3. Tests referencing signer.GetAddress
rg -n "GetAddress" --glob "*_test.go"# 4. Tests referencing store.Height
rg -n "Height(" --glob "*_test.go"Length of output: 5308 Add unit tests covering immediate‐mode branching in block/manager.go The complex logic in block/manager.go (lines 924–961) isn’t directly exercised by any existing tests. Please add unit tests around the following paths to ensure full coverage: • Block creator path (m.signer.GetAddress succeeds, store.Height returns currentHeight, header.Height()==currentHeight+1 → newStateRoot = header.AppHash; ExecuteTxs must not be called) Locations needing tests:
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 924-935: block/manager.go#L924-L935 [warning] 939-948: block/manager.go#L939-L948 [warning] 950-954: block/manager.go#L950-L954 [warning] 957-960: block/manager.go#L957-L960 🤖 Prompt for AI Agents | ||
| } else { | ||
| // For delayed mode, always execute transactions | ||
| rawTxs := make([][]byte, len(data.Txs)) | ||
| for i := range data.Txs { | ||
| rawTxs[i] = data.Txs[i] | ||
| } | ||
Comment on lines
+955
to
+958
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | ||
| 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) | ||
| } | ||
| } | ||
tac0turtle marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| s, err := lastState.NextState(header, newStateRoot) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -118,7 +118,7 @@ func submitToDA[T any]( | ||
| switch res.Code { | ||
| case coreda.StatusSuccess: | ||
| m.logger.Info(fmt.Sprintf("successfully submitted %s to DA layer", itemType), "gasPrice", gasPrice, "count", res.SubmittedCount) | ||
| m.logger.Info(fmt.Sprintf("successfully submitted %s to DA layer,", itemType), "gasPrice", gasPrice, "count", res.SubmittedCount) | ||
tac0turtle marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if res.SubmittedCount == uint64(remLen) { | ||
| submittedAll = true | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.