Skip to content

Comments

test: faster block time#3094

Draft
alpe wants to merge 1 commit intomainfrom
alex/bench1
Draft

test: faster block time#3094
alpe wants to merge 1 commit intomainfrom
alex/bench1

Conversation

@alpe
Copy link
Contributor

@alpe alpe commented Feb 20, 2026

Overview

Test with 10ms block time

@github-actions
Copy link
Contributor

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 20, 2026, 2:44 PM

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 focuses on enhancing the performance of EVM contract benchmarks. By reducing the EVM node's block time and refining the transaction receipt polling mechanism, the changes aim to achieve faster and more efficient test runs, thereby improving the overall development and testing workflow.

Highlights

  • Benchmark Optimization: Optimized EVM node block time in benchmarks to 10ms, significantly speeding up test execution.
  • Transaction Receipt Polling: Improved transaction receipt waiting logic by inlining require.Eventually with a faster 10ms polling interval, reducing variance and avoiding RPC overload.
  • Code Cleanup: Cleaned up test code by removing the now-unused waitForReceipt helper function and its associated ethclient import.

🧠 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
  • test/e2e/evm_contract_bench_test.go
    • Removed unused ethclient import.
    • Configured the EVM node to use a 10ms block time for benchmarks.
    • Replaced the waitForReceipt helper function call with an inline require.Eventually block for transaction inclusion, using a faster polling interval.
    • Deleted the waitForReceipt helper function.
Activity
  • No specific human activity (comments, reviews, or progress updates) has been recorded for this pull request yet.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

  1. 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.

@claude
Copy link
Contributor

claude bot commented Feb 20, 2026

Claude finished @alpe's task in 2m 10s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze changes
  • Post review

This is a focused, clean optimization to the EVM contract benchmark. The changes are small and well-motivated. A few observations below.

What Changed

Before After
No explicit block time (default applied) --evnode.node.block_time=10ms
waitForReceipt helper (50ms polling) Inlined require.Eventually (10ms polling)
ethclient import used by helper Import removed

Observations

Polling interval matches block time (good)

The polling interval in require.Eventually was reduced from 50ms to 10ms to match the new block time. This is a sensible alignment — with a 10ms block time, a 50ms poll interval would cause 4-5 wasted cycles on average. At 10ms, you expect roughly 1 extra poll cycle per transaction. The comment explains the reasoning well.

DA block time not explicitly overridden (test/e2e/evm_contract_bench_test.go:53-59)

The benchmark now sets --evnode.node.block_time=10ms but does not override --evnode.da.block_time. Based on evm_test_common.go:202, the default DA block time is 1s. If the DA submission pipeline has any backpressure effect on block production in this test setup, the 10ms node block time and 1s DA block time could create mismatch. It's worth confirming that DA submission is fire-and-forget here and doesn't gate block production in a way that would make the 10ms block time setting ineffective or misleading in benchmark results.

Receipt wait timeout remains 2s (minor)

The require.Eventually timeout of 2*time.Second stays the same (line 109). With a 10ms block time this is a 200x margin, which is generous — fine for correctness but worth noting that a tighter timeout (e.g., 500ms) would make flakiness more visible if a transaction isn't being included promptly. Not a blocker.

nolint format (pre-existing, not this PR)

Line 50 uses // nolint: errcheck (space before colon). golangci-lint expects //nolint:errcheck. Pre-existing issue, unrelated to this PR.

Context cancellation in closure (minor, acceptable)

The require.Eventually closure at line 107 captures ctx from b.Context(). If the benchmark context is cancelled mid-poll (e.g., timeout), the closure will keep returning false (because the RPC call fails) rather than short-circuiting. It will then hit the 2s timeout and fail with "transaction not included" rather than a cleaner context error. This is acceptable for test code, but noting it for awareness.

Removed return value of waitForReceipt (correct)

The old helper returned *types.Receipt but the call site never used the return value. Removing it via inlining is correct.


Summary

The changes are correct and achieve the intended goal of faster benchmark execution. The main question worth confirming is whether the DA block time (still 1s by default) has any measurable effect on the benchmark's block production rate when block_time=10ms. If the two paths are independent, the change is straightforward to approve.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request speeds up the BenchmarkEvmContractRoundtrip test by reducing the block time to 10ms and adjusting the transaction receipt polling interval accordingly. The changes are effective and include removing a now-unused helper function. I've added one suggestion to improve maintainability by using a constant for the new block time and polling interval.

"--evnode.instrumentation.tracing_endpoint", collector.endpoint(),
"--evnode.instrumentation.tracing_sample_rate", "1.0",
"--evnode.instrumentation.tracing_service_name", "ev-node-bench",
"--evnode.node.block_time=10ms",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The hardcoded value 10ms is also used for the polling interval on line 109. To improve maintainability and prevent these values from diverging in the future, consider defining a constant for it.

For example, you could add const fastBlockTime = 10 * time.Millisecond at the start of the function and use it here ("--evnode.node.block_time=" + fastBlockTime.String()) and for the polling interval (fastBlockTime).

@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.01%. Comparing base (05ce69e) to head (d7823ae).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3094      +/-   ##
==========================================
- Coverage   61.03%   61.01%   -0.02%     
==========================================
  Files         113      113              
  Lines       11544    11544              
==========================================
- Hits         7046     7044       -2     
- Misses       3699     3701       +2     
  Partials      799      799              
Flag Coverage Δ
combined 61.01% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant