(feat): disable pending block creation loop via flag#2175
(feat): disable pending block creation loop via flag#2175
Conversation
There was a problem hiding this comment.
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.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
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
DisablePendingBlockto miner/sealer configs and plumb it from CLI/config into the miner. - Skip
commitWork/ pending-tx snapshot maintenance inminer/workerwhen 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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
... and 22 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
@claude review |
|
Claude finished @manav2401's task in 1m 6s —— View job Code Review
Overall this is a clean, well-scoped feature. The core logic is correct — the two guard points in 1.
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|



Description
Adds
miner.disable-pending-blockto disable pending block creation loop in miner/worker. All rpc queries overpendingblock will return nil.Changes
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
Cross repository changes
Testing
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