iframe-proxy

0xDevNinja · 2026-06-16T11:58:00Z

Changes

Added the ability to pause and resume local block processing on a running node, intended for testing and diagnostics (e.g. let blocks accumulate in the processing queue and then drain them on demand).
BlockchainProcessor now exposes PauseProcessing(), ResumeProcessing() and an IsProcessingPaused flag via IBlockProcessingQueue. While paused, the processing loop parks after a block becomes available but before it is dequeued, so suggested blocks keep accumulating in the queue until processing is resumed. StopAsync resumes first so shutdown cannot deadlock on a paused processor.
Added the admin_setBlockProcessingPaused(bool paused) JSON-RPC method, which toggles the state and returns whether processing is paused after the call.

This enables the scenario from the issue: keep the EL online and responding while paused, stack blocks in the processing queue, then resume to process the backlog.

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

BlockchainProcessorTests.Can_pause_and_resume_block_processing drives the real processor: after pausing, a fully-allowed block is enqueued and verified to remain unprocessed (the mock branch processor records a hash the moment it starts, so an empty entry proves the parked loop never dispatched it), then resuming processes it and it becomes the new head. Verified the test fails if the pause park is removed.
AdminModuleTests.Admin_setBlockProcessingPaused_delegates_to_processing_queue verifies the RPC method delegates to PauseProcessing/ResumeProcessing and returns the resulting state.
Full BlockchainProcessorTests (21) and AdminModuleTests (48) fixtures pass.

Documentation

Requires documentation update

Yes
No

The new admin_setBlockProcessingPaused method should be added to the admin JSON-RPC namespace docs. The [JsonRpcMethod] description/examples are included on the interface so the generated reference is populated.

Requires explanation in Release Notes

Yes
No

…rocessing Adds the ability to pause and resume local block processing on a running node, which is useful for testing and diagnostics (e.g. letting blocks accumulate in the processing queue and then draining them on demand). - BlockchainProcessor gains PauseProcessing/ResumeProcessing and an IsProcessingPaused flag (exposed via IBlockProcessingQueue). While paused, the processing loop parks after a block becomes available but before it is dequeued, so suggested blocks keep accumulating in the queue until processing is resumed. StopAsync resumes first so shutdown cannot deadlock on a paused processor. - New admin_setBlockProcessingPaused(bool) JSON-RPC method toggles the state and returns whether processing is paused after the call. Closes NethermindEth#9914

svlachakis · 2026-06-16T13:00:14Z

claude · 2026-06-16T13:00:48Z

Claude finished @svlachakis's task in 4m 35s —— View job

Code Review

Gather context and understand the request
Read relevant rule files
Review changed files
Analyze correctness, edge cases, and potential issues
Post inline comments for specific issues
Provide final summary

Summary

The implementation is generally clean. The volatile TaskCompletionSource? + Lock pattern for pause state is correct, the WaitAsync(CancellationToken) call in the inner loop is cooperative with cancellation, and the StopAsync → ResumeProcessing() sequence correctly prevents deadlock on shutdown. The RPC wiring, DI registration, and admin namespace placement are all appropriate.

Two medium findings should be addressed before merge:

Findings

Medium

#	Location	Finding
1	`BlockchainProcessor.cs:359–366`	Pause granularity is batch-level, not block-level. `ProcessBlocks()` drains the entire queue via `TryRead` in one synchronous call. Calling `PauseProcessing()` while `ProcessBlocks()` is executing does not stop the current batch. The PR description says "before it is dequeued" which implies per-block semantics; the actual semantics are per-batch. This is acceptable for a diagnostic tool but must be clarified in the `<remarks>` of the interface doc.
2	`IBlockProcessingQueue.cs:43–53`	Breaking interface change without default implementations. The three new members have no `default` bodies. Any external plugin that manually implements `IBlockProcessingQueue` will fail to compile. All internal callers are fine (NSubstitute mocks + the single `BlockchainProcessor` implementation), but plugin authors are affected. Recommend adding `default` throw bodies.

Low

#	Location	Finding
3	`BlockchainProcessorTests.cs:473`	`Thread.Sleep(500)` for a negative "was not processed" assertion is timing-sensitive.
4	`BlockchainProcessorTests.cs:483`	`ThenBecomesNewHead` duplicates the polling logic already in `AfterBlock.BecomesNewHead`.
5	`BlockchainProcessor.cs:234`	`ResumeProcessing()` logs "Block processing resumed." even when called from `StopAsync` during shutdown, which looks like an operator action in the logs.

What looks good

volatile + Lock write protection is correct; no lock held during the await, avoiding deadlock risk.
Inner while (not if) re-checks pause after each wake-up, handling the PauseProcessing → ResumeProcessing → PauseProcessing rapid-fire case correctly.
StopAsync calls ResumeProcessing() before cancellation — shutdown cannot deadlock.
TaskCreationOptions.RunContinuationsAsynchronously prevents continuations running inline on the TrySetResult caller's thread.
Admin RPC is correctly placed in the restricted admin namespace.
IsProcessingPaused reads _resumeSignal without a lock — safe because volatile provides the necessary visibility guarantee for a single read.

- Per-block pause granularity: re-check pause before each dequeue in ProcessBlocks so a mid-drain pause takes effect at the next block boundary instead of after the whole batch drains. - Non-breaking interface change: default implementations on IBlockProcessingQueue (throw NotSupportedException / false) so external implementers keep compiling. - Suppress the "Block processing resumed." log when ResumeProcessing runs from StopAsync (internal shutdown unblock, not an operator action); the pause log now fires only on the real transition (idempotency). - Tests: replace the Thread.Sleep(500) negative assertion with a bounded wait on the new-head signal that must not fire; consolidate ThenBecomesNewHead/BecomesNewHead into a shared WaitForHead helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

svlachakis · 2026-06-16T13:58:00Z

@claude review again

claude · 2026-06-16T14:03:21Z

Claude finished @svlachakis's task in 4m 53s —— View job

Code Review (re-review)

Gather context and understand the request
Read relevant rule files
Review changed files for current state
Check whether previous findings were addressed
Look for new issues
Post final summary

Previous Findings — All Addressed ✅

The fix commit dead322 cleanly resolves all five previous findings:

#	Severity	Finding	Status
1	Medium	Pause granularity was batch-level	✅ `ProcessBlocks()` now re-checks `IsProcessingPaused` before each `TryRead()`, providing per-block-dequeue granularity. Interface docs updated to match.
2	Medium	Breaking interface change — no default implementations	✅ `PauseProcessing()` / `ResumeProcessing()` default to `throw new NotSupportedException()`, `IsProcessingPaused` defaults to `false`. External implementations keep compiling.
3	Low	`Thread.Sleep(500)` timing-based negative assertion	✅ Replaced with `_resetEvent.WaitOne(200)` + three direct state assertions on `_branchProcessor.Processed`, `IsProcessingPaused`, and `_blockTree.Head?.Hash` that are not timing-dependent.
4	Low	Duplicate `ThenBecomesNewHead` / `BecomesNewHead` polling logic	✅ Both delegate to a shared private `WaitForHead(block, message)` helper.
5	Low	`"Block processing resumed."` logged during shutdown	✅ `ResumeProcessing(bool duringShutdown)` overload suppresses the log when called from `StopAsync`.

New-Pass Findings

No new Critical, High, or Medium issues found. A few observations worth noting for completeness:

DI wiring verified: MainProcessingContext.cs registers IBlockProcessingQueue as the BlockchainProcessor instance. The DbMonitoringModule.Intercept<IBlockProcessingQueue>() only subscribes to BlockAdded/BlockRemoved events — it does not wrap the object — so the BlockchainProcessor that admin_setBlockProcessingPaused receives is the real implementation with all new methods.

Correctness of the per-block pause check: while (!IsProcessingPaused && _blockQueue.Reader.TryRead(...)) reads _resumeSignal via volatile without the lock, which is safe. If PauseProcessing() races between the pause check and TryRead(), exactly one block may slip through — this is the documented "parks before the next block" contract.

Shutdown deadlock prevention: StopAsync calls ResumeProcessing(duringShutdown: true) before CancelDisposeAndClear(ref _loopCancellationSource), so a paused loop is always unblocked before cancellation is signalled. Correct.

TaskCreationOptions.RunContinuationsAsynchronously retained — continuations from TrySetResult() do not run inline on the caller's thread.

Summary

All five findings from the first review have been addressed cleanly and correctly. The implementation is correct, the tests are sound, and the DI wiring is verified. This PR is ready to merge.

svlachakis · 2026-06-16T14:19:16Z

I'm not sure that's correct and in the best shape possible, will dig a bit into it

svlachakis · 2026-06-16T22:01:52Z

0xDevNinja requested review from LukaszRozmej, MarekM25, asdacap, benaadams, flcl42, smartprogrammer93 and svlachakis as code owners June 16, 2026 11:58

github-actions Bot added new feature rpc labels Jun 16, 2026