Conversation
… Actions workflow.
|
Claude finished @alpe's task in 1m 30s —— View job Code Review: feat: introduce e2e benchmarking
SummaryGood foundation for E2E benchmarking with OTLP trace collection. The IssuesCorrectness / Design
The benchmark uses A simpler pattern: pre-build Missing shutdown error propagation ( defer collector.close() // nolint: errcheck // test onlychatton already noted this. The
require.Eventually(t, ..., 2*time.Second, 50*time.Millisecond, ...)The E2E test in Global var globalNonce uint64 = 0This package-level mutable state is reset in OTLP collector has no c.server = &http.Server{Handler: mux}The HTTP server has no timeouts set. A misbehaving ev-node could stall the benchmark indefinitely. Add at least Error Handling
out, _ := proto.Marshal(resp)If marshaling the empty response fails (unlikely, but possible), the handler sends an empty body with
The function verifies Code QualityMagic numbers and hex literals ( Gemini's suggestion is valid. The repeated hex const (
StoreValue42Hex = "0x000000000000000000000000000000000000000000000000000000000000002a"
StoreTxGasLimit = 500_000
DefaultGasPrice = 30_000_000_000
)
Minor: Unused Both functions accept a
SecurityHardcoded private key ( TestPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"This is a test-only key for a local devnet, so the risk is low. However, if the chain ID or genesis config were accidentally reused against a real network, funds at this address would be at risk. Consider adding a comment explicitly stating this key controls a well-known test account and must never be used outside local devnets. Weak passphrase ( TestPassphrase = "secret"Same rationale — acceptable for local tests, but worth a comment clarifying test-only scope. WorkflowNo go test -tags evm -bench=. -benchmem -run='^$' -timeout=10m --evm-binary=...Without Missing
Positive Observations
|
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a foundational end-to-end benchmarking framework for EVM contract interactions, specifically targeting the performance of store and retrieve operations. The primary goal is to establish a baseline for measuring latency and throughput in a realistic environment. To support this, existing test infrastructure has been generalized to accommodate both traditional tests and the new benchmarks, ensuring consistent utility across different testing paradigms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3078 +/- ##
==========================================
+ Coverage 61.11% 61.14% +0.03%
==========================================
Files 113 113
Lines 11444 11444
==========================================
+ Hits 6994 6998 +4
+ Misses 3661 3657 -4
Partials 789 789
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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an end-to-end benchmark for EVM contract roundtrip latency and refactors existing test helper functions to accept testing.TB for improved reusability. A security audit identified two medium-severity vulnerabilities related to hardcoded secrets (a private key and a passphrase) within the test suite. While these are confined to test files, they represent a significant security anti-pattern. Additionally, suggestions were made for the new benchmark file to improve maintainability by using constants for magic values and to ensure compatibility with older Go versions. Remediation of the hardcoded secrets is recommended to improve the security posture and adhere to best practices.
| defer cleanup() | ||
|
|
||
| ctx := b.Context() | ||
| privateKey, err := crypto.HexToECDSA(TestPrivateKey) |
There was a problem hiding this comment.
A hardcoded private key (TestPrivateKey) is used. Hardcoding secrets, even for testing, is a significant security risk. This key could be inadvertently used in production-like environments or on public testnets, leading to compromised assets. It also sets a poor security precedent. Keys should be loaded from environment variables or a secrets manager, or generated dynamically for tests.
test/e2e/evm_test_common.go
Outdated
| // - genesisHash: Hash of the genesis block for chain validation | ||
| // - endpoints: TestEndpoints struct containing unique port assignments | ||
| func setupSequencerNode(t *testing.T, sut *SystemUnderTest, sequencerHome, jwtSecret, genesisHash string, endpoints *TestEndpoints) { | ||
| func setupSequencerNode(t testing.TB, sut *SystemUnderTest, sequencerHome, jwtSecret, genesisHash string, endpoints *TestEndpoints) { |
There was a problem hiding this comment.
A weak, hardcoded passphrase ("secret") is used for the test sequencer's key file. Hardcoding credentials, especially weak ones, is a poor security practice that can be replicated in production. Passphrases should be loaded from a secure source like environment variables or a secrets manager.
| storeData, err := hexutil.Decode("0x000000000000000000000000000000000000000000000000000000000000002a") | ||
| require.NoError(b, err) | ||
|
|
||
| const maxIter = 1024 | ||
| signedTxs := make([]*types.Transaction, maxIter) | ||
| for i := range maxIter { | ||
| tx := types.NewTx(&types.LegacyTx{ | ||
| Nonce: nonce + uint64(i), | ||
| To: &contractAddr, | ||
| Value: big.NewInt(0), | ||
| Gas: 500000, | ||
| GasPrice: big.NewInt(30000000000), | ||
| Data: storeData, | ||
| }) | ||
| signedTxs[i], err = types.SignTx(tx, signer, privateKey) | ||
| require.NoError(b, err) | ||
| } | ||
|
|
||
| expected := common.HexToHash("0x000000000000000000000000000000000000000000000000000000000000002a").Bytes() |
There was a problem hiding this comment.
The benchmark uses several magic numbers and strings for transaction parameters and expected values. To improve readability and maintainability, consider defining these as constants at the top of the file.
For example:
const (
benchGasLimit = 500000
benchGasPrice = 30000000000
storedValueHex = "0x000000000000000000000000000000000000000000000000000000000000002a"
)Then you can use these constants in the code:
storeData, err := hexutil.Decode(storedValueHex)
// ...
Gas: benchGasLimit,
GasPrice: big.NewInt(benchGasPrice),
// ...
expected := common.HexToHash(storedValueHex).Bytes()|
|
||
| const maxIter = 1024 | ||
| signedTxs := make([]*types.Transaction, maxIter) | ||
| for i := range maxIter { |
There was a problem hiding this comment.
The loop for i := range maxIter uses a new feature from Go 1.22. If this project needs to maintain compatibility with older Go versions, this should be changed to a traditional for loop to avoid breaking the build on older toolchains.
If Go 1.22 is the minimum required version for the project, then this is fine.
| for i := range maxIter { | |
| for i := 0; i < maxIter; i++ { |
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
I know the PR is still in draft so just leaving some thoughts! nice work so far
| // otlpCollector is a lightweight OTLP/HTTP receiver that collects trace spans | ||
| // in memory. It serves the /v1/traces endpoint that the node's OTLP exporter | ||
| // posts protobuf-encoded ExportTraceServiceRequest messages to. | ||
| type otlpCollector struct { |
There was a problem hiding this comment.
this is a really nice idea, I imagine this would be used quite a lot in these tests, it's probably worth pulling this out into a separate file file / package to be imported into these tests
There was a problem hiding this comment.
It definitively makes sense to pull this out when it is used by a second benchmark.
| // | ||
| // PATH="/path/to/binaries:$PATH" go test -tags evm \ | ||
| // -bench BenchmarkEvmContractRoundtrip -benchmem -benchtime=5x \ | ||
| // -run='^$' -timeout=10m -v --evm-binary=/path/to/evm . |
There was a problem hiding this comment.
ah we need to specify the evm binary because we're mixing and matching docker vs local binaries right?
If we are going to create a new set of tests, I would say it's worth while to just do everything via docker for these, we likely won't run them as frequently so I don't think them being slightly heavier is a bad thing in that regard.
It can also be quite confusing having a mix WDYT?
There was a problem hiding this comment.
As I said in the standup. Standalone is my personal preference but I would go with the majority. Can we get this in and refactor in a new PR?
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
LGTM happy to get this merged in as a jumping off point 💪
| n, err := new(reth.NodeBuilder). | ||
| WithTestName(testName). | ||
| WithLogger(logger). | ||
| WithImage(reth.DefaultImage()). | ||
| WithBin("ev-reth"). |
There was a problem hiding this comment.
I don't think any of this is required, the default behaviour we get from NewNodeBuilderWithTestName should be fine, if we want to set a logger we can just chain the logger after directly.
3 participants
Initial workflow
Runs and publishes results when running on main only