Skip to content

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jan 5, 2026

Overview

Enable reaper poll interval to be configurable to help reduce E2E tx latency

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 5, 2026, 3:15 PM

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @chatton, 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 enhances the system's configurability by allowing users to set the interval for the "reaper" component's polling of the execution layer. This change provides greater control over transaction detection latency and the associated RPC load, enabling operators to optimize performance based on their specific needs.

Highlights

  • Configurable Reaper Poll Interval: The pull request introduces a new configuration option to control the interval at which the "reaper" component polls the execution layer for new transactions.
  • Reduced Transaction Latency: By making the reaper poll duration configurable, users can now fine-tune this setting to potentially reduce end-to-end transaction latency.
  • New Configuration Flag: A new command-line flag, node.scrape_interval, has been added to allow users to specify the desired poll duration.

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

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 Jan 5, 2026

Claude finished @chatton's task in 1m 54s —— View job


Comprehensive Code Review

I've completed a thorough review of this PR. Overall, this is a well-implemented feature that makes the reaper polling interval configurable. The code quality is good, and the changes are clean and focused. Below are my detailed findings:


Summary

Purpose: Enable configuration of the reaper poll interval to reduce E2E transaction latency.

Changes:

  • Added new ScrapeInterval configuration option to NodeConfig
  • Removed now-unused DefaultInterval constant from reaping package
  • Added *.out to .gitignore (addresses reviewer request)
  • Updated tests to include the new configuration parameter

Code Quality & Architecture ✅

Strengths:

  • Clean separation of concerns - configuration defined in config package, used in block package
  • Follows existing patterns for adding configuration flags (consistent with BlockTime, LazyBlockInterval, etc.)
  • The DurationWrapper type is used correctly for YAML/time.Duration parsing
  • Removal of the DefaultInterval constant eliminates dead code (addresses reviewer feedback)
  • The change is minimal and focused - doesn't introduce architectural complexity

Observations:

  • The implementation correctly passes config.Node.ScrapeInterval.Duration to reaping.NewReaper() (block/components.go:230)
  • Validation in reaping.NewReaper() ensures the interval cannot be zero, preventing misconfiguration

Configuration Management ✅

Strengths:

  • Comprehensive documentation in the struct tag comment explaining purpose, trade-offs, and providing examples
  • Default value of 1 second is reasonable and maintains existing behavior
  • Configuration can be set via CLI flag, YAML file, or environment variable (standard pattern)
  • The flag is properly registered in AddFlags() and tested in TestAddFlags()

Documentation Quality:

ScrapeInterval DurationWrapper `mapstructure:"scrape_interval" yaml:"scrape_interval" 
comment:"Interval at which the reaper polls the execution layer for new transactions. 
Lower values reduce transaction detection latency but increase RPC load. 
Examples: \"250ms\", \"500ms\", \"1s\"."`

This is excellent - it clearly explains:

  • What the parameter controls
  • The trade-off (latency vs RPC load)
  • Concrete examples

Testing Coverage ⚠️ Needs Improvement

What's Good:

  • Test flag count updated correctly (44 → 45)
  • Flag assertion added in TestAddFlags() to verify the new flag exists with correct default
  • Test helper updated to set ScrapeInterval in test configurations

What's Missing (addresses reviewer comment):
The reviewer (gemini-code-assist) correctly identified that while you added the flag assertion on line 66 of config_test.go:

assertFlagValue(t, flags, FlagScrapeInterval, DefaultConfig().Node.ScrapeInterval)

This only verifies the default value. There's no test that verifies:

  1. The flag can be set via command line and properly overrides defaults
  2. The flag can be set via YAML configuration
  3. The configured value is actually used by the reaper

Recommendation:
Consider adding a test case to TestLoad() that sets FlagScrapeInterval via flag arguments and verifies it's properly loaded, similar to how FlagBlockTime is tested:

"--rollkit.node.scrape_interval", "2s",

Then assert:

assert.Equal(t, 2*time.Second, config.Node.ScrapeInterval.Duration, "ScrapeInterval should be set from flag")

Error Handling & Resilience ✅

Strengths:

  • Validation in reaping.NewReaper() prevents zero interval:
    if scrapeInterval == 0 {
        return nil, errors.New("scrape interval cannot be empty")
    }
  • Error wrapping follows Go best practices
  • The reaper already has exponential backoff for failures, independent of the scrape interval

No Issues Found - Error handling is appropriate for this change.


Security Considerations ✅

Analysis:

  • No security implications identified
  • Configuration parameter doesn't expose sensitive data
  • No injection or validation bypass risks
  • Lower scrape intervals increase RPC load but that's an operational concern, not a security issue

Performance & Resource Efficiency ✅

Trade-offs Properly Documented:
The configuration comment correctly identifies the performance trade-off:

  • Lower values: Reduce transaction detection latency (better for E2E testing)
  • Higher values: Reduce RPC load on the execution layer

Implementation Correctness:

  • The interval is used in reaperLoop() via time.NewTicker(r.interval) (reaper.go:92)
  • The ticker is properly reset during backoff scenarios (reaper.go:118, 124)
  • No resource leaks introduced

Recommendation:
For production use, operators should monitor RPC load and adjust based on their needs. The 1-second default is reasonable.


Documentation ✅

Strengths:

  • Flag description is clear: "interval at which the reaper polls the execution layer for new transactions"
  • YAML comment provides excellent guidance with examples
  • PR description clearly states the motivation: "reduce E2E tx latency"

No Additional Documentation Needed - The inline documentation is sufficient.


Code Consistency & Style ✅

Strengths:

  • Follows existing patterns exactly (compare with BlockTime, LazyBlockInterval)
  • Flag naming convention matches: FlagScrapeInterval with prefix FlagPrefixEvnode + "node.scrape_interval"
  • YAML field naming follows snake_case convention: scrape_interval
  • Go naming follows camelCase: ScrapeInterval

No Style Issues Found


Review of Existing Reviewer Feedback

gemini-code-assist's feedback:

  1. ✅ Remove unused DefaultInterval constant - ADDRESSED in commit 203c668
  2. ⚠️ Add test coverage for the new flag - PARTIALLY ADDRESSED (see Testing section above)

julienrbrt's feedback:

  1. ✅ Add *.out to .gitignore - ADDRESSED in commit bb341eb

Final Recommendations

Must Address:

None - The implementation is functionally correct and safe to merge.

Should Consider:

  1. Testing: Add an integration test or extend TestLoad() to verify the flag can be set via command line and properly overrides YAML/defaults. This would provide stronger confidence that the configuration flows end-to-end.

  2. Optional - Validation: Consider adding validation to ensure ScrapeInterval is reasonable (e.g., >= 100ms). While not strictly necessary, it could prevent operator mistakes. However, this may be over-engineering for this use case.

Nice to Have:

  1. Add a log message in reaper.Start() that includes the configured interval for operational visibility (already done! Line 87 of reaper.go logs the interval).

Conclusion

This is a clean, well-implemented feature that follows established patterns in the codebase. The code quality is high, documentation is excellent, and the changes are focused and minimal.

Approval Status: ✅ Recommended for merge after considering the testing feedback above.

The existing test coverage provides basic validation, and the implementation is safe. The suggestion for additional integration testing is a quality improvement rather than a blocking issue.


Great work! The implementation demonstrates good understanding of the codebase architecture and Go best practices.

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 introduces a new configuration option, scrape_interval, to control the reaper's polling duration. This is a valuable addition for tuning node performance, especially for reducing end-to-end transaction latency. The implementation looks good, adding the necessary flag, configuration property, and default value. I've included a couple of suggestions for improvement: one regarding test coverage for the new flag and another for code cleanup by removing a now-unused constant.

executor,
cacheManager,
reaping.DefaultInterval,
config.Node.ScrapeInterval.Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Now that the reaper interval is configurable and passed in here, the reaping.DefaultInterval constant appears to be unused. It would be good to remove it from block/internal/reaping/reaper.go to avoid dead code.

Copy link
Member

@julienrbrt julienrbrt Jan 5, 2026

Choose a reason for hiding this comment

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

+1 on this, cc @chatton

cmd.Flags().Duration(FlagLazyBlockTime, def.Node.LazyBlockInterval.Duration, "maximum interval between blocks in lazy aggregation mode")
cmd.Flags().Uint64(FlagReadinessWindowSeconds, def.Node.ReadinessWindowSeconds, "time window in seconds for calculating readiness threshold based on block time (default: 15s)")
cmd.Flags().Uint64(FlagReadinessMaxBlocksBehind, def.Node.ReadinessMaxBlocksBehind, "how many blocks behind best-known head the node can be and still be considered ready (0 = must be at head)")
cmd.Flags().Duration(FlagScrapeInterval, def.Node.ScrapeInterval.Duration, "interval at which the reaper polls the execution layer for new transactions")
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's great that you've added this new configuration flag. To ensure it's working as expected and to maintain test coverage, please consider adding a test case for FlagScrapeInterval in TestAddFlags within pkg/config/config_test.go. You'll also likely need to update the expectedFlagCount in that test to ensure all flags are accounted for.

@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.74%. Comparing base (66b491b) to head (c57b2af).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   58.73%   58.74%           
=======================================
  Files          90       90           
  Lines        8720     8722    +2     
=======================================
+ Hits         5122     5124    +2     
  Misses       3011     3011           
  Partials      587      587           
Flag Coverage Δ
combined 58.74% <100.00%> (+<0.01%) ⬆️

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.

@julienrbrt
Copy link
Member

Can you add *.out in gitignore?

@chatton chatton added this pull request to the merge queue Jan 6, 2026
Merged via the queue into main with commit 360499b Jan 6, 2026
27 of 28 checks passed
@chatton chatton deleted the cian/make-reaper-polling-configurable branch January 6, 2026 08:18
alpe added a commit that referenced this pull request Jan 6, 2026
* main: (34 commits)
  feat: make reaper poll duration configurable (#2951)
  chore!: move sequencers to pkg (#2931)
  feat: Ensure Header integrity on DA (#2948)
  feat(testda): add header support with GetHeaderByHeight method (#2946)
  chore: improve code comments clarity (#2947)
  chore(sequencers): optimize store check (#2945)
  fix: make evm_execution more robust (#2942)
  fix(sequencers/single): deterministic queue (#2938)
  fix(block): fix init logic sequencer for da epoch fetching (#2926)
  feat: use DA timestamp (#2939)
  chore: improve code comments clarity (#2943)
  build(deps): bump libp2p (#2937)
  build(deps): Bump actions/cache from 4 to 5 (#2934)
  build(deps): Bump actions/download-artifact from 6 to 7 (#2933)
  build(deps): Bump actions/upload-artifact from 5 to 6 (#2932)
  feat: DA Client remove interface part 3, replace types with new code (#2910)
  DA Client remove interface: Part 2.5, create e2e test to validate that a blob is posted in DA layer. (#2920)
  fix(syncing): skip forced txs checks for p2p blocks (#2922)
  build(deps): Bump the all-go group across 5 directories with 5 updates (#2919)
  chore: loosen syncer state check (#2927)
  ...
alpe added a commit that referenced this pull request Jan 8, 2026
* main:
  chore: fix some minor issues in the comments (#2955)
  feat: make reaper poll duration configurable (#2951)
  chore!: move sequencers to pkg (#2931)
  feat: Ensure Header integrity on DA (#2948)
  feat(testda): add header support with GetHeaderByHeight method (#2946)
  chore: improve code comments clarity (#2947)
  chore(sequencers): optimize store check (#2945)
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.

3 participants