fix: Make EVM Execution more robust #2327
Conversation
WalkthroughThis update introduces new test helpers and an end-to-end test for the EVM aggregator, refactors EVM execution and test code to use external helpers, and expands dependency lists in several Go modules. The Makefile is updated to support the new test flow. No public APIs are removed; new helpers and fields are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Docker as Docker Compose
participant Reth as Reth Engine
participant Agg as Aggregator Node
participant DA as DA Service
Test->>Docker: Start Reth Engine container with JWT
Docker->>Reth: Launch
Test->>Agg: Start Aggregator Node (with flags)
Agg->>DA: Connect to DA Service
Test->>Reth: Submit Transaction (via JSON-RPC)
Reth->>Agg: Block production
Agg->>Test: Aggregator available
Test->>Reth: Poll for transaction inclusion
Reth->>Test: Transaction included in block
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2327 +/- ##
==========================================
+ Coverage 54.89% 56.35% +1.45%
==========================================
Files 82 82
Lines 7503 7504 +1
==========================================
+ Hits 4119 4229 +110
+ Misses 2963 2866 -97
+ Partials 421 409 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
from
69e3d01 to
78609b1
Compare
Manav-Aggarwal
changed the title
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
6 times, most recently
from
d3b0f66 to
ef7354f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apps/evm/based/go.mod (1)
30-378: 💡 Verification agent🧩 Analysis chain
LGTM! Dependency updates support new EVM testing infrastructure.
The extensive addition of indirect dependencies aligns well with the PR objectives of adding E2E tests for EVM execution. The new dependencies include:
- Docker/containerd ecosystem: Support for container orchestration in tests
- AWS SDK v2: Cloud infrastructure integration
- Kubernetes client libraries: Container management capabilities
- OpenTelemetry: Observability and instrumentation
- testcontainers: Testing framework for containerized services
These additions are consistent with the new EVM testing infrastructure that uses Docker Compose to orchestrate Reth engine containers.
Please verify that all new dependencies are from trusted sources and check for any security advisories:
🏁 Script executed:
#!/bin/bash # Check for security vulnerabilities in new dependencies go list -m -json all | jq -r '.Path' | grep -E "(aws|docker|containerd|testcontainers)" | head -20 | while read dep; do echo "Checking $dep..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: GO, package: \"$dep\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }" doneLength of output: 223
I've updated the verification script to run within the correct module directory and enumerate only the relevant dependencies before querying the GitHub API. This should surface any advisories for
aws,docker,containerd, andtestcontainerspackages.
🏁 Script executed:
#!/bin/bash set -e # Switch to the EVM module containing go.mod cd apps/evm/based # List all modules, filter for our key dependencies, and dedupe deps=$(go list -m all | awk '{print $1}' | grep -E "(aws|docker|containerd|testcontainers)" | sort -u) # For each dependency, query GitHub security advisories for dep in $deps; do echo "Checking $dep..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: GO, package: \"$dep\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }" doneLength of output: 35232
Action Required: Upgrade AWS SDK v1 to Mitigate CVEs
The indirect module
github.com/aws/aws-sdk-gohas multiple moderate-severity CVEs in S3 Crypto SDK for versions < 1.34.0. Please confirm the resolved version inapps/evm/based/go.modand bump it to ≥ 1.34.0:
- CBC padding oracle issue (MODERATE) – vulnerable < 1.34.0
- In-band key negotiation issue (LOW) – vulnerable < 1.34.0
- Unencrypted MD5 hash in metadata (MODERATE) – vulnerable < 1.34.0
- Unencrypted plaintext hash metadata (MODERATE) – vulnerable < 1.34.0
All other “aws-sdk-go-v2”, Docker/containerd, and Testcontainers packages are at versions outside the reported vulnerable ranges. No additional high-severity advisories were detected for:
• Docker/containerd ecosystem
• testcontainers-go
• AWS SDK v2
• Kubernetes clients
• OpenTelemetryPlease update or remove the v1 SDK as appropriate to eliminate these known vulnerabilities.
🧹 Nitpick comments (3)
test/e2e/evm_aggregator_e2e_test.go (2)
19-21: Consider making the Docker path configurable.The hard-coded Docker path works for the current structure but could be made more flexible for different deployment scenarios.
Consider making this configurable via environment variable or test flag:
const ( - DOCKER_PATH = "../../execution/evm/docker" + defaultDockerPath = "../../execution/evm/docker" ) + +func getDockerPath() string { + if path := os.Getenv("EVM_DOCKER_PATH"); path != "" { + return path + } + return defaultDockerPath +}
31-90: Comprehensive E2E test with good structure.The test effectively covers the complete EVM aggregator workflow:
Strengths:
- Proper setup and teardown via
SystemUnderTest- Good use of helper functions from
execution/evmpackage- Realistic transaction flow testing
- Appropriate timeout handling for async operations
Areas for improvement:
- Hard-coded values could be extracted to constants
- Error messages could be more descriptive
Consider extracting magic values to constants for better maintainability:
+const ( + LocalDAStartDelay = 500 * time.Millisecond + BlockTime = "1s" + NodeAwaitTimeout = 10 * time.Second + TxInclusionTimeout = 20 * time.Second + TxPollInterval = 1 * time.Second + // Test transaction parameters + TestPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e" + TestToAddress = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E" + TestChainID = "1234" + TestGasLimit = uint64(22000) +)And improve error message specificity:
- require.True(t, found, "Transaction not included in EVM block after waiting") + require.True(t, found, "Transaction %s not included in EVM block after %d seconds", tx.Hash().Hex(), TxInclusionTimeout/time.Second)execution/evm/evm_helpers.go (1)
61-92: Consider test isolation for parallel execution.The Docker setup is well-structured with proper cleanup. However, if multiple tests run in parallel with the same JWT filename, they might conflict when writing to the same file path.
Consider using a unique filename per test (e.g., including the test name or a random suffix) to ensure proper isolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/evm/based/go.sumis excluded by!**/*.sumapps/evm/single/go.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
apps/evm/based/go.mod(8 hunks)apps/evm/single/go.mod(8 hunks)block/submitter.go(3 hunks)execution/evm/evm_helpers.go(1 hunks)execution/evm/execution.go(8 hunks)execution/evm/execution_test.go(5 hunks)scripts/test.mk(2 hunks)test/e2e/evm_aggregator_e2e_test.go(1 hunks)test/e2e/go.mod(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/e2e/evm_aggregator_e2e_test.go (2)
execution/evm/evm_helpers.go (5)
SetupTestRethEngine(61-92)GetGenesisHash(184-198)GetRandomTransaction(143-165)SubmitTransaction(167-172)CheckTxIncluded(174-181)test/e2e/sut_helper.go (1)
NewSystemUnderTest(47-57)
block/submitter.go (2)
types/block.go (1)
Data(33-36)types/tx.go (2)
Txs(7-7)Tx(4-4)
execution/evm/execution_test.go (2)
execution/evm/evm_helpers.go (3)
SetupTestRethEngine(61-92)GetRandomTransaction(143-165)SubmitTransaction(167-172)test/e2e/evm_aggregator_e2e_test.go (1)
DOCKER_PATH(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Run E2E System Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (21)
apps/evm/single/go.mod (1)
31-378: LGTM! Consistent dependency updates across EVM modules.The dependency additions mirror those in
apps/evm/based/go.mod, ensuring consistent dependency management across both EVM modules. This coordinated approach supports the new containerized testing infrastructure while maintaining module compatibility.The additions include the same categories of dependencies needed for the E2E testing infrastructure:
- Container orchestration libraries
- Testing frameworks
- Cloud SDK components
- Observability tools
scripts/test.mk (2)
14-16: LGTM! Proper integration of EVM testing into build system.The updates correctly:
- Add
build-evm-singledependency to ensure the EVM binary is available- Include EVM-specific build tags (
e2e evm)- Pass both regular and EVM binary paths as test parameters
The 15-minute timeout is appropriate for E2E tests that involve container orchestration.
33-35: Well-structured EVM test target.The new
test-evmtarget provides a focused way to run EVM-specific tests with proper build dependencies and tags. The structure follows the established pattern from other test targets.test/e2e/evm_aggregator_e2e_test.go (2)
1-3: LGTM! Proper build constraint usage.The build constraints ensure this test only runs when the
evmtag is specified, preventing it from running in environments where EVM dependencies aren't available.
75-75:Details
❓ Verification inconclusive
Verify transaction parameters are realistic.
The hard-coded private key and transaction parameters should be validated to ensure they represent realistic test scenarios.
Please verify the test transaction parameters:
🏁 Script executed:
#!/bin/bash # Check if the private key is valid and corresponds to expected address echo "Verifying test transaction parameters..." # You could add a simple Go script to validate the private key format python3 -c " import binascii try: key = 'cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e' bytes.fromhex(key) print(f'Private key format valid: {len(key)} characters') if len(key) == 64: print('✓ Valid 32-byte private key') else: print('✗ Invalid private key length') except ValueError: print('✗ Invalid hex format') "Length of output: 565
Validate E2E test transaction parameters for realism
We’ve confirmed the private key is a valid 32-byte hex string. Before merging, please:
- Derive and assert the public address from the key matches the hard-coded recipient (
0x944fDcD1c868E3cC566C78023CcB38A32cDA836E).- Consider using standard chain IDs (e.g. 1 for mainnet, 1337 for Ganache) instead of “1234,” or document why a custom ID is needed.
- Adjust the gas limit to a realistic value (21 000 for a basic ETH transfer) or explain why 22 000 was chosen here.
block/submitter.go (2)
207-210: Log message appropriately simplified.The removal of partial submission tracking from the log message aligns with the new all-or-nothing submission approach.
223-231: All-or-nothing submission correctly implemented.The code properly creates a
types.Dataobject from the batch transactions and updates the data cache on successful submission. The immediate return on success aligns with the simplified submission approach.execution/evm/execution_test.go (3)
65-65: Test setup properly refactored to use external helper.Good use of the exported
SetupTestRethEnginehelper function for improved modularity.
85-86: Proper nonce management implemented.The use of a pointer to
lastNonceallows the helper function to maintain proper nonce sequencing across multiple transactions.Also applies to: 95-95
98-98: Transaction submission properly delegated to helper.Good refactoring to use the external
SubmitTransactionhelper for cleaner test code.Also applies to: 262-262
execution/evm/evm_helpers.go (3)
31-58: JWT helper functions are well-implemented.The JWT secret generation, decoding, and token creation follow best practices with appropriate secret length and token expiry.
95-140: Comprehensive container readiness check.The function properly validates both RPC endpoints with appropriate timeout and polling intervals. JWT authentication for the Engine API is correctly implemented.
184-198: Genesis hash helper properly implemented.The function correctly retrieves the genesis hash with proper error handling and resource cleanup.
execution/evm/execution.go (8)
12-12: LGTM: Sync import added for mutex support.The addition of the
syncpackage import is necessary for the mutex functionality introduced in this PR.
51-51: LGTM: Constructor renamed for clarity.The function rename from
NewPureEngineExecutionClienttoNewEngineExecutionClientimproves clarity and aligns with the updated architecture.
95-96: LGTM: Proper mutex locking for thread safety.The mutex locking in
InitChaincorrectly protects the shared state during initialization.
127-128: LGTM: Proper mutex locking for thread safety.The mutex locking in
GetTxscorrectly protects access to shared state during transaction retrieval.
166-167: LGTM: Proper mutex locking for thread safety.The mutex locking in
ExecuteTxscorrectly protects the shared state during transaction execution.
183-183: Excellent improvement: Enhanced robustness through error tolerance.Changing from
returntocontinuemakes the execution more robust by allowing the batch to proceed even if individual transactions fail. This prevents a single failed transaction from stopping the entire execution flow.
278-299: Well-designed forkchoice state management with proper error handling.The refactored
setFinalmethod correctly manages forkchoice state by:
- Tracking and using stored hash values appropriately
- Handling both finalization and non-finalization cases
- Adding proper error checking for invalid payload status
This is a significant improvement over the previous implementation.
319-320: LGTM: Proper mutex locking for thread safety.The mutex locking in
SetFinalcorrectly protects the shared state during finalization operations.
block/submitter.go
Outdated
| "height", res.Height, | ||
| "submittedTxs", submittedTxs, | ||
| "remainingTxs", len(currentBatch.Transactions)-submittedTxs) | ||
|
|
||
| submittedTxCount += submittedTxs | ||
|
|
||
| // Check if all transactions in the current batch were submitted | ||
| if submittedTxs == len(currentBatch.Transactions) { | ||
| submittedAllTxs = true | ||
| } else { | ||
| // Update the current batch to contain only the remaining transactions | ||
| currentBatch.Transactions = currentBatch.Transactions[submittedTxs:] | ||
| } | ||
|
|
There was a problem hiding this comment.
We have one PFB transaction per signed data so this was unnecessary
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
2 times, most recently
from
d8b93bc to
29f7a12
Compare
tzdybal
left a comment
tzdybal
left a comment
There was a problem hiding this comment.
Nice catch 👍
Left some comments. Nothing critical.
There was a problem hiding this comment.
I think we shouldn't export most of the functions, as they are test oriented.
There was a problem hiding this comment.
Yes I wanted to but in that case, we'd need to duplicate them in end to end tests. Should we do that instead?
There was a problem hiding this comment.
you can also introduce a testing package for clean separation. But IMHO it is fair to have useful testing code next to the production code. Nevertheless some methods can be private as they are not used by e2e.
How about renaming to test_support.go , test_helpers.go or test_utils.go to make the testing scope explicit? "evm" is in the package name already
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
execution/evm/evm_helpers.go (5)
31-58: Consider making test helper functions unexported.Based on previous feedback, these JWT helper functions are test-oriented and should likely be unexported to follow Go conventions for internal test utilities.
Apply this diff to make the functions unexported:
-func GenerateJWTSecret() (string, error) { +func generateJWTSecret() (string, error) { -func DecodeSecret(jwtSecret string) ([]byte, error) { +func decodeSecret(jwtSecret string) ([]byte, error) { -func GetAuthToken(jwtSecret []byte) (string, error) { +func getAuthToken(jwtSecret []byte) (string, error) {The implementation logic is solid with proper error handling and secure random generation.
61-92: Improve identifier sanitization and consider function visibility.Two improvements to consider:
Function visibility: This is a test helper and should likely be unexported based on previous feedback.
Identifier sanitization: The string replacements for the Docker compose identifier could be more robust.
Apply this diff for better identifier handling:
-func SetupTestRethEngine(t *testing.T, dockerPath, jwtFilename string) string { +func setupTestRethEngine(t *testing.T, dockerPath, jwtFilename string) string { dockerAbsPath, err := filepath.Abs(dockerPath) require.NoError(t, err) jwtPath := filepath.Join(dockerAbsPath, "jwttoken") err = os.MkdirAll(jwtPath, 0750) require.NoError(t, err) - jwtSecret, err := GenerateJWTSecret() + jwtSecret, err := generateJWTSecret() require.NoError(t, err) jwtFile := filepath.Join(jwtPath, jwtFilename) err = os.WriteFile(jwtFile, []byte(jwtSecret), 0600) require.NoError(t, err) t.Cleanup(func() { _ = os.Remove(jwtFile) }) composeFilePath := filepath.Join(dockerAbsPath, "docker-compose.yml") - identifier := tc.StackIdentifier(strings.ToLower(t.Name())) - identifier = tc.StackIdentifier(strings.ReplaceAll(string(identifier), "/", "_")) - identifier = tc.StackIdentifier(strings.ReplaceAll(string(identifier), " ", "_")) + // Sanitize test name for Docker compose stack identifier + sanitizedName := strings.ToLower(t.Name()) + sanitizedName = strings.ReplaceAll(sanitizedName, "/", "_") + sanitizedName = strings.ReplaceAll(sanitizedName, " ", "_") + // Remove other problematic characters for Docker naming + sanitizedName = strings.ReplaceAll(sanitizedName, "-", "_") + identifier := tc.StackIdentifier(sanitizedName)
95-140: Consider making function unexported.This function is well-implemented with proper timeout handling, resource cleanup, and error management. However, as a test helper function, it should likely be unexported based on previous feedback.
Apply this diff to make the function unexported and update internal calls:
-func WaitForRethContainer(t *testing.T, jwtSecret, ethURL, engineURL string) error { +func waitForRethContainer(t *testing.T, jwtSecret, ethURL, engineURL string) error { t.Helper() client := &http.Client{Timeout: 100 * time.Millisecond} timer := time.NewTimer(30 * time.Second) defer timer.Stop() for { select { case <-timer.C: return fmt.Errorf("timeout waiting for reth container to be ready") default: rpcReq := strings.NewReader(`{"jsonrpc":"2.0","method":"net_version","params":[],"id":1}`) resp, err := client.Post(ethURL, "application/json", rpcReq) if err == nil { if err := resp.Body.Close(); err != nil { return fmt.Errorf("failed to close response body: %w", err) } if resp.StatusCode == http.StatusOK { 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) + secret, err := decodeSecret(jwtSecret) if err != nil { return err } - authToken, err := GetAuthToken(secret) + authToken, err := getAuthToken(secret)And update the call in setupTestRethEngine:
- err = WaitForRethContainer(t, jwtSecret, "http://localhost:8545", "http://localhost:8551") + err = waitForRethContainer(t, jwtSecret, "http://localhost:8545", "http://localhost:8551")
143-165: Consider function visibility and improve nonce handling clarity.The function implementation is solid, but two improvements to consider:
Function visibility: As a test helper, this should likely be unexported.
Nonce parameter handling: Modifying the
lastNonceparameter in place could be confusing. Consider returning the new nonce or making the side effect more explicit.Apply this diff for better nonce handling:
-func GetRandomTransaction(t *testing.T, privateKeyHex, toAddressHex, chainID string, gasLimit uint64, lastNonce *uint64) *types.Transaction { +func getRandomTransaction(t *testing.T, privateKeyHex, toAddressHex, chainID string, gasLimit uint64, nonce uint64) (*types.Transaction, uint64) { privateKey, err := crypto.HexToECDSA(privateKeyHex) require.NoError(t, err) chainId, _ := new(big.Int).SetString(chainID, 10) txValue := big.NewInt(1000000000000000000) gasPrice := big.NewInt(30000000000) toAddress := common.HexToAddress(toAddressHex) data := make([]byte, 16) _, err = rand.Read(data) require.NoError(t, err) tx := types.NewTx(&types.LegacyTx{ - Nonce: *lastNonce, + Nonce: nonce, To: &toAddress, Value: txValue, Gas: gasLimit, GasPrice: gasPrice, Data: data, }) - *lastNonce++ - signedTx, err := types.SignTx(tx, types.NewEIP155Signer(chainId), privateKey) + signedTx, err := types.SignTx(tx, types.NewEIP155Signer(chainId), privateKey) require.NoError(t, err) - return signedTx + return signedTx, nonce + 1 }
187-201: Consider function visibility and URL parameterization.The function is well-implemented with proper resource cleanup and error handling. Two improvements to consider:
Function visibility: As a test helper, this should likely be unexported.
URL parameterization: The hardcoded "http://localhost:8545" URL could be parameterized for flexibility.
Apply this diff to address these suggestions:
-func GetGenesisHash(t *testing.T) string { +func getGenesisHash(t *testing.T, rpcURL string) string { client := &http.Client{Timeout: 2 * time.Second} data := []byte(`{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x0", false],"id":1}`) - resp, err := client.Post("http://localhost:8545", "application/json", bytes.NewReader(data)) + resp, err := client.Post(rpcURL, "application/json", bytes.NewReader(data)) require.NoError(t, err) defer resp.Body.Close() var result struct { Result struct { Hash string `json:"hash"` } `json:"result"` } require.NoError(t, json.NewDecoder(resp.Body).Decode(&result)) require.NotEmpty(t, result.Result.Hash) return result.Result.Hash }
🧹 Nitpick comments (1)
execution/evm/evm_helpers.go (1)
167-184: Resource leak issue resolved, consider remaining improvements.Good work addressing the previous resource leak concerns by adding
defer rpcClient.Close()statements! The functions now properly manage RPC client connections.Remaining improvements to consider:
Function visibility: These test helpers should likely be unexported.
URL parameterization: The hardcoded "http://localhost:8545" URL could be parameterized for flexibility.
Apply this diff to address the remaining suggestions:
-func SubmitTransaction(t *testing.T, tx *types.Transaction) { - rpcClient, err := ethclient.Dial("http://localhost:8545") +func submitTransaction(t *testing.T, rpcURL string, tx *types.Transaction) { + rpcClient, err := ethclient.Dial(rpcURL) require.NoError(t, err) defer rpcClient.Close() err = rpcClient.SendTransaction(context.Background(), tx) require.NoError(t, err) } -func CheckTxIncluded(t *testing.T, txHash common.Hash) bool { - rpcClient, err := ethclient.Dial("http://localhost:8545") +func checkTxIncluded(t *testing.T, rpcURL string, txHash common.Hash) bool { + rpcClient, err := ethclient.Dial(rpcURL) if err != nil { return false } defer rpcClient.Close() receipt, err := rpcClient.TransactionReceipt(context.Background(), txHash) return err == nil && receipt != nil && receipt.Status == 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
execution/evm/evm_helpers.go(1 hunks)execution/evm/execution.go(8 hunks)test/e2e/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- execution/evm/execution.go
- test/e2e/go.mod
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: Analyze (go)
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Run E2E System Tests
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: Summary
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
from
5ccd78a to
ab44092
Compare
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
from
47187df to
c627cda
Compare
Manav-Aggarwal
force-pushed
the
manav/fix-go-execution-evm
branch
from
1818af6 to
7660edc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/evm/based/go.mod (2)
30-30: 🛠️ Refactor suggestionConsolidate
mergodependencies
You have bothdario.cat/mergo v1.0.1(line 30) andgithub.com/imdario/mergo v0.3.16(line 154). This can lead to version conflicts and larger binaries. Choose one module path/version, remove the other fromgo.mod, and update all imports accordingly.Also applies to: 154-154
244-245: 🛠️ Refactor suggestionAvoid dual
go-tomlmajor versions
Currently you depend ongithub.com/pelletier/go-toml v1.9.5andgithub.com/pelletier/go-toml/v2 v2.2.3. Mixing majors can confuse consumers and bloat the binary. Migrate all consumers to/v2and drop the v1 path.
🧹 Nitpick comments (3)
apps/evm/based/go.mod (1)
26-378: Prune and document indirect dependencies
This module now includes hundreds of indirect requirements, which can inflate build times and binary size. Audit and remove any transitive dependencies not directly used by the EVM execution paths. For optional features (e.g., AWS, Kubernetes, telemetry), consider separate modules or build tags.execution/evm/test_helpers.go (2)
75-120: Consider optimizing the polling strategy for better performance.The current implementation polls every 100ms with a 30-second timeout. Consider using exponential backoff or a longer initial delay to reduce unnecessary load.
- time.Sleep(100 * time.Millisecond) + time.Sleep(time.Duration(200+100*attempts) * time.Millisecond) // exponential backoff
163-172: Consider logging transaction check failures for better debugging.The function gracefully handles errors by returning false, but logging the specific error could help with debugging test failures.
func CheckTxIncluded(t *testing.T, txHash common.Hash) bool { t.Helper() rpcClient, err := ethclient.Dial("http://localhost:8545") if err != nil { + t.Logf("Failed to dial RPC client: %v", err) return false } defer rpcClient.Close() receipt, err := rpcClient.TransactionReceipt(context.Background(), txHash) + if err != nil { + t.Logf("Failed to get transaction receipt for %s: %v", txHash.Hex(), err) + } return err == nil && receipt != nil && receipt.Status == 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/evm/based/go.sumis excluded by!**/*.sumapps/evm/single/go.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
apps/evm/based/go.mod(8 hunks)apps/evm/single/go.mod(8 hunks)execution/evm/execution.go(6 hunks)execution/evm/execution_test.go(5 hunks)execution/evm/test_helpers.go(1 hunks)scripts/test.mk(2 hunks)test/e2e/evm_aggregator_e2e_test.go(1 hunks)test/e2e/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/evm_aggregator_e2e_test.go
- execution/evm/execution.go
- apps/evm/single/go.mod
- scripts/test.mk
- test/e2e/go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
execution/evm/execution_test.go (2)
execution/evm/test_helpers.go (3)
SetupTestRethEngine(40-72)GetRandomTransaction(125-149)SubmitTransaction(152-160)test/e2e/evm_aggregator_e2e_test.go (1)
DOCKER_PATH(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Run E2E System Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (13)
execution/evm/test_helpers.go (6)
1-3: LGTM: Proper build constraints for EVM-specific code.The build tags correctly ensure this file is only compiled when the
evmbuild tag is present.
29-37: LGTM: Cryptographically secure JWT secret generation.The function correctly uses
crypto/rand.Readfor cryptographic randomness and proper error handling.
40-72: Well-structured Docker Compose setup with proper resource management.The function handles directory creation, JWT file management, and Docker lifecycle correctly. The cleanup functions ensure resources are properly released even if tests fail.
125-149: Excellent transaction helper with proper nonce management.The function correctly handles transaction signing, uses explicit parameters for better testability, and properly manages nonce state through a pointer. The use of legacy transactions is appropriate for testing.
152-160: LGTM: Clean transaction submission helper.Simple, focused function with proper resource cleanup and error handling.
175-190: LGTM: Robust genesis hash retrieval.The function properly handles HTTP requests, JSON decoding, and error validation. The timeout is appropriate for test environments.
execution/evm/execution_test.go (7)
65-65: LGTM: Correct usage of the new helper function.The call to
SetupTestRethEnginewith explicit parameters improves clarity and matches the new helper function signature.
85-85: Good improvement: Explicit nonce management.Moving from implicit global nonce to explicit local variable improves test clarity and reduces potential race conditions.
95-95: LGTM: Proper usage of the new transaction helper.The explicit parameter passing for
GetRandomTransactionimproves testability and makes dependencies clear.
98-98: LGTM: Clean transaction submission.Using the external
SubmitTransactionhelper maintains consistency and reduces code duplication.
144-144: LGTM: Consistent helper usage in sync test.Proper reuse of the
SetupTestRethEnginehelper for the sync test phase.
255-255: LGTM: Proper nonce initialization for manual test.Using
NonceAtto get the current nonce before generating transactions is the correct approach.
261-262: LGTM: Consistent usage of helper functions.The stress test properly uses the new helper functions with correct parameter passing.
| github.com/testcontainers/testcontainers-go v0.37.0 // indirect | ||
| github.com/testcontainers/testcontainers-go/modules/compose v0.37.0 // indirect |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Isolate test-only dependencies
Indirect pulls of testcontainers-go modules (v0.37.0) belong in the E2E test module (test/e2e/go.mod), not the production apps/evm/based module. Moving them will keep your production binary lean and dependencies better scoped.
🤖 Prompt for AI Agents
In apps/evm/based/go.mod at lines 301-302, the indirect dependencies on
testcontainers-go modules are included in the production module. Remove these
lines from this file and add them to the test/e2e/go.mod file instead to isolate
test-only dependencies from the production build.
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
left one suggestion, otherwise lGTM
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
execution/evm/execution_test.go (1)
238-238: Clean up commented debug statement.The commented log statement should be removed to keep the code clean.
- //t.Logf("Latest block: height=%d, hash=%s, txs=%d", blockNumber, blockHash.Hex(), txCount)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
execution/evm/execution_test.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
execution/evm/execution_test.go (2)
execution/evm/test_helpers.go (3)
SetupTestRethEngine(40-72)GetRandomTransaction(125-149)SubmitTransaction(152-160)test/e2e/evm_aggregator_e2e_test.go (1)
DOCKER_PATH(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run E2E System Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: lint / golangci-lint
- GitHub Check: lint / markdown-lint
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (6)
execution/evm/execution_test.go (6)
24-25: Constants updated to match new genesis configuration.The genesis hash and state root constants have been updated, which aligns with the Docker environment and genesis configuration changes mentioned in the PR objectives.
65-65: Good refactoring to use external helper function.The
SetupTestRethEnginehelper function properly encapsulates Docker Compose orchestration and JWT token management, improving code reuse and maintainability.Also applies to: 144-144
85-85: Improved nonce management with local variable.Using a local variable for nonce tracking instead of package-level globals is a good practice that improves test isolation and thread safety.
95-95: Excellent use of helper functions for transaction handling.The refactoring to use
GetRandomTransactionandSubmitTransactionhelpers improves code reuse and makes the test more maintainable. The nonce parameter passing with&lastNonceensures proper sequential nonce management.Also applies to: 98-98
255-255: Consistent nonce management pattern.The nonce retrieval and management in
TestSubmitTransactionfollows the same pattern as the main test, ensuring consistency across the test suite.
261-262: Proper use of helper functions in stress test.The stress test correctly uses the same helper functions, maintaining consistency and ensuring the same transaction creation logic is tested under load.
Overview
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores