@asdacap @LukaszRozmej do we still consider merging it?

@claude review

Claude finished @LukaszRozmej's task in 4m 57s —— View job

PR Review: feat: implement ProgressLogger for Full Pruning, Trie Verification and Analytics

• Gather context and read repository guidelines
• Read changed files
• Analyze code for correctness and issues
• Post inline findings

Summary

The PR integrates the existing ProgressLogger abstraction into several long-running operations (full pruning, trie verification, supply verification, migration). The intent is good, but there are 2 High and 4 Medium issues that need to be addressed before this is production-ready.

High

Medium

Low

• LogProgress(string state) method in CopyTreeVisitor is now dead code (never called) — should be removed.
• SupplyVerifier.VisitLeaf doesn't call _progressLogger.Update, making the node count incomplete. The removed per-branch/per-leaf balance logs also eliminated useful intermediate diagnostics.
• Indentation inconsistency in StateSyncFeedTestsBase.cs:259.
• Explanatory comments throughout describe what the code does rather than why (violates project coding-style guidelines).

Not mergeable — H1 means the feature's core value (readable progress output) is broken as-is (wrong units), and H2 is a diagnostic regression in a path used for trie health monitoring.

@claude review

Claude finished @LukaszRozmej's task in 4m 50s —— View job

PR Review: feat: add ProgressReporter abstraction

• Gather context and read repository guidelines
• Read changed files
• Analyze code for correctness and issues
• Post inline findings and summary

Summary

Clean, focused PR. All High and Medium findings from the two prior review rounds are resolved. The scope is now clearly the ProgressReporter abstraction itself plus the four application sites. All callers are updated, all disposal lifetimes are properly closed (using), and the NoErrorLimboLogs / LimboNoErrorLogger substitution preserves test fail-fast semantics.

Two Low findings remain (inline comments posted):

Neither blocks merge. The abstraction is correct and all previous issues are addressed.

@claude re-review

@claude re-review

Claude finished @LukaszRozmej's task in 4m 32s —— View job

Re-review: feat: add ProgressReporter abstraction

• Gather context and read repository guidelines
• Examine current diff vs master (18 files since merge base 9393692)
• Read changed files
• Analyze findings
• Post updated findings

Scope of this review

28 commits since the last review. The PR has grown significantly from the single-file TotalDifficultyFixMigration change to a full abstraction rollout across:

• New ProgressReporter abstraction (Nethermind.Core)
• Three block-tree visitors: DbBlocksLoader, StartupBlockTreeFixer, ReceiptsVerificationVisitor
• Era1 + EraE export/import
• SnapshotDownloader (replaces the custom ProgressTracker class)
• IBlockTreeVisitor now extends IDisposable with a no-op default

Previous findings — status

New findings

No Critical, High, or Medium issues.

Design notes (no action required)

• ProgressLogger concurrent writes in Era paths: EraExporter and EraImporter call progress.Update(Interlocked.Increment(ref totalProcessed)) from parallel epoch tasks. ProgressLogger.Update assigns plain auto-properties without synchronization — on 64-bit targets long assignment is atomic, so speed displays may be transiently stale but no corruption occurs. Acceptable for observability-only logging.
• SnapshotDownloader timer semantics: Old ProgressTracker used AutoReset = false (re-arm after each tick), new code uses a periodic 5s timer. LogProgress() deduplication suppresses output when the value hasn't changed, so stalled-download behavior is equivalent. The new approach is cleaner.
• ReceiptsVerificationVisitor.Dispose() is virtual: MissingReceiptsFixVisitor doesn't add disposable resources and doesn't override Dispose(), so the base implementation correctly disposes _progress. The virtual modifier leaves room for subclasses to extend if needed.

Summary

All previous findings are resolved. The abstraction is clean, the disposal lifetimes are correct throughout, and the IBlockTreeVisitor : IDisposable extension is backward-compatible via the no-op default. One formatting nit in IBlockTreeVisitor.cs is the only remaining item.

Mergeable ✓

Claude finished @LukaszRozmej's task in 3m 40s —— View job

Re-review: feat: add ProgressReporter abstraction

• Gather context and read repository guidelines
• Examine current diff vs master
• Read changed files
• Compare against previous review findings
• Post updated findings

Changes since round 3

Two commits were added:

6e0e175 — fix: address bot review L1+L2

• ProgressReporter.Dispose() now calls _progressLogger.LogProgress() after MarkEnd() ✓
• TotalDifficultyFixMigration restores the per-run summary with a fixedEntries counter ✓
  (Renamed from fixedBlocks → fixedEntries to reflect that the counter increments per BlockInfo, not per block height — correct.)

7f012de — refactor: migrate Era + Snapshot progress to ProgressReporter

• Era1 EraImporter / EraExporter and EraE equivalents drop the manual Reset + modulo Update/LogProgress pattern in favour of using ProgressReporter. Every block now calls Update with Interlocked.Increment(ref counter) for thread safety on the counter itself.
• SnapshotDownloader: replaces the bespoke ProgressTracker (custom ITimer + AddProgress) with ProgressReporter + SetFormat for byte-size formatting. ProgressTracker.cs deleted. ITimerFactory constructor parameter dropped.

Analysis

L1 (final LogProgress): Resolved. Dispose() calls both MarkEnd() then LogProgress(). The dedup check (CurrentValue, TargetValue, CurrentQueued, _skipped) means if the timer had already logged the terminal state it'll be a no-op — which is correct, not a regression.

L2 (fixedEntries count): Resolved. "Ended TotalDifficultyFixMigration. Fixed {fixedEntries} entries." is back.

Era importer/exporter parallel updates: Both Era1.EraImporter and EraE.EraImporter call progress.Update(Interlocked.Increment(ref blocksProcessed)) from concurrent tasks. ProgressLogger.Update writes CurrentValue as a plain auto-property (no volatile/Interlocked). Because ProgressReporter's constructor calls Reset before any Update is possible, the if (!UtcStartTime.HasValue) branch in Update is never entered concurrently, so the initialization TOCTOU isn't a problem. CurrentValue last-writer-wins under concurrent writes is acceptable for an observability-only counter; stale reads by the timer thread produce at most slightly-off speed figures in log output. Not a correctness concern.

SnapshotDownloader custom formatter: When totalSize is null, total = 0 is passed to ProgressReporter but SetFormat is called immediately after, overriding the default formatter entirely before the first 5-second tick. The 100%-bar-on-zero-total issue from previous review cycles does not apply here. The byte-range display (X out of Y / X) is semantically correct for resumable downloads.

ProgressTracker.cs deletion: Clean — InitDatabaseSnapshot.cs updated to new SnapshotDownloader(api.LogManager) without the removed ITimerFactory param.

Previous Finding Status

Summary

No Critical, High, or Medium findings. All previous Low findings addressed. The ProgressReporter abstraction is correct, the four application sites are clean, and the new Era + Snapshot migrations follow the same pattern correctly. Mergeable.

@claude check the validity of the following review

🟠 WARNING — two real issues, both rooted in the same gap

W1 — Dispose() doesn't drain in-flight timer callbacks; final "done" log can be overwritten by a stale tick

ProgressReporter.cs:
_timer.Stop();
_timer.Dispose();
_progressLogger.MarkEnd(); // sets UtcEndTime
_progressLogger.LogProgress(); // emits "done" line

System.Timers.Timer.Stop() is documented as not draining in-flight Elapsed callbacks — a tick already queued on the thread pool can land
after Stop() returns. That tick calls _progressLogger.LogProgress(), which:

• reads UtcEndTime (now set by MarkEnd()),
• resets LastMeasurement / LastValue via SetMeasuringPoint(),
• writes _lastReportState.

Two observable bad outcomes:

1. The final "100% done" line is followed by — and effectively replaced in operator-visible state by — a stale partial-progress line.
2. The speed counters in the "done" line get recomputed against post-reset state.

Fix: use Timer.Dispose(WaitHandle) and block on it:
using ManualResetEvent drained = new(false);
_timer.Dispose(drained);
drained.WaitOne();
_progressLogger.MarkEnd();
_progressLogger.LogProgress();
Or move to PeriodicTimer which is await-able and drains naturally.

W2 — ProgressLogger field state is unsynchronized; Update() and timer-fired LogProgress() race

ProgressLogger.Update(long) writes CurrentValue, UtcStartTime, UtcEndTime. LogProgress() reads those and _lastReportState. None of it is
locked. In ProgressReporter, the work thread calls Update() while the 1 Hz timer callback calls LogProgress() on a thread-pool thread —
they truly run concurrently.

Concrete torn-state path: Update() sets UtcEndTime = null first; a tick between that and the CurrentValue = value assignment computes the
speed line against the old CurrentValue with the new "still running" flag. _lastReportState is a 4-long tuple written non-atomically — a
half-written tuple either suppresses (dedup fires spuriously) or emits a duplicate line.

This is technically pre-existing in ProgressLogger, but the old ProgressTracker used Interlocked.Add + AutoReset=false which kept the timer
single-shot per tick. The new ProgressReporter introduces true concurrency where none existed before — at all 8 call sites.

Fix: a lock in ProgressReporter around both update and log paths is sufficient (uncontended at 1 Hz):
private readonly object _lock = new();
public void Update(long value) { lock (_lock) _progressLogger.Update(value); }
private void OnElapsed(object? _, ElapsedEventArgs __) { lock (_lock) _progressLogger.LogProgress(); }
Combined with W1's drain, the Dispose path also takes the lock before MarkEnd() + LogProgress().

🟡 SUGGESTION

S1 — Functional regression on unknown-size snapshot downloads
SnapshotDownloader.cs passes totalSize ?? 0 to the reporter. ProgressLogger.DefaultFormatter() does current / (float)Math.Max(total, 1), so
when Content-Length is absent (chunked transfer is common) the displayed percentage always pegs at 100%. The old ProgressTracker had an
explicit "unknown total" branch that showed bytes-without-percentage. Outside the race/leak/allocation scope strictly, but it's a real
operator-facing regression worth flagging since the diff touches this code path.

Claude finished @svlachakis's task in 3m 25s —— View job

Review validity check

• Read ProgressReporter.cs and ProgressLogger.cs
• Verify W1 — timer drain claim
• Verify W2 — unsynchronized field claim
• Verify S1 — SnapshotDownloader regression claim
• Post findings

W1 — Dispose() timer drain

Verdict: Valid in theory, materially overstated in practice.

System.Timers.Timer.Stop() is correctly documented as not draining in-flight thread-pool callbacks. A tick queued before Stop() returns can land after _timer.Dispose(). That part is accurate.

But the claimed "bad outcomes" don't hold up under the dedup logic in LogProgress():

Case A — stale tick fires before MarkEnd():
The tick emits one more regular progress line (identical to what the timer was already emitting). Then MarkEnd() + LogProgress() runs normally and emits the "done" line. Cosmetic extra line, not a regression.

Case B — stale tick fires between MarkEnd() and the final LogProgress():
MarkEnd() has set UtcEndTime, so CurrentPerSecond returns 0 in the tick's LogProgress() call (that branch is if (UtcEndTime is not null) return 0). The stale tick emits a "done" line (0 speed). When the final LogProgress() runs in Dispose(), reportState hasn't changed, so dedup fires — no second "done" line. Output: one "done" line from the stale tick, not two.

Case C — stale tick fires after the final LogProgress():
Dedup fires immediately (_lastReportState already matches). No log emitted.

The reviewer's claim that "the final '100% done' line is followed by a stale partial-progress line" requires the stale tick to fire after Dispose() completes with UtcEndTime still null — impossible, because MarkEnd() runs before the final LogProgress() in Dispose().

The Timer.Dispose(WaitHandle) fix is a valid defensive improvement, but the severity is Low (cosmetic extra progress line in the narrow race, not a corrupted "done" state).

W2 — Unsynchronized ProgressLogger fields

Verdict: Valid concern, but it's pre-existing and explicitly accepted.

ProgressLogger fields (CurrentValue, UtcStartTime, UtcEndTime, _lastReportState) are plain auto-properties with no volatile/Interlocked protection. The work thread calls Update() concurrently with the timer-thread LogProgress() in ProgressReporter — this is a real data race.

The old ProgressTracker used AutoReset = false (confirmed from the deleted source), which serialized timer callbacks: the callback restarted the timer itself, preventing overlapping ticks. ProgressReporter uses AutoReset = true (periodic), introducing a genuine concurrent-access pattern that ProgressTracker avoided.

However, the consequences are bounded:

• CurrentValue is a long — on 64-bit targets, 8-byte aligned reads/writes are atomic (no tearing of the value itself).
• _lastReportState is a ValueTuple<long,long,long,long> struct. A non-atomic write here could cause a spurious dedup or missed dedup: one extra or missing log line at most.
• UtcEndTime = null in Update() racing against CurrentPerSecond's UtcEndTime is not null check can produce a stale speed figure in a log line. Purely cosmetic.

This was explicitly reviewed and accepted in round 3: "speed displays may be transiently stale but no corruption occurs. Acceptable for observability-only logging." The lock fix is simple and would remove the concern entirely, but the risk is objectively low.

Severity: Low (pre-existing in ProgressLogger, explicitly accepted in prior review, observability-only impact).

S1 — SnapshotDownloader unknown-total regression

Verdict: Invalid. The code correctly handles the unknown-total case.

The reviewer claims the percentage bar pegs at 100% when Content-Length is absent, because totalSize ?? 0 is passed to ProgressReporter. This misses the SetFormat call immediately after:

// SnapshotDownloader.cs:73-74
using ProgressReporter progress = new("Snapshot download", logManager, totalSize ?? 0, ProgressInterval);
progress.Logger.SetFormat(FormatBytes(totalSize));   // ← overrides DefaultFormatter entirely

FormatBytes takes the original long? totalSize (not totalSize ?? 0) and returns two different lambdas:

private static Func<ProgressLogger, string> FormatBytes(long? totalBytes) =>
    totalBytes is null
        ? static logger => $"Snapshot download {HumanReadableSize(logger.CurrentValue)}"        // no percentage
        : logger => $"Snapshot download {HumanReadableSize(logger.CurrentValue)} out of {HumanReadableSize(totalBytes.Value)}";

When totalSize is null, the custom formatter shows bytes-only with no percentage — exactly matching the old ProgressTracker behavior. DefaultFormatter() (the one with the 100% issue) is never reached because _formatter is set.

Summary

None of these findings warrant blocking the PR. W1 and W2 are valid observations worth a follow-up improvement but carry no correctness or user-data risk — this is observability-only code. S1 is factually incorrect.

@claude re-review

Claude finished @LukaszRozmej's task in 2m 14s —— View job

Re-review: feat: add ProgressReporter abstraction

• Gather context and read repository guidelines
• Examine changes since last review (1 new commit: e841f46)
• Read changed files
• Analyze findings
• Post updated findings

Changes since Round 4

One commit added: e841f46 — fix: serialize ProgressReporter access; drain in-flight ticks on Dispose

Two files changed:

• ProgressReporter.cs — locking added to address W1 and W2 from svlachakis's external review
• IBlockTreeVisitor.cs — extra blank line removed (N1 nit from round 3)

Previous Finding Status

W1 + W2 Fix Analysis

The fix uses a single lock (_progressLogger) across three entry points:

public void Update(long value)
    => lock (_progressLogger) _progressLogger.Update(value);

private void OnElapsed(object? sender, ElapsedEventArgs e)
    => lock (_progressLogger) _progressLogger.LogProgress();

public void Dispose()
{
    if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
    _timer.Stop();
    _timer.Dispose();
    lock (_progressLogger)
    {
        _progressLogger.MarkEnd();
        _progressLogger.LogProgress();
    }
}

W2 (unsynchronized fields): Update and OnElapsed can no longer interleave, so ProgressLogger's plain auto-property fields (CurrentValue, UtcStartTime, UtcEndTime, _lastReportState) are protected from concurrent writes. ✓

W1 (timer drain): After _timer.Stop() + _timer.Dispose() no new callbacks are queued, but a callback already enqueued in the thread pool can still run. When it does, it blocks on lock (_progressLogger) until Dispose() releases it. At that point _lastReportState is already set to the terminal state, so LogProgress()'s dedup suppresses any output. ✓

Lock object choice: Locking on _progressLogger itself (exclusively owned by this instance) avoids an extra allocation and is idiomatic for owned, non-public resources. ✓

Double-dispose guard: Interlocked.Exchange(ref _disposed, 1) prevents two concurrent Dispose() calls from both emitting a "done" line. ✓

Contention profile: The timer fires at 1 Hz; Update() is called at block-processing cadence. The lock is held for a single field write (Update) or one log-format pass (LogProgress). Contention is negligible in practice. ✓

Logger property exposure: progress.Logger.SetFormat(...) is called at construction time (before the timer fires at any call site), so direct Logger access introduces no concurrent-access risk. ✓

New Findings

None. No Critical, High, or Medium findings.

Summary

All previous findings are resolved. The W1/W2 fix is correct: the lock serialises Update vs. timer ticks, and the lock inside Dispose() ensures any in-flight tick completes before the terminal "done" line is emitted. The IBlockTreeVisitor cosmetic nit is also fixed.

Mergeable ✓