test: Add comprehensive EVM E2E tests #2394
Conversation
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
WalkthroughThis update introduces comprehensive end-to-end tests for Rollkit's EVM sequencer and full node, including P2P sync, block propagation, lazy mode, and restart recovery. It adds shared EVM test utilities, extends test helpers for full node setup, corrects documentation, and adjusts timestamp handling in transaction execution logic. One legacy aggregator test is removed. The logging framework is migrated throughout the codebase from Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant DA_Node
participant Sequencer
participant FullNode
participant EVM_Engine
Tester->>DA_Node: Start DA node
Tester->>EVM_Engine: Start EVM engine (sequencer)
Tester->>Sequencer: Start sequencer node (aggregator/lazy)
Tester->>EVM_Engine: Start EVM engine (full node)
Tester->>FullNode: Start full node (P2P with sequencer)
Tester->>Sequencer: Submit transaction(s)
Sequencer->>EVM_Engine: Process and include transactions in block(s)
Sequencer-->>FullNode: Propagate blocks via P2P
FullNode->>EVM_Engine: Sync and verify blocks/transactions/state
Tester->>Sequencer: Restart node (optional)
Tester->>FullNode: Restart node (optional)
Tester->>Sequencer: Submit/verify further transactions
Tester->>FullNode: Verify state and block consistency
Possibly related PRs
Suggested labels
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 ✨ 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 (
|
![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 significantly enhances our testing capabilities by introducing a new end-to-end test that validates the complete flow of an EVM chain, from transaction submission on a sequencer to its eventual synchronization and confirmation on a full node. This ensures robust data propagation and consistency across different node types in the network.
Highlights
- New E2E Test for Full Node Sync: I've added a comprehensive end-to-end test,
TestEvmSequencerWithFullNodeE2E, which simulates a full EVM network setup. This test deploys both a sequencer node and a full node, submits a transaction to the sequencer, and then verifies that the transaction is correctly synced and confirmed on the full node within the same block. - Modular Test Helpers: To support the new E2E test, I've introduced several new helper functions. These include
setupTestRethEngineFullNodefor configuring the Reth engine for a full node,setupSequencerNodeandsetupFullNodefor initializing and starting the respective nodes, andsubmitTransactionAndGetBlockNumberandverifyTransactionSyncfor transaction submission and cross-node verification. - Terminology Update: I've updated the terminology in the
README.mdand within the E2E test code from 'aggregator' to 'sequencer' to align with current naming conventions for the--rollkit.node.aggregatorflag and the node's role.
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 comprehensive end-to-end tests for the EVM full node, significantly improving test coverage for the Rollkit EVM chain. The changes include new helper functions for setting up and interacting with both sequencer and full node environments, and a dedicated test case (TestEvmSequencerWithFullNodeE2E) to verify transaction synchronization between them. The terminology has also been updated from "aggregator" to "sequencer" for consistency. While the overall structure and new test coverage are excellent, there are a few areas where the tests could be made more robust by replacing fixed time.Sleep calls with condition-based polling, and ensuring all critical components (like the Engine API) are properly checked for readiness during setup.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2394 +/- ##
==========================================
+ Coverage 72.38% 72.99% +0.60%
==========================================
Files 67 67
Lines 6461 6377 -84
==========================================
- Hits 4677 4655 -22
+ Misses 1385 1323 -62
Partials 399 399
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:
|
Manav-Aggarwal
changed the title
Manav-Aggarwal
force-pushed
the
manav/add_evm_full_node_tests
branch
from
9526212 to
adedcda
Compare
Manav-Aggarwal
force-pushed
the
manav/add_evm_full_node_tests
branch
2 times, most recently
from
406cea5 to
2046291
Compare
![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
♻️ Duplicate comments (1)
test/e2e/evm_sequencer_e2e_test.go (1)
93-96: Extract common test setup to reduce duplication.The setup steps (Local DA, Reth engine, sequencer initialization) are duplicated across multiple test functions.
🧹 Nitpick comments (1)
test/e2e/evm_test_common.go (1)
312-315: Consider using pkg/os helpers for file operations.For consistency with the codebase patterns, consider using the helper functions from
pkg/os/os.go:-genesisData, err := os.ReadFile(sequencerGenesis) +import pkgos "github.com/rollkit/rollkit/pkg/os" +// ... +genesisData, err := pkgos.ReadFile(sequencerGenesis) require.NoError(t, err, "failed to read sequencer genesis file") -err = os.WriteFile(fullNodeGenesis, genesisData, 0644) +err = pkgos.WriteFile(fullNodeGenesis, genesisData, 0644) require.NoError(t, err, "failed to write full node genesis file")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/evm/single/README.md(1 hunks)execution/evm/test_helpers.go(1 hunks)test/e2e/evm_aggregator_e2e_test.go(0 hunks)test/e2e/evm_full_node_e2e_test.go(1 hunks)test/e2e/evm_sequencer_e2e_test.go(1 hunks)test/e2e/evm_test_common.go(1 hunks)test/e2e/go.mod(1 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/evm_aggregator_e2e_test.go
🧰 Additional context used
📓 Path-based instructions (2)
`test/e2e/**`: End-to-end tests must be placed in the test/e2e/ directory.
test/e2e/**: End-to-end tests must be placed in the test/e2e/ directory.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/go.modtest/e2e/evm_full_node_e2e_test.gotest/e2e/evm_sequencer_e2e_test.gotest/e2e/evm_test_common.go
`**/*_test.go`: Unit tests must be placed in *_test.go files alongside the code they test.
**/*_test.go: Unit tests must be placed in *_test.go files alongside the code they test.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_full_node_e2e_test.gotest/e2e/evm_sequencer_e2e_test.go
🧬 Code Graph Analysis (2)
test/e2e/evm_sequencer_e2e_test.go (3)
test/e2e/sut_helper.go (2)
SystemUnderTest(34-44)NewSystemUnderTest(47-57)execution/evm/test_helpers.go (3)
GetRandomTransaction(125-149)SubmitTransaction(152-160)CheckTxIncluded(163-172)test/e2e/evm_test_common.go (5)
TestPrivateKey(70-70)TestToAddress(71-71)DefaultChainID(66-66)DefaultGasLimit(67-67)SequencerEthURL(55-55)
test/e2e/evm_test_common.go (3)
execution/evm/test_helpers.go (5)
SetupTestRethEngine(40-72)SetupTestRethEngineFullNode(194-230)GetRandomTransaction(125-149)SubmitTransaction(152-160)GetGenesisHash(175-190)test/e2e/sut_helper.go (1)
SystemUnderTest(34-44)pkg/os/os.go (2)
ReadFile(49-51)WriteFile(65-67)
🪛 LanguageTool
apps/evm/single/README.md
[uncategorized] ~49-~49: Loose punctuation mark.
Context: ...ones are: - --rollkit.node.aggregator: Set to true to run in sequencer mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Gitleaks (8.26.0)
test/e2e/evm_test_common.go
70-70: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
test/e2e/go.mod (2)
6-7: LGTM!The promotion of
github.com/ethereum/go-ethereumandgithub.com/golang-jwt/jwt/v5from indirect to direct dependencies is appropriate for the new EVM tests that directly use these packages.
3-3: Invalid Go version specified.Go version 1.24.2 doesn't exist. The latest stable Go version is in the 1.22.x series. This appears to be a typo or misconfiguration.
Apply this diff to fix the Go version:
-go 1.24.2 +go 1.22.0Likely an incorrect or invalid review comment.
apps/evm/single/README.md (1)
49-49: LGTM!The terminology update from "aggregator mode" to "sequencer mode" correctly reflects the node's operational mode and aligns with the broader terminology changes in the codebase.
test/e2e/evm_sequencer_e2e_test.go (1)
196-199: LGTM!The loop variable is correctly shadowed to avoid the common Go pitfall of taking addresses of loop variables. This properly addresses the issue mentioned in the past review.
test/e2e/evm_full_node_e2e_test.go (1)
397-398: Verifyminfunction availability.The code uses the
minfunction which was introduced as a built-in in Go 1.21. However, the go.mod file specifies an invalid Go version (1.24.2).Please verify that the minimum Go version in go.mod supports the
minbuilt-in function, or implement a custom min function:func min(a, b uint64) uint64 { if a < b { return a } return b }test/e2e/evm_test_common.go (2)
70-70: Test private key is acceptable in test files.The static analysis tool flagged this as a generic API key, but this is actually a test private key used exclusively for E2E testing. Since this is in a test file and not production code, having a hardcoded test key is acceptable and common practice.
1-479: Well-structured test utilities with comprehensive documentation.This file provides an excellent foundation for EVM E2E testing with:
- Clear separation of concerns between sequencer and full node setup
- Robust error handling and timeout management
- Comprehensive documentation for each function
- Proper JWT authentication implementation
- Smart fallback mechanisms (e.g., P2P ID extraction)
The code follows Go best practices and maintains consistency with the testing patterns in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/evm_full_node_e2e_test.go (2)
421-422: Update comment to reflect sequencer instead of aggregator.The comment mentions "aggregator" but the test is actually about the sequencer producing blocks.
-// TestEvmFullNodeBlockPropagationE2E tests that blocks produced by the aggregator +// TestEvmFullNodeBlockPropagationE2E tests that blocks produced by the sequencer
633-636: Simplify absolute difference calculation.Consider using a more concise approach for calculating the absolute difference.
- heightDiff := int64(seqHeight) - int64(fnHeight) - if heightDiff < 0 { - heightDiff = -heightDiff - } + var heightDiff uint64 + if seqHeight > fnHeight { + heightDiff = seqHeight - fnHeight + } else { + heightDiff = fnHeight - seqHeight + }Alternatively, if you prefer to keep using int64:
- heightDiff := int64(seqHeight) - int64(fnHeight) - if heightDiff < 0 { - heightDiff = -heightDiff - } + heightDiff := int64(seqHeight) - int64(fnHeight) + if heightDiff < 0 { + heightDiff = -heightDiff + }Note: The current implementation is already correct, just offering a potentially clearer alternative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/evm_full_node_e2e_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`test/e2e/**`: End-to-end tests must be placed in the test/e2e/ directory.
test/e2e/**: End-to-end tests must be placed in the test/e2e/ directory.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_full_node_e2e_test.go
`**/*_test.go`: Unit tests must be placed in *_test.go files alongside the code they test.
**/*_test.go: Unit tests must be placed in *_test.go files alongside the code they test.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_full_node_e2e_test.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Run Integration Tests
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (7)
test/e2e/evm_full_node_e2e_test.go (7)
56-124: Well-structured block propagation verification helper.The function provides comprehensive validation across multiple nodes with proper error handling and informative logging.
126-174: Robust transaction synchronization verification.Good use of
require.Eventuallywith appropriate timeout and comprehensive receipt validation.
176-220: Comprehensive state root consistency validation.Excellent verification of state consistency with detailed comparisons of all block metadata.
645-675: Proper lazy mode sequencer setup.Good implementation with appropriate lazy mode flags and configuration.
677-708: Effective verification of lazy mode block production behavior.Good implementation for monitoring and ensuring no automatic block production occurs.
710-920: Comprehensive lazy mode test implementation.Excellent test coverage for lazy mode functionality with thorough validation of block production behavior and state consistency.
400-400: Verify availability of theminfunction.The
minbuilt-in function is available in Go 1.21+. Ensure the project's Go version supports this.#!/bin/bash # Description: Check the Go version requirement in the project # Check go.mod for Go version if [ -f "go.mod" ]; then echo "Go version in go.mod:" grep -E "^go\s+" go.mod fi # Check for any custom min function definitions echo -e "\nChecking for custom min function definitions:" rg -A 3 "func min\(" --type go
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/evm_sequencer_e2e_test.go (1)
439-439: Fix: Checking nonce of wrong address.The test is checking the nonce of
TestToAddress(the recipient) instead of the sender's address. The sender's address should be derived fromTestPrivateKey.
🧹 Nitpick comments (1)
test/e2e/evm_sequencer_e2e_test.go (1)
764-822: Consider using SystemUnderTest for process management.The manual process management using
pkillcould be fragile and may affect other tests running in parallel. Consider using the SystemUnderTest's built-in process management capabilities for more reliable test isolation.Instead of manual pkill commands, use:
-// Manual approach: Find and kill evm-single processes using pkill -t.Log("Attempting to find and kill evm-single processes manually...") - -// First try graceful shutdown with SIGTERM -cmd := exec.Command("pkill", "-f", "evm-single") -if err := cmd.Run(); err != nil { - t.Logf("pkill SIGTERM failed (process may not exist): %v", err) -} else { - t.Log("Sent SIGTERM to evm-single processes") -} +// Use SystemUnderTest to stop the sequencer process +sut.StopProcess(evmSingleBinaryPath) +t.Log("Stopped sequencer process via SystemUnderTest")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
execution/evm/execution.go(1 hunks)test/e2e/evm_sequencer_e2e_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`test/e2e/**`: End-to-end tests must be placed in the test/e2e/ directory.
test/e2e/**: End-to-end tests must be placed in the test/e2e/ directory.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_sequencer_e2e_test.go
`**/*_test.go`: Unit tests must be placed in *_test.go files alongside the code they test.
**/*_test.go: Unit tests must be placed in *_test.go files alongside the code they test.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_sequencer_e2e_test.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: lint / markdown-lint
- GitHub Check: lint / golangci-lint
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Rollkit EVM Single Docker Image
- GitHub Check: test / Build Rollkit Docker Image
- GitHub Check: test / Run Integration Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (5)
execution/evm/execution.go (1)
184-194: Good improvement for state synchronization.Using the actual previous block hash instead of the stored client state fields ensures more accurate forkchoice updates. This change aligns well with the PR's goal of improving EVM node synchronization and state consistency.
test/e2e/evm_sequencer_e2e_test.go (4)
90-111: Well-structured basic sequencer test.The test properly validates core functionality with clear setup, execution, and verification phases.
146-287: Comprehensive high-throughput transaction test.Good test coverage for transaction ordering and inclusion verification. The loop variable handling at line 199-200 is correctly implemented.
491-660: Thorough invalid transaction rejection testing.Good coverage of various invalid transaction scenarios including bad signatures, insufficient funds, invalid nonces, and gas limits. The system stability verification after rejections is valuable.
969-993: Clean helper function for sequencer restart.The function properly handles restarting an existing sequencer node without re-initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/evm/single/README.md (2)
49-49: Flag name vs. wording may confuse readersThe flag is still called
--rollkit.node.aggregator, yet the comment says “run in sequencer mode”. This mismatch can trip up new users skimming for a--sequencerflag.-`--rollkit.node.aggregator`: Set to true to run in sequencer mode +`--rollkit.node.aggregator`: Set to true to run the node **as a sequencer** (legacy flag name)Adding the parenthetical “legacy flag name” (or similar) keeps the docs accurate without promising a renamed CLI.
87-88: Reuse placeholder text for JWT secret to avoid duplicationYou already introduce
<path_to>/execution/evm/docker/jwttoken/jwt.hexabove (line 32). Re-using the same placeholder (or extracting it to an environment variable in both snippets) reduces cognitive load and copy-paste mistakes.Example:
---evm.jwt-secret $(cat <path_to>/execution/evm/docker/jwttoken/jwt.hex) \ +--evm.jwt-secret $(cat <path_to>/execution/evm/docker/jwttoken/jwt.hex) \ # same path as in sequencer exampleOr declare once:
export JWT_SECRET_PATH=<path_to>/execution/evm/docker/jwttoken/jwt.hex ... --evm.jwt-secret "$(cat $JWT_SECRET_PATH)" \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/evm/single/README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/evm/single/README.md
[uncategorized] ~49-~49: Loose punctuation mark.
Context: ...ones are: - --rollkit.node.aggregator: Set to true to run in sequencer mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (go)
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Run Integration 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: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/e2e/evm_sequencer_e2e_test.go (1)
446-446: Fix: Checking nonce of wrong address.The test is checking the nonce of
TestToAddress(the recipient) instead of the sender's address. The sender's address should be derived fromTestPrivateKey.Apply this diff to fix the address:
-fromAddress := common.HexToAddress(TestToAddress) +privateKey, err := crypto.HexToECDSA(TestPrivateKey) +require.NoError(t, err, "Should parse private key") +fromAddress := crypto.PubkeyToAddress(privateKey.PublicKey)You'll also need to import the
cryptopackage if not already imported:import "github.com/ethereum/go-ethereum/crypto"test/e2e/evm_full_node_e2e_test.go (1)
265-265: Remove redundant conditions for uint64 values.The conditions
seqHeight >= 0andfnHeight >= 0are always true since both variables are of typeuint64and cannot be negative.- return seqHeight >= 0 && fnHeight >= 0 && (seqHeight == 0 || fnHeight+5 >= seqHeight) + return seqHeight == 0 || fnHeight+5 >= seqHeight
🧹 Nitpick comments (1)
test/e2e/evm_sequencer_e2e_test.go (1)
771-806: Consider improving process management approach.The current manual
pkillapproach for stopping the evm-single process is fragile and platform-dependent. Consider using a more robust process management approach.Some alternatives to consider:
- Use the SystemUnderTest's process management capabilities if available
- Implement a graceful shutdown endpoint in the sequencer
- Use process groups or supervisors for better control
- Store and use the actual process PID for targeted termination
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/e2e/evm_full_node_e2e_test.go(1 hunks)test/e2e/evm_sequencer_e2e_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`test/e2e/**`: End-to-end tests must be placed in the test/e2e/ directory.
test/e2e/**: End-to-end tests must be placed in the test/e2e/ directory.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_sequencer_e2e_test.gotest/e2e/evm_full_node_e2e_test.go
`**/*_test.go`: Unit tests must be placed in *_test.go files alongside the code they test.
**/*_test.go: Unit tests must be placed in *_test.go files alongside the code they test.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_sequencer_e2e_test.gotest/e2e/evm_full_node_e2e_test.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: Summary
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
test/e2e/evm_sequencer_e2e_test.go (2)
192-200: Good fix for the loop variable issue.The implementation correctly shadows the loop variable
txHashbefore appending its address to avoid all elements referencing the same variable.
470-667: Well-structured invalid transaction testing.The test comprehensively covers various invalid transaction scenarios including invalid signatures, insufficient funds, invalid nonces, and gas limits. The error handling and system stability verification are properly implemented.
test/e2e/evm_full_node_e2e_test.go (3)
56-124: Excellent block propagation verification logic.The function provides comprehensive validation of block consistency across nodes, checking block hashes, state roots, and transaction counts with clear error messages.
272-419: Excellent comprehensive test for P2P synchronization.The test thoroughly validates sequencer and full node synchronization with multiple transaction batches, comprehensive state root verification, and clear test phases. The implementation demonstrates best practices for E2E testing.
710-920: Outstanding lazy mode test implementation.This test provides exceptional coverage of lazy mode functionality with:
- Clear verification that blocks are only produced when transactions are submitted
- Multiple test rounds with different transaction patterns (single, burst, delayed)
- Comprehensive state root verification
- Excellent documentation and structured test phases
The test effectively validates resource optimization in lazy mode while ensuring P2P sync functionality remains intact.
Manav-Aggarwal
force-pushed
the
manav/add_evm_full_node_tests
branch
from
9dfa0cd to
1df4ebc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/e2e/evm_test_common.go (2)
249-253: Consider extracting the hardcoded fallback P2P ID into a named constant.The fallback P2P ID is hardcoded inline, which reduces maintainability. Extract it to a constant for better clarity.
+const FallbackP2PID = "12D3KooWSequencerTestNode123456789012345678901234567890" // Pragmatic approach: The sequencer doesn't start P2P services until there are peers // Generate a deterministic P2P ID for the test -fallbackID := "12D3KooWSequencerTestNode123456789012345678901234567890" +fallbackID := FallbackP2PID t.Logf("⚠️ Failed to extract P2P ID from sequencer logs, using fallback test P2P ID: %s", fallbackID) t.Logf("⚠️ This indicates that P2P ID logging may have changed or failed - please verify log parsing is working correctly")
161-174: Consider refactoring JWT authentication request creation.Extract the JWT authentication logic into a helper function to improve readability and reusability.
+func createAuthenticatedRequest(engineURL, jwtSecret, body string) (*http.Request, error) { + req, err := http.NewRequest("POST", engineURL, strings.NewReader(body)) + if err != nil { + return nil, err + } + req.Header.Set("Content-Type", "application/json") + + secret, err := decodeSecret(jwtSecret) + if err != nil { + return nil, err + } + + authToken, err := getAuthToken(secret) + if err != nil { + return nil, err + } + + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", authToken)) + return req, nil +} // Also check the engine URL with JWT authentication -req, err := http.NewRequest("POST", engineURL, strings.NewReader(`{"jsonrpc":"2.0","method":"engine_getClientVersionV1","params":[],"id":1}`)) -if err != nil { - return err -} -req.Header.Set("Content-Type", "application/json") -secret, err := decodeSecret(jwtSecret) -if err != nil { - return err -} -authToken, err := getAuthToken(secret) -if err != nil { - return err -} -req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", authToken)) +req, err := createAuthenticatedRequest(engineURL, jwtSecret, `{"jsonrpc":"2.0","method":"engine_getClientVersionV1","params":[],"id":1}`) +if err != nil { + return err +}test/e2e/evm_full_node_e2e_test.go (1)
249-253: Remove redundant conditions for uint64 values.The conditions
seqHeight >= 0andfnHeight >= 0are always true since both variables are of typeuint64and cannot be negative.- return seqHeight >= 0 && fnHeight >= 0 && (seqHeight == 0 || fnHeight+20 >= seqHeight) + return seqHeight == 0 || fnHeight+20 >= seqHeight
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/evm/single/README.md(2 hunks)apps/evm/single/docker-compose.yml(1 hunks)execution/evm/execution.go(2 hunks)execution/evm/test_helpers.go(1 hunks)test/e2e/evm_aggregator_e2e_test.go(0 hunks)test/e2e/evm_full_node_e2e_test.go(1 hunks)test/e2e/evm_sequencer_e2e_test.go(1 hunks)test/e2e/evm_test_common.go(1 hunks)test/e2e/go.mod(1 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/evm_aggregator_e2e_test.go
✅ Files skipped from review due to trivial changes (1)
- apps/evm/single/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/go.mod
- execution/evm/test_helpers.go
- execution/evm/execution.go
- test/e2e/evm_sequencer_e2e_test.go
🧰 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:
test/e2e/evm_test_common.gotest/e2e/evm_full_node_e2e_test.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:
test/e2e/evm_full_node_e2e_test.go
`test/e2e/**/*_test.go`: End-to-end (E2E) tests should be placed in the `test/e2e/` directory.
test/e2e/**/*_test.go: End-to-end (E2E) tests should be placed in thetest/e2e/directory.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
test/e2e/evm_full_node_e2e_test.go
🧠 Learnings (4)
📓 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.
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.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.942Z
Learning: Update tests when changing functionality.
Learnt from: Eoous
PR: rollkit/rollkit#2208
File: pkg/store/store.go:117-138
Timestamp: 2025-04-22T17:57:47.384Z
Learning: In the Rollkit pruning implementation, index entries (mapping block hash to height) are intentionally preserved even when block data is pruned to maintain a lightweight historical record of the blockchain structure.
test/e2e/evm_test_common.go (8)
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.942Z
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.942Z
Learning: Applies to test/integration/**/*_test.go : Integration tests should be placed in the `test/integration/` directory.
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: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.942Z
Learning: Applies to **/*_test.go : Unit tests should be placed in `*_test.go` files alongside the code being tested.
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: Manav-Aggarwal
PR: rollkit/rollkit#2280
File: sequencers/based/sequencer_test.go:89-89
Timestamp: 2025-05-26T10:26:59.799Z
Learning: In DummyDA test cases, the height ticker (StartHeightTicker/StopHeightTicker) is not needed for tests that don't rely on time-based height progression, such as TestSequencer_GetNextBatch_ExceedsMaxDrift which only tests max drift behavior without requiring height advancement over time.
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.
test/e2e/evm_full_node_e2e_test.go (10)
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.942Z
Learning: Applies to test/e2e/**/*_test.go : End-to-end (E2E) tests should be placed in the `test/e2e/` directory.
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.
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.942Z
Learning: Applies to test/integration/**/*_test.go : Integration tests should be placed in the `test/integration/` directory.
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: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.942Z
Learning: Applies to **/*_test.go : Unit tests should be placed in `*_test.go` files alongside the code being tested.
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.
Learnt from: Manav-Aggarwal
PR: rollkit/rollkit#2280
File: sequencers/based/sequencer_test.go:89-89
Timestamp: 2025-05-26T10:26:59.799Z
Learning: In DummyDA test cases, the height ticker (StartHeightTicker/StopHeightTicker) is not needed for tests that don't rely on time-based height progression, such as TestSequencer_GetNextBatch_ExceedsMaxDrift which only tests max drift behavior without requiring height advancement over time.
apps/evm/single/README.md (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.
🪛 Gitleaks (8.26.0)
test/e2e/evm_test_common.go
79-79: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
apps/evm/single/README.md
[uncategorized] ~49-~49: Loose punctuation mark.
Context: ...ones are: - --rollkit.node.aggregator: Set to true to run in sequencer mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ 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: Summary
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
apps/evm/single/README.md (1)
49-49: Documentation improvements look good!The terminology correction from "aggregator mode" to "sequencer mode" accurately reflects the node's role, and using placeholder paths makes the documentation more universally applicable.
Also applies to: 88-88
test/e2e/evm_test_common.go (1)
1-702: Excellent implementation of shared EVM test utilities!This file provides a comprehensive set of well-structured utilities that effectively eliminate code duplication across EVM tests. The functions are well-documented, properly handle errors, and provide good separation of concerns between sequencer and full node setups.
test/e2e/evm_full_node_e2e_test.go (1)
1-1387: Outstanding implementation of comprehensive full node E2E tests!This test file provides excellent coverage of full node functionality with:
- Well-structured test cases covering P2P sync, block propagation, lazy mode, and restart scenarios
- Clear phase-based organization making tests easy to understand
- Comprehensive state verification including state roots, transaction consistency, and block metadata
- Excellent documentation explaining test purpose and validation steps
- Good use of helper functions to reduce duplication
- Thorough logging for debugging
The tests effectively validate the distributed nature of Rollkit's EVM integration.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cmd/run_node.go (1)
53-87: LGTM: Well-structured logger configurationThe refactored
SetupLoggerimplementation correctly uses the options pattern and handles all configuration cases appropriately. The case-insensitive log level handling and sensible defaults are good practices.Consider moving the logger initialization outside the conditional for consistency:
- // Initialize logger with configured options - configuredLogger := log.NewLogger(os.Stdout) - if len(logOptions) > 0 { - configuredLogger = log.NewLogger(os.Stdout, logOptions...) - } + // Initialize logger with configured options + configuredLogger := log.NewLogger(os.Stdout, logOptions...)This simplifies the code since
log.NewLoggershould handle empty options gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
apps/evm/based/go.sumis excluded by!**/*.sumapps/evm/single/go.sumis excluded by!**/*.sumapps/testapp/go.sumis excluded by!**/*.sumda/go.sumis excluded by!**/*.sumexecution/evm/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumsequencers/based/go.sumis excluded by!**/*.sumsequencers/single/go.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
apps/evm/based/go.mod(5 hunks)apps/evm/single/cmd/run.go(3 hunks)apps/evm/single/go.mod(4 hunks)apps/testapp/cmd/run.go(2 hunks)apps/testapp/go.mod(6 hunks)apps/testapp/kv/kvexecutor.go(0 hunks)block/aggregation_test.go(3 hunks)block/da_includer_test.go(4 hunks)block/da_speed_test.go(3 hunks)block/lazy_aggregation_test.go(2 hunks)block/manager.go(8 hunks)block/manager_test.go(6 hunks)block/pending_base.go(2 hunks)block/pending_base_test.go(9 hunks)block/pending_data.go(2 hunks)block/pending_headers.go(2 hunks)block/publish_block_p2p_test.go(4 hunks)block/publish_block_test.go(5 hunks)block/reaper.go(2 hunks)block/reaper_test.go(4 hunks)block/retriever_test.go(7 hunks)block/store_test.go(2 hunks)block/submitter_test.go(5 hunks)block/sync_test.go(3 hunks)block/test_utils.go(2 hunks)core/execution/dummy.go(0 hunks)core/execution/execution.go(0 hunks)da/cmd/local-da/local.go(2 hunks)da/cmd/local-da/main.go(2 hunks)da/go.mod(1 hunks)da/jsonrpc/client.go(3 hunks)da/jsonrpc/proxy_test.go(3 hunks)da/jsonrpc/server.go(3 hunks)execution/evm/execution.go(2 hunks)execution/evm/go.mod(1 hunks)go.mod(6 hunks)node/full.go(8 hunks)node/full_node_test.go(2 hunks)node/helpers_test.go(5 hunks)node/light.go(2 hunks)node/light_test.go(2 hunks)node/node.go(2 hunks)pkg/cmd/run_node.go(2 hunks)pkg/cmd/run_node_test.go(5 hunks)pkg/p2p/client.go(4 hunks)pkg/p2p/client_test.go(7 hunks)pkg/p2p/utils_test.go(2 hunks)pkg/rpc/client/client_test.go(2 hunks)pkg/rpc/example/example.go(2 hunks)pkg/rpc/server/server.go(3 hunks)pkg/rpc/server/server_test.go(8 hunks)pkg/service/baseservice.go(3 hunks)pkg/service/baseservice_test.go(4 hunks)pkg/sync/sync_service.go(6 hunks)pkg/sync/sync_service_test.go(2 hunks)sequencers/based/go.mod(2 hunks)sequencers/based/sequencer.go(3 hunks)sequencers/based/sequencer_test.go(3 hunks)sequencers/single/go.mod(7 hunks)sequencers/single/sequencer.go(4 hunks)sequencers/single/sequencer_test.go(18 hunks)test/mocks/execution.go(0 hunks)types/da.go(3 hunks)types/da_test.go(3 hunks)
💤 Files with no reviewable changes (4)
- core/execution/dummy.go
- apps/testapp/kv/kvexecutor.go
- core/execution/execution.go
- test/mocks/execution.go
✅ Files skipped from review due to trivial changes (42)
- block/aggregation_test.go
- node/light_test.go
- da/cmd/local-da/main.go
- da/jsonrpc/proxy_test.go
- block/pending_base.go
- block/reaper_test.go
- block/pending_data.go
- pkg/rpc/client/client_test.go
- types/da_test.go
- block/lazy_aggregation_test.go
- block/sync_test.go
- block/store_test.go
- apps/evm/single/go.mod
- execution/evm/go.mod
- block/pending_headers.go
- sequencers/based/sequencer.go
- node/full_node_test.go
- node/helpers_test.go
- da/go.mod
- sequencers/based/sequencer_test.go
- da/cmd/local-da/local.go
- da/jsonrpc/server.go
- node/light.go
- pkg/p2p/utils_test.go
- sequencers/based/go.mod
- go.mod
- pkg/service/baseservice_test.go
- block/submitter_test.go
- block/reaper.go
- pkg/p2p/client.go
- types/da.go
- apps/testapp/go.mod
- pkg/rpc/server/server_test.go
- node/node.go
- sequencers/single/sequencer.go
- pkg/rpc/server/server.go
- pkg/service/baseservice.go
- sequencers/single/go.mod
- node/full.go
- block/pending_base_test.go
- apps/evm/based/go.mod
- pkg/sync/sync_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/evm/execution.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*_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/da_speed_test.gopkg/sync/sync_service_test.gopkg/p2p/client_test.gopkg/cmd/run_node_test.goblock/publish_block_p2p_test.goblock/publish_block_test.goblock/manager_test.goblock/da_includer_test.gosequencers/single/sequencer_test.goblock/retriever_test.go
`**/*.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/da_speed_test.gopkg/sync/sync_service_test.goapps/testapp/cmd/run.goapps/evm/single/cmd/run.gopkg/rpc/example/example.gopkg/p2p/client_test.gopkg/cmd/run_node_test.goblock/publish_block_p2p_test.goblock/publish_block_test.goblock/manager_test.goda/jsonrpc/client.goblock/da_includer_test.gosequencers/single/sequencer_test.goblock/test_utils.gopkg/cmd/run_node.goblock/retriever_test.goblock/manager.go
🧠 Learnings (18)
📓 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.
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.
Learnt from: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Update tests when changing functionality.
Learnt from: Eoous
PR: rollkit/rollkit#2208
File: pkg/store/store.go:117-138
Timestamp: 2025-04-22T17:57:47.384Z
Learning: In the Rollkit pruning implementation, index entries (mapping block hash to height) are intentionally preserved even when block data is pruned to maintain a lightweight historical record of the blockchain structure.
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 test/e2e/**/*_test.go : End-to-end (E2E) tests should be placed in the `test/e2e/` directory.
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/da_speed_test.go (5)
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: 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: 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 structured logging and include relevant context in log messages in Go code.
Learnt from: Manav-Aggarwal
PR: rollkit/rollkit#2280
File: sequencers/based/sequencer_test.go:89-89
Timestamp: 2025-05-26T10:26:59.799Z
Learning: In DummyDA test cases, the height ticker (StartHeightTicker/StopHeightTicker) is not needed for tests that don't rely on time-based height progression, such as TestSequencer_GetNextBatch_ExceedsMaxDrift which only tests max drift behavior without requiring height advancement over time.
pkg/sync/sync_service_test.go (2)
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.
apps/testapp/cmd/run.go (8)
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 **/*.go : Use structured logging and include relevant context in log messages 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 : Profile before optimizing 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: 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 : Never expose private keys in logs or errors.
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: 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.
apps/evm/single/cmd/run.go (7)
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 **/*.go : Use structured logging and include relevant context in log messages in Go code.
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: 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: 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: 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.
pkg/rpc/example/example.go (3)
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 **/*.go : Use structured logging and include relevant context in log messages 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.
pkg/p2p/client_test.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: 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: 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 structured logging and include relevant context in log messages in Go code.
pkg/cmd/run_node_test.go (6)
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: 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: 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 structured logging and include relevant context in log messages 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, so no 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. They handle error checking internally using s.Require().NoError(err) for their operations, so no external error handling is needed when calling these methods.
block/publish_block_p2p_test.go (6)
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: 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 **/*.go : Use structured logging and include relevant context in log messages in Go code.
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_test.go (6)
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: 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: 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 structured logging and include relevant context in log messages in Go code.
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.
block/manager_test.go (8)
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#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: 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: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use structured logging and include relevant context in log messages in Go code.
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 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.
da/jsonrpc/client.go (5)
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: 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: CR
PR: rollkit/rollkit#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:47:52.948Z
Learning: Applies to **/*.go : Use structured logging and include relevant context in log messages in Go code.
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.
block/da_includer_test.go (6)
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: 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 appropriate log levels in Go code.
Learnt from: Manav-Aggarwal
PR: rollkit/rollkit#2280
File: sequencers/based/sequencer_test.go:89-89
Timestamp: 2025-05-26T10:26:59.799Z
Learning: In DummyDA test cases, the height ticker (StartHeightTicker/StopHeightTicker) is not needed for tests that don't rely on time-based height progression, such as TestSequencer_GetNextBatch_ExceedsMaxDrift which only tests max drift behavior without requiring height advancement over time.
sequencers/single/sequencer_test.go (5)
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: 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: 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 structured logging and include relevant context in log messages in Go code.
Learnt from: Manav-Aggarwal
PR: rollkit/rollkit#2280
File: sequencers/based/sequencer_test.go:89-89
Timestamp: 2025-05-26T10:26:59.799Z
Learning: In DummyDA test cases, the height ticker (StartHeightTicker/StopHeightTicker) is not needed for tests that don't rely on time-based height progression, such as TestSequencer_GetNextBatch_ExceedsMaxDrift which only tests max drift behavior without requiring height advancement over time.
block/test_utils.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 : 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 **/*.go : Use structured logging and include relevant context in log messages 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: 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.
pkg/cmd/run_node.go (5)
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 **/*.go : Use structured logging and include relevant context in log messages 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: 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 : Never expose private keys in logs or errors.
block/retriever_test.go (6)
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: 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: 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: 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.
block/manager.go (5)
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#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: 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 **/*.go : Use structured logging and include relevant context in log messages 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.
🧬 Code Graph Analysis (4)
apps/testapp/cmd/run.go (1)
pkg/config/config.go (1)
FlagLogLevel(94-94)
apps/evm/single/cmd/run.go (2)
pkg/config/config.go (1)
FlagLogLevel(94-94)node/setup.go (1)
DefaultMetricsProvider(18-26)
pkg/p2p/client_test.go (1)
block/metrics.go (1)
NopMetrics(74-82)
pkg/cmd/run_node.go (1)
pkg/config/config.go (1)
LogConfig(175-179)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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 (35)
pkg/sync/sync_service_test.go (1)
11-11: LGTM! Logging framework migration is correctly implemented.The migration from
github.com/ipfs/go-log/v2tocosmossdk.io/logis properly implemented with appropriate use ofNewTestLogger(t)for test scenarios. This aligns with the broader logging framework standardization across the codebase.Also applies to: 46-46
apps/testapp/cmd/run.go (2)
8-9: LGTM! Appropriate imports for logging framework migration.The new imports for
cosmossdk.io/log,zerolog, andconfigpackage are correctly added to support the logging framework migration and dynamic log level configuration.Also applies to: 15-15
27-35: LGTM! Dynamic log level configuration is properly implemented.The log level parsing implementation is correct:
- Properly retrieves the log level flag using
config.FlagLogLevel- Uses
zerolog.ParseLevelfor robust parsing- Includes appropriate error handling for invalid log levels
- Correctly appends the log level option to the options slice
block/da_speed_test.go (2)
11-11: LGTM! Logging import updated correctly.The migration to
cosmossdk.io/logis properly implemented and aligns with the broader logging framework standardization.
95-95: LGTM! Appropriate use of no-op logger for performance tests.Using
log.NewNopLogger()is ideal for performance tests likeTestDASpeedwhere logging output would add unnecessary overhead. The logger assignment is correctly updated to use the new logger type.Also applies to: 132-132
apps/evm/single/cmd/run.go (3)
10-10: LGTM! Imports correctly added for logging framework migration.The new imports for
zerologandcosmossdk.io/logare properly added to support dynamic log level configuration and the logging framework migration.Also applies to: 12-12
31-39: LGTM! Consistent log level configuration implementation.The log level parsing implementation is identical to the testapp version, ensuring consistency across the codebase. The implementation correctly handles flag retrieval, parsing, and error handling.
63-66: LGTM! Indentation fix improves code formatting.The indentation correction for the
singleMetricsinitialization block aligns the code properly with the surrounding structure, improving readability.pkg/rpc/example/example.go (3)
11-11: LGTM! Logging import migration properly implemented.The migration to
cosmossdk.io/logwith thesdklogalias is correctly implemented and aligns with the broader logging framework standardization across the codebase.
18-18: LGTM! Function signature updated for new logger type.The function signature correctly updates the logger parameter type from
logging.EventLoggertosdklog.Logger, maintaining the same functionality while conforming to the new logging framework.
80-80: LGTM! Appropriate use of no-op logger for example code.Using
sdklog.NewNopLogger()is suitable for example/demonstration code where logging output is not the primary focus. This maintains the functionality while using the new logging framework.pkg/p2p/client_test.go (1)
11-11: LGTM: Logging framework migration completed correctly.The migration from
ipfs/go-log/v2tocosmossdk.io/logis implemented consistently throughout the test file. The use oflog.NewTestLogger(t)for test contexts andlog.NewNopLogger()for non-test scenarios follows best practices and aligns with the broader codebase standardization effort.Also applies to: 37-37, 130-130, 147-147, 168-168, 221-221, 285-285
block/publish_block_p2p_test.go (1)
14-14: LGTM: Excellent structured logging implementation.The logging migration is well-executed with proper use of
log.NewTestLogger(t)for test contexts and structured logging with module context usinglogger.With("module", "..."). This pattern enhances log traceability and follows Cosmos SDK logging best practices.Also applies to: 181-181, 191-195, 207-207
pkg/cmd/run_node_test.go (1)
11-11: LGTM: Consistent no-op logger usage in command tests.The logging migration is properly implemented with consistent use of
log.NewNopLogger()for command testing scenarios where log output is not required. The reuse of the same logger instance is efficient and appropriate for these test contexts.Also applies to: 296-296, 393-393, 410-410, 446-446
da/jsonrpc/client.go (1)
10-10: LGTM: Clean logging type migration completed.The logging framework migration is cleanly implemented with:
- Proper import update to
cosmossdk.io/log- Consistent type changes from
logging.EventLoggertolog.Logger- Function signatures appropriately updated
The changes maintain all existing functionality while adopting the new logging framework.
Also applies to: 24-24, 248-248, 253-253
block/manager_test.go (3)
11-11: LGTM: Logging framework migrationThe import change from
ipfs/go-log/v2tocosmossdk.io/logis consistent with the codebase-wide logging migration.
43-43: LGTM: Correct test logger initializationThe migration to
log.NewTestLogger(t)is the proper way to initialize loggers in test functions with the new logging framework.Also applies to: 71-71, 107-107, 123-123
578-599: Confirm TODO tracking and plan for re-enabling testsThe commented-out tests for timestamp and app-hash validation reference issue #2250, which has been closed with the rationale for removing these checks due to EVM execution nuances. To close the loop:
- Issue #2250 exists and correctly tracks the removal of the “block time must be strictly increasing” and “app hash mismatch” validations.
- What is the timeline or plan for re-enabling these tests (or permanently removing them)?
- Have we evaluated any security or consensus implications from disabling these validations?
sequencers/single/sequencer_test.go (2)
10-10: LGTM: Logging framework migrationImport change to
cosmossdk.io/logis consistent with the codebase-wide logging migration.
28-28: LGTM: Appropriate use of no-op logger in testsUsing
log.NewNopLogger()is correct for these tests since they don't need to verify log output. This is more efficient than test loggers when log verification isn't required.Also applies to: 60-60, 112-112, 186-186, 245-245, 265-265, 285-285, 305-305, 326-326, 347-347, 375-375, 431-431, 453-453, 477-477, 515-515, 630-630
block/da_includer_test.go (2)
27-30: LGTM: Simplified logger mock setupThe streamlined mock expectations with
mock.Anythingand.Maybe()are more maintainable and less brittle than specific argument matching for all log levels.
170-171: LGTM: Enhanced error logging verificationAdding specific logger expectations for error scenarios improves test coverage by verifying that appropriate error messages are logged during failure conditions.
Also applies to: 227-230, 254-259
pkg/cmd/run_node.go (2)
10-10: LGTM: Required imports for logging refactorThe new imports (
strings,cosmossdk.io/log,github.com/rs/zerolog) are necessary to support the refactoredSetupLoggerfunction.Also applies to: 14-14, 16-16
52-52: LGTM: Function signatures updated for new logging frameworkThe return type and parameter type changes from
logging.EventLoggertolog.Loggerare correct and necessary for the logging migration.Also applies to: 91-91
block/test_utils.go (2)
8-8: LGTM: Logging framework migrationImport change to
cosmossdk.io/logis consistent with the codebase-wide logging migration.
36-41: LGTM: Simplified MockLogger for new interfaceThe
MockLoggerimplementation correctly matches the newlog.Loggerinterface with simplified method signatures and proper interface compliance.block/retriever_test.go (2)
37-40: LGTM! Logger mock calls simplified to match new logging interface.The consistent 2-argument format aligns with the migration to
cosmossdk.io/log.
632-672: Excellent improvement: Channel-based synchronization replaces unreliable sleep.The refactoring from
time.Sleep(100 * time.Millisecond)to channel-based signaling eliminates timing-dependent test failures and improves test execution speed. The ordered mock expectations also make the test more precise.block/manager.go (2)
16-16: LGTM! Logger migration to cosmossdk.io/log is consistent.The logger type changes are part of the broader codebase migration.
Also applies to: 138-138, 174-174, 282-282
867-891: LGTM! Cleaner transaction handling logic.The refactored code properly initializes an empty transaction list and only populates it for non-empty batches. This is more explicit and easier to understand.
block/publish_block_test.go (5)
13-13: LGTM: Clean logging framework migrationThe import statement has been properly updated from
github.com/ipfs/go-log/v2tocosmossdk.io/log, aligning with the broader codebase migration mentioned in the summary.
57-60: LGTM: Proper logger initialization with new frameworkThe logger initialization has been correctly updated to use
log.NewLoggerwith appropriate options. The configuration includes disabling color output, which is suitable for test buffer logging.
149-149: LGTM: Enhanced test verification with log assertionGood addition of log output verification to ensure the expected warning message is logged when pending headers/data reach the configured limit. This improves test coverage and validates the logging behavior.
167-167: LGTM: Appropriate test logger usageThe logger creation has been properly updated to use
log.NewTestLogger(t), which is the correct method for test environments in the new logging framework.
237-237: LGTM: Consistent test logger usageThe logger creation follows the same pattern as other test functions, using
log.NewTestLogger(t)appropriately for the test environment.
| } | ||
|
|
||
| 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) { | ||
| func (m *Manager) execCreateBlock(_ context.Context, height uint64, lastSignature *types.Signature, lastHeaderHash types.Hash, _ types.State, batchData *BatchData) (*types.SignedHeader, *types.Data, error) { |
There was a problem hiding this comment.
Potential race condition: Use the passed lastState parameter instead of m.lastState.
The function ignores the lastState parameter and directly accesses m.lastState fields. This bypasses the mutex protection that the caller has already established. The caller acquires m.lastStateMtx.RLock() and passes a safe copy of the state, but this function ignores it.
Either:
- Use the passed
lastStateparameter (recommended) - Remove the parameter if it's truly not needed
-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(_ context.Context, height uint64, lastSignature *types.Signature, lastHeaderHash types.Hash, lastState types.State, batchData *BatchData) (*types.SignedHeader, *types.Data, error) {
// ...
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()),
},
LastHeaderHash: lastHeaderHash,
ConsensusHash: make(types.Hash, 32),
- AppHash: m.lastState.AppHash,
+ AppHash: lastState.AppHash,
ProposerAddress: m.genesis.ProposerAddress,
},Also applies to: 846-857
🤖 Prompt for AI Agents
In block/manager.go around lines 818 and 846-857, the function execCreateBlock
ignores the passed lastState parameter and instead accesses m.lastState
directly, which can cause a race condition by bypassing the caller's mutex
protection. To fix this, replace all uses of m.lastState within these lines with
the passed lastState parameter to ensure thread-safe access. If lastState is
truly unused, consider removing the parameter, but using it is recommended to
maintain concurrency safety.
| // // 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 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 | ||
| // 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) | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Re-enable timestamp and AppHash validation to prevent consensus issues.
Commenting out these validations removes important safety checks:
- Timestamp validation: Prevents blocks with non-monotonic timestamps
- AppHash validation: Ensures state consistency between blocks
These checks are fundamental to blockchain integrity and should not be disabled without a compelling reason.
Why were these validations disabled? If there's a specific issue being addressed, consider a more targeted solution rather than removing these critical checks entirely.
🏁 Script executed:
#!/bin/bash
# Locate block/manager.go
file=$(fd manager.go | grep "block/manager\.go$")
echo "File path: $file"
# Show the commented out timestamp and AppHash validation lines with context
echo -e "\nTimestamp validation context:"
rg -C2 "header.Time" "$file"
echo -e "\nAppHash validation context:"
rg -C2 "header.AppHash" "$file"
# Check if any other part of the codebase enforces these validations elsewhere
echo -e "\nSearching for timestamp checks in applyBlock implementations:"
rg -n "Time().After" -C2 -g '*.go'
echo -e "\nSearching for AppHash comparisons elsewhere:"
rg -n "AppHash" -C2 -g '*.go'Length of output: 33513
🏁 Script executed:
#!/bin/bash
# Show the applyBlock signature
rg -n "func .*applyBlock" -C3 block/manager.go
# Display lines 700-900 to see applyBlock implementation context
sed -n '700,900p' block/manager.goLength of output: 7504
🏁 Script executed:
#!/bin/bash
# Locate NextState implementation in types/state.go
rg -n "func (.+)NextState" -C3 types/state.go
# Display NextState function body for deeper insight
rg -n "NextState" -A30 -B5 types/state.go
# Locate types.Validate(header, data) implementation
rg -n "func Validate" -C3 types/validate.go
# Display Validate function body
rg -n "Validate" -A30 -B5 types/validate.goLength of output: 890
🏁 Script executed:
#!/bin/bash
# Search for all Validate functions to locate types.Validate and examine its implementation
echo "Listing all Validate function definitions:"
rg -n "func .*Validate" -C3 --glob '*.go'
# Search for any reference to types.Validate in imports or calls
echo -e "\nReferences to types.Validate:"
rg -n "types\.Validate" -C3 --glob '*.go'
# Inspect header.ValidateBasic implementation to see what checks it performs
echo -e "\nInspecting header.ValidateBasic:"
rg -n "func (h \*Header) ValidateBasic" -A20 -B5 --glob 'types/header.go'Length of output: 13830
Re-enable timestamp and AppHash validation in execValidate
Both checks were commented out in block/manager.go but are essential to prevent blocks with non-monotonic times or mismatched state hashes from being accepted:
• File: block/manager.go (func Manager.execValidate, ≈lines 802–813)
• File: block/manager_test.go (tests for “app hash mismatch” around lines 594–600)
Suggested diff in execValidate:
@@ -800,6 +800,16 @@ func (m *Manager) execValidate(lastState types.State, header *types.SignedHeader, data *types.Data) error {
// Check that the header's height is the expected next height
expectedHeight := lastState.LastBlockHeight + 1
if header.Height() != expectedHeight {
return fmt.Errorf("invalid height: expected %d, got %d", expectedHeight, header.Height())
}
+ // 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 lastState.AppHash
+ if !bytes.Equal(header.AppHash, lastState.AppHash) {
+ return fmt.Errorf("app hash mismatch: expected %x, got %x", lastState.AppHash, header.AppHash)
+ }And restore or update the corresponding commented-out test in block/manager_test.go to assert that a bad AppHash or timestamp triggers an error.
🤖 Prompt for AI Agents
In block/manager.go around lines 802 to 813, re-enable the commented-out
validation checks in the execValidate function that ensure the block header's
timestamp is strictly greater than the last block's time and that the header's
AppHash matches the lastState's AppHash. Uncomment these checks to prevent
acceptance of blocks with non-monotonic timestamps or mismatched state hashes.
Additionally, in block/manager_test.go near lines 594 to 600, restore or update
the test cases to verify that invalid AppHash or timestamp values correctly
trigger validation errors.
<!-- 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 Duplicate of #2408, 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> --> Co-authored-by: pthmas <9058370+pthmas@users.noreply.github.com>
This reverts commit a89ee79.
<!-- 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 Duplicate of #2411, 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 * **New Features** * Added a script and documentation for automated setup and management of EVM nodes, including sequencer and full node orchestration, JWT handling, Docker integration, and process monitoring. * **Refactor** * Replaced the previous logging system with a new logging backend throughout the codebase, updating logger types, initialization, and log message formatting for improved consistency. * Updated public interfaces and constructors to use the new logger type. * Adjusted log message formatting for clarity and readability. * Simplified P2P address handling by extracting full multiaddress strings instead of parsing P2P IDs from logs. * **Documentation** * Added and updated documentation to reflect new scripts and clarify execution-layer compatibility. * **Chores** * Cleaned up and updated dependencies, removing unused logging libraries and related packages across multiple modules and test files. * **Tests** * Updated test code to use the new logging system and adjusted logger initialization and configuration in tests. * Removed an obsolete test for transaction submission in the EVM execution layer. * Minor test logic adjustments, including increased block time and improved P2P address retrieval. * Removed redundant comments in test helpers. * **Style** * Improved log message formatting for better clarity in output. * **Bug Fixes** * Added a small delay in an end-to-end test to improve reliability of block height assertions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: tac0turtle <you@example.com> Co-authored-by: Marko <marko@baricevic.me>
| args := engine.ForkchoiceStateV1{ | ||
| HeadBlockHash: c.currentHeadBlockHash, | ||
| SafeBlockHash: c.currentSafeBlockHash, | ||
| FinalizedBlockHash: c.currentFinalizedBlockHash, |
There was a problem hiding this comment.
were these empty or why remove them?
There was a problem hiding this comment.
this is actually how the our execution evm implementation used to work before this PR: https://github.com/rollkit/rollkit/pull/2373/files#r2178116028
we already have a follow up issue for this: #2402
if we do not have this change, restarts for the evm do not work well since on startup the all the current hashes are set to the genesisHash, and it'll try to make a forkChoiceUpdatedV3 call with the genesis hash again on restart. Made this change back to make restarts work well when XO markets pointed out that restarts used to work fine but didn't work after this PR: #2373
|
|
||
| ts := uint64(timestamp.Unix()) | ||
| if ts <= prevTimestamp { | ||
| ts = prevTimestamp + 1 // Subsequent blocks must have a higher timestamp. |
There was a problem hiding this comment.
this seems entirely arbitrary, taking the old time stamp add adding one could mean that the execution timestamp could skew over time.
There was a problem hiding this comment.
yes this is also a reversion to what was changed already in https://github.com/rollkit/rollkit/pull/2373/files#r2178117196
Note that all indexers etc use timestamp in seconds, and what's that Unix() time here does. Without this change, running a chain with a block time in milliseconds breaks since reth rejects building two blocks with the same timestamp, so this was a workaround for the time being. We should have a different way as a followup
<!-- 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
Adds tests 1, 2, 3, 4, 9, 10, 11 from #2351
Sequencer tests added:
TestEvmMultipleTransactionInclusionE2E- High-throughput processing with 200 transactions (can bump it up but kept it lower for ligher CI load)TestEvmDoubleSpendNonceHandlingE2E- Double-spend prevention with duplicate noncesTestEvmInvalidTransactionRejectionE2E- Various invalid transaction type rejectionsTestEvmSequencerRestartRecoveryE2E- Able to switch modes between restarts and recovery validationStandardRestart: Normal start -> Normal restartLazyModeRestart: Normal start -> Lazy restartLazyToStandardRestart: Lazy start -> Normal restartLazyToLazyRestart: Lazy start -> Lazy restartFull node tests added:
TestEvmSequencerWithFullNodeE2E- Full node P2P sync with sequencerTestEvmFullNodeBlockPropagationE2E- Block propagation across multiple nodes (simulated)TestEvmLazyModeSequencerE2E- Lazy mode sequencer with full node P2P syncTestEvmSequencerFullNodeRestartE2E- Able to switch modes between restarts and recovery testingStandardRestart: Normal start -> Normal restartLazyModeRestart: Normal start -> Lazy restartLazyToStandardRestart: Lazy start -> Normal restartLazyToLazyRestart: Lazy start -> Lazy restartSummary by CodeRabbit
Documentation
New Features
Tests
Chores
github.com/ipfs/go-log/v2tocosmossdk.io/logacross codebase and tests for consistent and improved logging.Bug Fixes
Refactor