feat: execution verification #2415
Conversation
WalkthroughThis change introduces an execution mode abstraction for block execution, distinguishing between delayed and immediate execution. The Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Executor
participant State
Manager->>Executor: GetExecutionMode()
Executor-->>Manager: ExecutionMode (Delayed/Immediate)
Manager->>Manager: Cache executionMode
alt Block Creation
Manager->>Executor: ExecuteTxs (if Immediate mode & non-empty block)
Executor-->>Manager: New StateRoot (AppHash)
Manager->>State: Update with new AppHash
else Delayed mode or empty block
Manager->>State: Use last AppHash
end
alt Block Validation
Manager->>State: Compare AppHash (mode-dependent)
end
alt Block Application
Manager->>Executor: ExecuteTxs (if required by mode)
Executor-->>Manager: New StateRoot (AppHash)
Manager->>State: Advance state
end
Possibly related PRs
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ 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).
|
![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 @Manav-Aggarwal, 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 significant enhancement to Rollkit's execution layer by implementing a flexible framework for different 'execution modes.' This allows Rollkit to seamlessly integrate with applications that handle state transitions in various ways, specifically supporting both 'delayed' (post-block execution) and 'immediate' (in-block execution) models. The core changes involve extending the Executor interface, adapting the block.Manager's logic to respect these modes, and updating existing executor implementations to declare their supported mode, ensuring proper AppHash verification and transaction processing across the system.
Highlights
- Introduced Execution Modes: I've added a new
ExecutionModeenum (Delayed,Immediate,Optimistic) within thecore/executionpackage. This allows for explicit declaration of how an application's state transitions are handled relative to block creation. - Executor Interface Extension: The
core/execution.Executorinterface now includes aGetExecutionMode()method. This enables executors to communicate their supported execution model to the Rollkit consensus layer. - Dynamic Block Processing in Manager: The
block.Managerhas been updated to dynamically adapt its block validation (execValidate), block creation (execCreateBlock), and block application (execApplyBlock) logic based on theExecutionModereported by the connected executor. This ensures correctAppHashhandling and transaction execution timing. - AppHash Validation Logic Refinement: The
execValidatefunction now conditionally validates theAppHash. InDelayedmode, it verifies that the block header'sAppHashmatches the previous state'sAppHash. ForImmediatemode, this check is skipped during validation as theAppHashrepresents the state after the current block's transactions. - Transaction Execution Timing Adjustment: In
execCreateBlock, transactions are now executed before block finalization forImmediatemode to derive the correctAppHashfor the block header. InexecApplyBlock, syncing nodes inImmediatemode will execute transactions to verify theAppHashagainst the one in the block header, while the block creator skips re-execution as it already performed this step. - Executor Implementations Updated: I've configured
KVExecutorandDummyExecutorto operate inExecutionModeDelayed, aligning with their current state transition behavior. Conversely, theEngineClient(EVM executor) is now explicitly set toExecutionModeImmediate, reflecting its ability to calculate the state root during block production. - Module Dependency Management: I've updated
go.modandgo.sumfiles forapps/testappandexecution/evmto correctly manage dependencies, including adding areplacedirective inexecution/evm/go.modto point to the localcoremodule, which is typical for monorepo development.
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 is currently in preview and 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 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 the concept of ExecutionMode to support both delayed and immediate transaction execution models. The changes are well-structured across the affected modules. My review focuses on improving the maintainability and clarity of the new logic in the block.Manager, particularly around reducing code duplication and clarifying error messages.
| rawTxs := make([][]byte, len(data.Txs)) | ||
| for i := range data.Txs { | ||
| rawTxs[i] = data.Txs[i] | ||
| } |
There was a problem hiding this comment.
This block of code to convert types.Txs to [][]byte is duplicated from lines 945-948 in this same function, and is also present in execCreateBlock. Consider extracting this logic into a helper function to improve maintainability and reduce code duplication.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/execution/dummy.go (1)
94-98: LGTM! Add test coverage for the new method.The
GetExecutionModemethod implementation is correct and well-documented. However, ensure test coverage is added for this new method.Would you like me to help generate test cases for the
GetExecutionModemethod?block/manager.go (1)
867-883: Consider extracting immediate mode execution logic.The transaction execution logic for immediate mode could be extracted into a helper method to improve readability and reusability, as similar logic appears in
execApplyBlock.+func (m *Manager) executeTransactionsImmediate(ctx context.Context, txs types.Txs, height uint64, timestamp time.Time, prevStateRoot []byte) ([]byte, error) { + rawTxs := make([][]byte, len(txs)) + for i := range txs { + rawTxs[i] = txs[i] + } + + newStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, height, timestamp, prevStateRoot) + if err != nil { + return nil, fmt.Errorf("failed to execute transactions: %w", err) + } + return newStateRoot, nil +} // Determine AppHash based on execution mode var appHash []byte if m.executionMode == coreexecutor.ExecutionModeImmediate && !isEmpty { // For immediate execution, execute transactions now to get the new state root - rawTxs := make([][]byte, len(blockData.Txs)) - for i := range blockData.Txs { - rawTxs[i] = blockData.Txs[i] - } - - // Execute transactions - newStateRoot, _, err := m.exec.ExecuteTxs(ctx, rawTxs, height, batchData.Time, lastState.AppHash) + newStateRoot, err := m.executeTransactionsImmediate(ctx, blockData.Txs, height, batchData.Time, lastState.AppHash) if err != nil { return nil, nil, fmt.Errorf("failed to execute transactions for immediate mode: %w", err) } appHash = newStateRoot
📜 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 (10)
apps/testapp/go.mod(1 hunks)apps/testapp/kv/kvexecutor.go(2 hunks)block/manager.go(7 hunks)block/manager_test.go(1 hunks)block/publish_block_p2p_test.go(2 hunks)core/execution/dummy.go(1 hunks)core/execution/execution.go(3 hunks)execution/evm/execution.go(1 hunks)execution/evm/go.mod(1 hunks)test/mocks/execution.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
**/*.go: Follow standard Go formatting (enforced by golangci-lint).
Use meaningful variable names in Go code.
Keep functions small and focused in Go code.
Document exported types and functions in Go code.
Use context.Context for cancellation in Go code.
Wrap errors with context usingfmt.Errorfin Go code.
Return errors early in Go code.
Use custom error types for domain-specific errors in Go code.
Use structured logging and include relevant context in log messages in Go code.
Use appropriate log levels in Go code.
Never expose private keys in logs or errors.
Validate all inputs from external sources.
Use secure random number generation in Go code.
Be careful with concurrent access to shared state in Go code.
Be mindful of goroutine leaks in Go code.
Use buffered channels appropriately in Go code.
Profile before optimizing Go code.
Consider memory allocation in hot paths in Go code.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
apps/testapp/kv/kvexecutor.goblock/publish_block_p2p_test.goexecution/evm/execution.gocore/execution/dummy.goblock/manager_test.gocore/execution/execution.gotest/mocks/execution.goblock/manager.go
`**/*_test.go`: Unit tests should be placed in `*_test.go` files alongside the code being tested.
**/*_test.go: Unit tests should be placed in*_test.gofiles alongside the code being tested.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
block/publish_block_p2p_test.goblock/manager_test.go
`**/mocks/**/*`: Mocks should be placed in `mocks/` directories and generated using mockery.
**/mocks/**/*: Mocks should be placed inmocks/directories and generated using mockery.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/mocks/execution.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-06-10T19:23:16.839Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
apps/testapp/go.mod (3)
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
apps/testapp/kv/kvexecutor.go (3)
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
execution/evm/go.mod (4)
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors, so no error handling is needed when calling these methods.
block/publish_block_p2p_test.go (3)
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
block/manager_test.go (4)
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-06-10T19:23:16.839Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors, so no error handling is needed when calling these methods.
core/execution/execution.go (3)
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-06-10T19:23:16.839Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use context.Context for cancellation in Go code.
test/mocks/execution.go (5)
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use context.Context for cancellation in Go code.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors, so no error handling is needed when calling these methods.
block/manager.go (2)
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-06-10T19:23:16.839Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
🧬 Code Graph Analysis (5)
apps/testapp/kv/kvexecutor.go (1)
core/execution/execution.go (2)
ExecutionMode(9-9)ExecutionModeDelayed(14-14)
block/publish_block_p2p_test.go (1)
core/execution/execution.go (2)
ExecutionMode(9-9)ExecutionModeDelayed(14-14)
execution/evm/execution.go (1)
core/execution/execution.go (2)
ExecutionMode(9-9)ExecutionModeImmediate(18-18)
core/execution/dummy.go (1)
core/execution/execution.go (2)
ExecutionMode(9-9)ExecutionModeDelayed(14-14)
test/mocks/execution.go (1)
core/execution/execution.go (1)
ExecutionMode(9-9)
🪛 GitHub Check: codecov/patch
core/execution/dummy.go
[warning] 96-97: core/execution/dummy.go#L96-L97
Added lines #L96 - L97 were not covered by tests
block/manager.go
[warning] 868-872: block/manager.go#L868-L872
Added lines #L868 - L872 were not covered by tests
[warning] 875-879: block/manager.go#L875-L879
Added lines #L875 - L879 were not covered by tests
[warning] 924-935: block/manager.go#L924-L935
Added lines #L924 - L935 were not covered by tests
[warning] 939-948: block/manager.go#L939-L948
Added lines #L939 - L948 were not covered by tests
[warning] 950-954: block/manager.go#L950-L954
Added lines #L950 - L954 were not covered by tests
[warning] 957-960: block/manager.go#L957-L960
Added lines #L957 - L960 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Run E2E System Tests
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (13)
execution/evm/go.mod (1)
3-7: LGTM! Module configuration supports local development.The Go version bump and local replace directive appropriately support development of the execution mode framework. The replace directive enables testing changes to the core execution module locally.
apps/testapp/go.mod (1)
16-16: LGTM! Dependency change reflects direct usage.Moving the core dependency from indirect to direct is appropriate since the testapp now directly uses the execution mode interfaces from the core module.
block/publish_block_p2p_test.go (2)
23-23: LGTM! Import supports execution mode testing.The import of the core execution package with proper aliasing enables the mock executor to implement the execution mode interface.
240-242: LGTM! Mock executor properly implements execution mode.The
GetExecutionModemethod implementation is correct and consistent with other test mocks. ReturningExecutionModeDelayedenables testing of execution mode-dependent logic paths.apps/testapp/kv/kvexecutor.go (2)
13-13: LGTM! Import enables execution mode functionality.The import of the execution package is clean and enables the KVExecutor to implement the execution mode interface.
247-251: LGTM! Execution mode implementation is correct.The
GetExecutionModemethod is properly implemented with clear documentation. ReturningExecutionModeDelayedis appropriate for the KVExecutor and consistent with the delayed execution pattern used by other similar executors.execution/evm/execution.go (1)
305-310: LGTM!The implementation correctly returns
ExecutionModeImmediatefor the EVM executor, which aligns with how the engine API calculates state roots during payload creation.block/manager_test.go (1)
578-597: Tests correctly re-enabled with updated error messages.The re-enabled tests properly validate the block time monotonicity and AppHash consistency checks that are now enforced in
execValidatebased on execution mode.core/execution/execution.go (2)
8-23: Well-designed execution mode abstraction.The
ExecutionModetype and constants provide a clean way to distinguish between different state transition timing strategies. The documentation clearly explains each mode's behavior.
104-115: Clear interface extension with proper requirements.The
GetExecutionModemethod is well-documented with explicit requirements about consistency and immutability, which is crucial for correct block processing behavior.test/mocks/execution.go (1)
134-176: Correct mock implementation for GetExecutionMode.The mock follows the standard mockery pattern and provides all necessary helper methods for test setup.
block/manager.go (2)
151-152: Proper caching of execution mode.Good design decision to cache the execution mode during initialization rather than calling
GetExecutionMode()repeatedly.Also applies to: 356-357, 387-387
808-823: Correct validation logic based on execution mode.The re-enabled validation checks are properly implemented:
- Block time monotonicity is enforced for blocks with height > 1
- AppHash validation is correctly conditional on delayed execution mode
| // 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 | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Complex 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:
- Block creator path (trusting header's AppHash)
- Syncing node path (verifying AppHash)
- Error cases in address/height retrieval
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)
• Syncing path (any of the signer/height checks fail → ExecuteTxs executes all Txs, matches header.AppHash)
• Error cases:
– signer.GetAddress returns an error
– store.Height returns an error
– exec.ExecuteTxs returns an error (propagate failure)
– AppHash mismatch between header and computedStateRoot
Locations needing tests:
- block/manager.go:924–961
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 924-935: block/manager.go#L924-L935
Added lines #L924 - L935 were not covered by tests
[warning] 939-948: block/manager.go#L939-L948
Added lines #L939 - L948 were not covered by tests
[warning] 950-954: block/manager.go#L950-L954
Added lines #L950 - L954 were not covered by tests
[warning] 957-960: block/manager.go#L957-L960
Added lines #L957 - L960 were not covered by tests
🤖 Prompt for AI Agents
In block/manager.go between lines 924 and 961, add unit tests to cover all
branches of the immediate-mode logic. Create tests for the block creator path
where m.signer.GetAddress and store.Height succeed and header.Height equals
currentHeight+1, ensuring newStateRoot is set to header.AppHash and ExecuteTxs
is not called. Add tests for the syncing path where any signer or height check
fails, verifying ExecuteTxs runs all transactions and the computed AppHash
matches header.AppHash. Also, include tests for error cases where
signer.GetAddress returns an error, store.Height returns an error,
exec.ExecuteTxs returns an error (which should propagate), and where there is an
AppHash mismatch between header and computedStateRoot.
Manav-Aggarwal
force-pushed
the
marko/2250
branch
from
7bef5ae to
1e6f42f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2415 +/- ##
==========================================
- Coverage 72.71% 72.29% -0.42%
==========================================
Files 67 67
Lines 6395 6465 +70
==========================================
+ Hits 4650 4674 +24
- Misses 1347 1390 +43
- Partials 398 401 +3
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/testapp/go.mod(1 hunks)block/da_includer.go(1 hunks)block/manager.go(7 hunks)block/manager_test.go(1 hunks)block/publish_block_p2p_test.go(2 hunks)block/submitter.go(1 hunks)block/sync.go(1 hunks)execution/evm/execution.go(3 hunks)scripts/run.go(1 hunks)scripts/test.mk(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- block/da_includer.go
- block/sync.go
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/testapp/go.mod
- execution/evm/execution.go
- block/publish_block_p2p_test.go
- block/manager_test.go
- block/manager.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
**/*.go: Follow standard Go formatting (enforced by golangci-lint).
Use meaningful variable names in Go code.
Keep functions small and focused in Go code.
Document exported types and functions in Go code.
Use context.Context for cancellation in Go code.
Wrap errors with context usingfmt.Errorfin Go code.
Return errors early in Go code.
Use custom error types for domain-specific errors in Go code.
Use structured logging and include relevant context in log messages in Go code.
Use appropriate log levels in Go code.
Never expose private keys in logs or errors.
Validate all inputs from external sources.
Use secure random number generation in Go code.
Be careful with concurrent access to shared state in Go code.
Be mindful of goroutine leaks in Go code.
Use buffered channels appropriately in Go code.
Profile before optimizing Go code.
Consider memory allocation in hot paths in Go code.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
block/submitter.goscripts/run.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-06-10T19:23:16.839Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
scripts/test.mk (6)
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to test/e2e/**/*_test.go : End-to-end (E2E) tests should be placed in the `test/e2e/` directory.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use appropriate log levels in Go code.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*_test.go : Unit tests should be placed in `*_test.go` files alongside the code being tested.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use meaningful variable names in Go code.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to test/integration/**/*_test.go : Integration tests should be placed in the `test/integration/` directory.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: All tests must pass (`make test-all`).
block/submitter.go (1)
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Run E2E System Tests
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
scripts/test.mk (1)
27-27: LGTM! Consistent verbose output for debugging.Adding the
-vflag to enable verbose output during e2e tests is a good practice, especially when introducing new execution mode features. This change is consistent with other test targets in the file that already use verbose output.scripts/run.go (1)
258-258: Verify thenet-infointegration and output format consistencyPlease ensure that the switch from
node-infotonet-infois fully supported and that your parsing logic remains valid:
- Confirm the
net-infocommand is defined inpkg/cmd/p2p.go(Use:"net-info")- Verify invocations in:
scripts/run.go(around line 258)scripts/run-evm-nodes.go- E2E helper
test/e2e/evm_test_common.go- Make sure the output still includes the
🔗 Full Address:marker exactly as before so the address‐extraction code continues to work- This change should align with the execution verification feature introduced in this PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/run-evm-nodes.go (3)
397-401: Define wait times as constants for better maintainability.The magic numbers for wait times should be defined as constants at the package level.
Add these constants at the top of the file:
const ( sequencerInitWaitTime = 5 * time.Second fullNodeInitWaitTime = 3 * time.Second )Then update the code:
-waitTime := 5 * time.Second -if !node.isSequencer { - waitTime = 3 * time.Second -} +waitTime := sequencerInitWaitTime +if !node.isSequencer { + waitTime = fullNodeInitWaitTime +}
413-439: Consider making the P2P address parsing more robust.The current parsing logic is somewhat fragile and depends on specific output format. Consider using a more structured approach or adding validation.
Consider extracting this logic into a separate function and adding more validation:
func extractP2PAddress(netInfoOutput []byte) (string, error) { lines := strings.Split(string(netInfoOutput), "\n") // Define a regex pattern for P2P addresses p2pPattern := regexp.MustCompile(`/ip4/\d+\.\d+\.\d+\.\d+/tcp/\d+/p2p/\w+`) for _, line := range lines { if strings.Contains(line, "Addr:") { if matches := p2pPattern.FindStringSubmatch(line); len(matches) > 0 { return stripANSI(matches[0]), nil } } } return "", fmt.Errorf("no valid P2P address found in output") }
299-446: Consider breaking down this large method for better maintainability.The
startNodemethod is handling multiple responsibilities and is quite long (147 lines). Consider extracting logical sections into separate methods.Break down the method into smaller, focused functions:
func (nm *nodeManager) startNode(node *nodeConfig, sequencerP2PAddr string) (string, error) { if err := nm.initializeNode(node); err != nil { return "", err } if !node.isSequencer && len(nm.nodes) > 0 { if err := nm.copyGenesisFromSequencer(node); err != nil { return "", err } } jwtContent, err := os.ReadFile(nm.jwtPath) if err != nil { return "", fmt.Errorf("failed to read JWT secret: %w", err) } if err := nm.runNode(node, sequencerP2PAddr, string(jwtContent)); err != nil { return "", err } if node.isSequencer { return nm.getNodeP2PAddress(node) } return "", nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/run-evm-nodes.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
**/*.go: Follow standard Go formatting (enforced by golangci-lint).
Use meaningful variable names in Go code.
Keep functions small and focused in Go code.
Document exported types and functions in Go code.
Use context.Context for cancellation in Go code.
Wrap errors with context usingfmt.Errorfin Go code.
Return errors early in Go code.
Use custom error types for domain-specific errors in Go code.
Use structured logging and include relevant context in log messages in Go code.
Use appropriate log levels in Go code.
Never expose private keys in logs or errors.
Validate all inputs from external sources.
Use secure random number generation in Go code.
Be careful with concurrent access to shared state in Go code.
Be mindful of goroutine leaks in Go code.
Use buffered channels appropriately in Go code.
Profile before optimizing Go code.
Consider memory allocation in hot paths in Go code.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
scripts/run-evm-nodes.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-06-10T19:23:16.839Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
scripts/run-evm-nodes.go (2)
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors, so no error handling is needed when calling these methods.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Go Mod Tidy Check
- GitHub Check: test / Run Integration Tests
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / markdown-lint
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (6)
scripts/run-evm-nodes.go (6)
24-40: LGTM! Well-structured constants with clear separation.The base ports and offsets are appropriately defined. The 10-port offset provides sufficient spacing between nodes to avoid conflicts.
42-51: LGTM! Clean and comprehensive node configuration structure.The
nodeConfigstruct effectively encapsulates all necessary per-node settings with clear field names.
67-87: LGTM! Proper flag handling and validation.Good validation of the nodes parameter and proper initialization of the slice with capacity hint for performance.
214-236: LGTM! Clean implementation of dynamic node configuration.The port offset calculation ensures no conflicts between nodes, and the naming convention clearly distinguishes between sequencer and full nodes.
138-143: LGTM! Clean iteration with proper error handling.The loop properly starts EVM execution layers for all nodes with descriptive error messages.
335-349: Improve file handling to prevent potential resource leaks.The current implementation might not close files if an error occurs before the defer statements. Consider restructuring to ensure proper cleanup.
-srcFile, err := os.Open(sequencerGenesis) -if err != nil { - return "", fmt.Errorf("failed to open sequencer genesis: %w", err) -} -defer srcFile.Close() - -dstFile, err := os.Create(nodeGenesis) -if err != nil { - return "", fmt.Errorf("failed to create %s genesis: %w", node.name, err) -} -defer dstFile.Close() - -if _, err := io.Copy(dstFile, srcFile); err != nil { - return "", fmt.Errorf("failed to copy genesis: %w", err) -} +if err := copyFile(sequencerGenesis, nodeGenesis); err != nil { + return "", fmt.Errorf("failed to copy genesis for %s: %w", node.name, err) +}Consider adding a helper function:
func copyFile(src, dst string) error { srcFile, err := os.Open(src) if err != nil { return err } defer srcFile.Close() dstFile, err := os.Create(dst) if err != nil { return err } defer dstFile.Close() _, err = io.Copy(dstFile, srcFile) return err }⛔ Skipped due to learnings
Learnt from: CR PR: rollkit/rollkit#0 File: CLAUDE.md:0-0 Timestamp: 2025-06-30T08:47:52.948Z Learning: Applies to **/*.go : Be mindful of goroutine leaks in Go code.Learnt from: CR PR: rollkit/rollkit#0 File: CLAUDE.md:0-0 Timestamp: 2025-06-30T08:47:52.948Z Learning: Applies to **/*.go : Be careful with concurrent access to shared state in Go code.Learnt from: CR PR: rollkit/rollkit#0 File: CLAUDE.md:0-0 Timestamp: 2025-06-30T08:47:52.948Z Learning: Applies to **/*.go : Consider memory allocation in hot paths in Go code.
|
|
||
| // Remove existing directory | ||
| os.RemoveAll(sequencerHome) | ||
| os.RemoveAll(node.homeDir) |
There was a problem hiding this comment.
Add error handling for directory removal.
The error from os.RemoveAll should be handled, even if just logged for debugging purposes.
-os.RemoveAll(node.homeDir)
+if err := os.RemoveAll(node.homeDir); err != nil {
+ log.Printf("Warning: failed to remove existing directory %s: %v", node.homeDir, err)
+}📝 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.
| os.RemoveAll(node.homeDir) | |
| if err := os.RemoveAll(node.homeDir); err != nil { | |
| log.Printf("Warning: failed to remove existing directory %s: %v", node.homeDir, err) | |
| } |
🤖 Prompt for AI Agents
In scripts/run-evm-nodes.go at line 305, the call to os.RemoveAll(node.homeDir)
currently ignores any error returned. Modify this line to capture the error
returned by os.RemoveAll and handle it by logging the error message for
debugging purposes, ensuring that any failure to remove the directory is
recorded.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
block/manager.go (2)
924-961: Complex execution mode logic needs comprehensive test coverage.The branching logic for immediate mode execution (block creator vs syncing node paths) is complex and lacks test coverage as noted in previous reviews. This includes:
- Block creator detection via signer address and height validation
- Conditional execution based on node role
- AppHash verification for syncing nodes
The static analysis confirms insufficient test coverage for these critical execution paths. Please add unit tests covering:
- Block creator path (trusting header AppHash)
- Syncing node path (executing and verifying AppHash)
- Error scenarios in address/height retrieval
- AppHash mismatch conditions
869-872: Address code duplication as flagged in previous reviews.This transaction conversion logic is duplicated in multiple locations as noted in previous reviews. The pattern appears again here and in lines 945-948 and 964-967.
Extract this repeated logic into a helper function:
func convertTxsToRawTxs(txs types.Txs) [][]byte { rawTxs := make([][]byte, len(txs)) for i := range txs { rawTxs[i] = txs[i] } return rawTxs }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
block/da_includer.go(4 hunks)block/manager.go(7 hunks)execution/evm/execution.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/evm/execution.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
**/*.go: Follow standard Go formatting (enforced by golangci-lint).
Use meaningful variable names in Go code.
Keep functions small and focused in Go code.
Document exported types and functions in Go code.
Use context.Context for cancellation in Go code.
Wrap errors with context usingfmt.Errorfin Go code.
Return errors early in Go code.
Use custom error types for domain-specific errors in Go code.
Use structured logging and include relevant context in log messages in Go code.
Use appropriate log levels in Go code.
Never expose private keys in logs or errors.
Validate all inputs from external sources.
Use secure random number generation in Go code.
Be careful with concurrent access to shared state in Go code.
Be mindful of goroutine leaks in Go code.
Use buffered channels appropriately in Go code.
Profile before optimizing Go code.
Consider memory allocation in hot paths in Go code.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
block/manager.goblock/da_includer.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-06-10T19:23:16.839Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
block/manager.go (5)
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-06-10T19:23:16.839Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: S1nus
PR: rollkit/rollkit#1374
File: block/manager.go:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: The overriding of `DataHash` and `Commit` in `block/manager.go` is necessary for the block to pass basic validation when `applyBlock` is called. The user has added a comment to document this requirement.
Learnt from: S1nus
PR: rollkit/rollkit#1368
File: types/utils.go:128-130
Timestamp: 2024-10-08T18:35:32.960Z
Learning: The `ProposerPubkey` field in the `SignedHeader` struct is of type `[]byte` and not `ed25519.PubKey`, which requires conversion before using `Address().Bytes()` method.
Learnt from: S1nus
PR: rollkit/rollkit#1368
File: types/utils.go:128-130
Timestamp: 2024-06-10T19:23:16.839Z
Learning: The `ProposerPubkey` field in the `SignedHeader` struct is of type `[]byte` and not `ed25519.PubKey`, which requires conversion before using `Address().Bytes()` method.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use context.Context for cancellation in Go code.
block/da_includer.go (4)
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:88-88
Timestamp: 2025-01-27T22:03:14.288Z
Learning: In the rollkit codebase, the `github.com/rollkit/go-da` package is imported with different aliases in test (`da`) and production code (`goDA`), but they refer to the same package. Suggestions to change between these aliases are unnecessary as they don't affect functionality.
Learnt from: tzdybal
PR: rollkit/rollkit#1875
File: da/da_test.go:178-178
Timestamp: 2025-01-27T22:02:44.446Z
Learning: In the test files of rollkit/rollkit repository, the `github.com/rollkit/go-da` package is imported with the alias `da`, while in production code it might use a different alias. The code should maintain consistent import aliases within each file.
Learnt from: tac0turtle
PR: rollkit/rollkit#2221
File: types/da.go:26-27
Timestamp: 2025-04-28T07:23:13.738Z
Learning: The hardcoded placeholder namespace (`[]byte("placeholder")`) in the DA helper functions is intentional as a temporary solution to avoid breaking the API until upstream fixes in the node component can be implemented. This is part of a deliberate incremental refactoring strategy.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Return errors early in Go code.
🧬 Code Graph Analysis (1)
block/manager.go (7)
core/execution/execution.go (3)
ExecutionMode(9-9)ExecutionModeDelayed(14-14)ExecutionModeImmediate(18-18)types/data.go (3)
Signature(46-46)Data(33-36)Version(16-19)types/header.go (3)
Hash(14-14)Header(35-62)BaseHeader(25-32)types/state.go (1)
State(20-39)types/signed_header.go (1)
SignedHeader(47-54)types/pb/rollkit/v1/rollkit.pb.go (12)
SignedHeader(233-240)SignedHeader(253-253)SignedHeader(268-270)Data(422-428)Data(441-441)Data(456-458)Header(82-116)Header(129-129)Header(144-146)Version(29-35)Version(48-48)Version(63-65)types/tx.go (2)
Txs(7-7)Tx(4-4)
🪛 GitHub Check: lint / golangci-lint
block/da_includer.go
[failure] 86-86:
File is not properly formatted (gofmt)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Run Integration Tests
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (6)
block/da_includer.go (1)
61-64: Good retry-friendly error handling for execution engine synchronization.The special handling for "not yet indexed" errors appropriately converts what would be a hard failure into a retry scenario, allowing the DA includer to gracefully handle timing issues when the execution engine hasn't yet indexed a block. This aligns well with the caching and retry logic mentioned in the AI summary.
block/manager.go (5)
150-151: LGTM: Clean execution mode integration.The execution mode caching approach is well-designed:
- Fetches mode once during Manager construction for efficiency
- Stores it as a field for easy access throughout the lifecycle
- Eliminates repeated calls to
exec.GetExecutionMode()Also applies to: 356-356, 387-387
950-950: Ensure consistent context usage across execution modes.The
SignedHeaderContextKeyis properly added to context in both execution paths, which addresses the consistency concern raised in previous reviews.Also applies to: 969-969
827-827: Good refactor: Pass lastState explicitly for better testability.Changing the signature to accept
lastStateas a parameter improves testability and makes dependencies explicit, which is a positive architectural improvement.
808-823: AppHash validation is consistently appliedI verified across the repository that:
- Delayed mode enforces
header.AppHash == lastState.AppHash- Immediate mode enforces
header.AppHash == computedStateRootNo other code paths perform AppHash checks, and tests cover the delayed-mode mismatch as well as the publish-block AppHash. No inconsistencies found—approving these changes.
866-883: ExecuteTxs parameter consistency verifiedBoth in block creation and block application,
ExecuteTxsis called with the same three key parameters—block height, timestamp, and previous state root:
- execCreateBlock (block/manager.go:872–875):
m.exec.ExecuteTxs(ctx, rawTxs, height, batchData.Time, lastState.AppHash)- execApplyBlock (block/manager.go:951–954 and 969–974):
m.exec.ExecuteTxs(ctx, rawTxs, header.Height(), header.Time(), lastState.AppHash)No discrepancies found—no further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/run-evm-nodes.go (1)
305-305: Add error handling for directory removal.The error from
os.RemoveAllshould be handled, even if just logged for debugging purposes.-os.RemoveAll(node.homeDir) +if err := os.RemoveAll(node.homeDir); err != nil { + log.Printf("Warning: failed to remove existing directory %s: %v", node.homeDir, err) +}
🧹 Nitpick comments (2)
scripts/run-evm-nodes.go (2)
71-77: Add validation for maximum reasonable number of nodes.While validating for minimum nodes is good, consider adding an upper bound to prevent resource exhaustion or unrealistic configurations.
if *numNodes < 1 { log.Fatal("Number of nodes must be at least 1") } +if *numNodes > 10 { + log.Fatal("Number of nodes cannot exceed 10 (resource constraints)") +}
238-247: Consider making the initialization wait time configurable.The hardcoded 3-second wait might not be sufficient for slower systems or could be unnecessary for faster ones.
// Wait for EVM layer to initialize -time.Sleep(3 * time.Second) +waitTime := 3 * time.Second +if evmInitWait := os.Getenv("EVM_INIT_WAIT"); evmInitWait != "" { + if duration, err := time.ParseDuration(evmInitWait); err == nil { + waitTime = duration + } +} +time.Sleep(waitTime)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
block/da_includer.go(3 hunks)block/manager.go(7 hunks)scripts/run-evm-nodes.go(7 hunks)test/e2e/evm_test_common.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/evm_test_common.go
🚧 Files skipped from review as they are similar to previous changes (2)
- block/da_includer.go
- block/manager.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Follow standard Go formatting (enforced by golangci-lint). Use meanin...
**/*.go: Follow standard Go formatting (enforced by golangci-lint).
Use meaningful variable names in Go code.
Keep functions small and focused in Go code.
Document exported types and functions in Go code.
Use context.Context for cancellation in Go code.
Wrap errors with context usingfmt.Errorfin Go code.
Return errors early in Go code.
Use custom error types for domain-specific errors in Go code.
Use structured logging and include relevant context in log messages in Go code.
Use appropriate log levels in Go code.
Never expose private keys in logs or errors.
Validate all inputs from external sources.
Use secure random number generation in Go code.
Be careful with concurrent access to shared state in Go code.
Be mindful of goroutine leaks in Go code.
Use buffered channels appropriately in Go code.
Profile before optimizing Go code.
Consider memory allocation in hot paths in Go code.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
scripts/run-evm-nodes.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-06-10T19:23:16.839Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
Learnt from: tzdybal
PR: rollkit/rollkit#1641
File: node/full_node_test.go:266-321
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Issue #1663 was created to address adding comprehensive tests for error scenarios in `TestVoteExtension` in the rollkit repository.
scripts/run-evm-nodes.go (4)
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Never expose private keys in logs or errors.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use appropriate log levels in Go code.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
Learnt from: chatton
PR: rollkit/rollkit#2378
File: test/docker-e2e/base_test.go:22-32
Timestamp: 2025-06-18T12:52:25.919Z
Learning: In the rollkit codebase test/docker-e2e, the StartBridgeNode and StartRollkitNode methods in DockerTestSuite do not return errors, so no error handling is needed when calling these methods.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Run Integration Tests
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (8)
scripts/run-evm-nodes.go (8)
25-33: Good port organization with clear naming.The use of base ports with offsets provides a clean way to handle multiple nodes without port conflicts.
42-51: Well-structured configuration abstraction.The
nodeConfigstruct effectively encapsulates per-node configuration, making the code more maintainable and readable.
214-236: Clean node configuration logic.The port calculation and directory naming logic is clear and systematic.
330-350: Genesis file copying logic is well-implemented.The file copying operation is properly handled with explicit resource cleanup using defer statements.
397-401: Good differentiated wait times for node types.The logic to use different wait times for sequencer vs full nodes shows good understanding of the initialization requirements.
406-443: Robust P2P address extraction with good error handling.The parsing logic properly handles the command output and includes comprehensive error checking. The ANSI stripping is a nice touch for clean address extraction.
578-583: Efficient ANSI escape sequence removal.The regex-based approach is appropriate and the function is well-named and focused.
372-383: Sequencer P2P address is correctly initialized and passed to full nodesVerified that
nm.startNodeis first invoked for the sequencer (capturing its P2P address) before any full nodes are started, and that the returnedsequencerP2PAddris then passed into each full‐node invocation. No changes required.
|
will use the cleanup from this pr, ill keep it open until i make a new pr with the changes |
|
Closing this in favor of #2433 which is merged |
|
close in favour of #2443 |
Overview
Duplicate of #2377, was reverted in #2394
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores