Skip to content

(feat): disable pending block creation loop via flag#2175

Open
manav2401 wants to merge 6 commits intodevelopfrom
manav/disable-pending-loop
Open

(feat): disable pending block creation loop via flag#2175
manav2401 wants to merge 6 commits intodevelopfrom
manav/disable-pending-loop

Conversation

@manav2401
Copy link
Copy Markdown
Member

Description

Adds miner.disable-pending-block to disable pending block creation loop in miner/worker. All rpc queries over pending block will return nil.

Changes

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Changes only for a subset of nodes

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

Nodes audience

In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)

Checklist

  • I have added at least 2 reviewer or the whole pos-v1 team
  • I have added sufficient documentation in code
  • I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
  • Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
  • Includes RPC methods changes, and the Notion documentation has been updated

Cross repository changes

  • This PR requires changes to heimdall
    • In case link the PR here:
  • This PR requires changes to matic-cli
    • In case link the PR here:

Testing

  • I have added unit tests
  • I have added tests to CI
  • I have tested this code manually on local environment
  • I have tested this code manually on remote devnet using express-cli
  • I have tested this code manually on amoy
  • I have created new e2e tests into express-cli

Manual tests

Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

Copilot AI review requested due to automatic review settings March 31, 2026 13:43
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@claude
Copy link
Copy Markdown

claude bot commented Mar 31, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new miner.disable-pending-block flag to disable building/maintaining the miner “pending block” snapshot (primarily used to serve pending RPC queries) and wires it through CLI/config into the miner worker.

Changes:

  • Add DisablePendingBlock to miner/sealer configs and plumb it from CLI/config into the miner.
  • Skip commitWork / pending-tx snapshot maintenance in miner/worker when the flag is enabled for non-producing nodes.
  • Add worker test coverage for pending snapshot behavior when the flag is enabled/disabled, plus CLI docs/config examples.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
miner/worker.go Conditionally skips pending snapshot generation/maintenance based on the new flag.
miner/worker_test.go Adds TestDisablePendingBlock to validate pending snapshot behavior.
miner/miner.go Introduces DisablePendingBlock in miner config + default value.
internal/cli/server/flags.go Exposes miner.disable-pending-block CLI flag.
internal/cli/server/config.go Adds sealer config field, default value, and wiring into n.Miner.
docs/cli/server.md Documents the new CLI flag.
docs/cli/example_config.toml Adds example TOML setting.
docs/cli/default_config.toml Adds default TOML setting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.90%. Comparing base (e1ddb72) to head (6fa492e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2175      +/-   ##
===========================================
- Coverage    51.94%   51.90%   -0.05%     
===========================================
  Files          884      884              
  Lines       155431   155445      +14     
===========================================
- Hits         80734    80679      -55     
- Misses       69496    69558      +62     
- Partials      5201     5208       +7     
Files with missing lines Coverage Δ
internal/cli/server/config.go 63.87% <100.00%> (+0.02%) ⬆️
internal/cli/server/flags.go 100.00% <100.00%> (ø)
miner/miner.go 72.35% <ø> (ø)
miner/worker.go 73.10% <100.00%> (+1.11%) ⬆️

... and 22 files with indirect coverage changes

Files with missing lines Coverage Δ
internal/cli/server/config.go 63.87% <100.00%> (+0.02%) ⬆️
internal/cli/server/flags.go 100.00% <100.00%> (ø)
miner/miner.go 72.35% <ø> (ø)
miner/worker.go 73.10% <100.00%> (+1.11%) ⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@manav2401
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Apr 2, 2026

Claude finished @manav2401's task in 1m 6s —— View job


Code Review

  • Read changed files and gather context
  • Analyze implementation correctness
  • Check for bugs, security issues, and edge cases
  • Post review feedback

Overall this is a clean, well-scoped feature. The core logic is correct — the two guard points in mainLoop (skip commitWork on newWorkCh, skip tx application on txsCh) are the right places to disable pending block building. A few items to address:

1. pendingWorkBlock bookkeeping mismatch (bug)

miner/worker.go:838-839 — When the flag is set and the worker isn't running, commitWork is skipped via continue, but the newWorkLoop at line 722 has already called w.pendingWorkBlock.Store(currentBlock + 1) before sending to newWorkCh. Since commitWork never runs, its deferred pendingWorkBlock.Store(0) at line 1942 never fires.

This leaves pendingWorkBlock set to a non-zero value, which causes the dedup checks at lines 728-731 and 755-756 to skip subsequent commit() calls for the same block number. Effectively, chainHeadCh and veblopTimer paths will think work is already in progress and suppress future commits.

On non-validator nodes this is mostly benign since the worker isn't sealing, but it's still incorrect bookkeeping that could cause subtle issues if the node transitions to mining later. The fix is straightforward — reset pendingWorkBlock before continuing:

if w.config.DisablePendingBlock && !w.IsRunning() {
    w.pendingWorkBlock.Store(0)
    continue
}

Fix this →

2. Flag description could be more precise

internal/cli/server/flags.go:513 and docs/cli/server.md — The flag description says "RPC queries for 'pending' block will return nil", which is accurate only when the node is not a block producer (!w.IsRunning()). On a producing/validating node, the flag has no effect since the !w.IsRunning() guard lets commitWork proceed normally.

This is by design (the config comment on miner.go:71 says "on non block producer nodes"), but the CLI flag usage text doesn't mention that qualifier. Consider aligning:

"Disable the pending block creation loop on non block producer nodes. When set, RPC queries for 'pending' block will return nil on non-validator nodes"

The latest commit (82c089e) updated the description but it still reads slightly ambiguously — it says "on non block producer nodes" at the end, which could be clearer as a leading qualifier.

3. Test uses time.Sleep — flake risk

miner/worker_test.go:2985,3002 — Both subtests use time.Sleep(500ms) to wait for the pending block build. This is inherently flaky on slow CI. Consider using require.Eventually to poll w.pending():

require.Eventually(t, func() bool {
    block, _, _ := w.pending()
    return block != nil
}, 2*time.Second, 50*time.Millisecond, "pending block should be created")

For the "nil" case, require.Never (or a short sleep followed by a single check) is acceptable since you can't poll for something to not happen, but it's worth noting in a comment.

4. No issues with security or consensus

The flag only affects the pending block snapshot path for non-producing nodes. It does not touch commitWork for validators, block sealing, header verification, or any consensus-critical path. The !w.IsRunning() guard correctly ensures producing nodes are unaffected. No security concerns.


Summary: The main actionable item is #1 (reset pendingWorkBlock). Items #2 and #3 are minor improvements for clarity and test reliability.
|

Copilot AI review requested due to automatic review settings April 3, 2026 06:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — the substantive bugs from prior review have been addressed (pendingWorkBlock reset added, tests improved with require.Never/Eventually).

Extended reasoning...

Overview

This PR adds a miner.disable-pending-block boolean flag that, when set on a non-validator node (\!w.IsRunning()), skips commitWork in mainLoop and suppresses tx application via the txsCh path. Changes span miner config, CLI flags, config struct, worker logic, and tests across 8 files.

Security risks

No security risks. The flag only affects the pending block snapshot path for non-producing nodes; it does not touch block sealing, header verification, or consensus-critical code. The \!w.IsRunning() guard ensures validators are unaffected.

Level of scrutiny

Medium — the miner/worker is production-critical, but the code change is small and well-contained. The two guard points added are the correct places to intercept pending-block work.

Other factors

The two substantive bugs flagged in my prior review have been addressed: w.pendingWorkBlock.Store(0) is now present before the continue (fixing the veblopTimer spin issue), and the test was upgraded from time.Sleep to require.Never/require.Eventually (fixing the false-positive/flake risk). The only remaining finding is a [Nit]-tagged documentation inconsistency across four files about whether pending RPC queries return nil or an error — that inline comment has been posted separately.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 3, 2026

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