Code review

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

Code review

1 issue found. Checked for bugs and CLAUDE.md compliance.

Bug: Off-by-one in txIndex diagnostic log

File: miner/worker.go:1254

When the interrupt fires after a successful commitTransaction, env.tcount has already been incremented (at line 1161), so it points to the next unprocessed slot, not the index of the transaction identified by txHash (lastTxHash, set at line 1304).

Example: If lastTxHash refers to the tx at index 4, env.tcount is already 5 by the time this log fires. An operator seeing txHash=0xabc txIndex=5 would think tx 0xabc was at index 5, when it was actually at index 4.

Suggested fix: Change env.tcount to env.tcount-1 on line 1254.

@claude Do a review again, see if you find any other issues

Claude finished @lucca30's task in 2m 7s —— View job

Code Review (Re-review)

• Gather context and read all changed files
• Review miner/worker.go changes
• Review common/format.go and tests
• Review config/flag changes
• Review benchmark test
• Post findings

1 issue found, 2 minor observations. Checked for bugs, races, security, and CLAUDE.md compliance.

Issue: Weak ordering guarantee between interruptFlagSetAt and interruptBlockBuilding atomics

File: miner/worker.go:2145-2148

In createInterruptTimer, the goroutine writes:

interruptFlagSetAt.Store(time.Now().UnixNano())  // line 2146
interruptBlockBuilding.Store(true)                // line 2148

The main loop reads in the opposite order:

if w.interruptBlockBuilding.Load() {              // line 1241
    if flagSetAt := w.interruptFlagSetAt.Load()   // line 1246

Go's memory model does not guarantee ordering across different atomic variables. The main goroutine could observe interruptBlockBuilding=true while still reading a stale interruptFlagSetAt=0. The flagSetAt > 0 guard means this just silently omits the timing fields from the log rather than producing incorrect data, so the impact is low — it's a diagnostic gap, not a correctness bug.

If you want to eliminate this, you could combine both values behind a single sync.Mutex, or accept the rare missing timestamp as acceptable for a diagnostic log (which I think is fine).

Severity: Low (self-inflicted, diagnostic-only impact)

Observation 1: Previous off-by-one is fixed

The previous review flagged env.tcount being used directly in the interrupt log. The new code correctly captures lastTxIndex = env.tcount before commitTransaction (which increments tcount on success), so the logged index now matches the tx identified by lastTxHash. Nice fix.

Observation 2: ErrInterrupt handler uses txs.Pop() (drops all sender txs)

File: miner/worker.go:1449

When a tx is interrupted due to timeout, txs.Pop() drops all remaining txs from the same sender. This is more aggressive than txs.Shift() (which would try the next tx). Given that the block-building timer has already fired at this point, dropping the sender's remaining txs is the right call — there's no time left to try more txs anyway, and the loop will exit on the next iteration when it re-checks interruptBlockBuilding. No issue here, just noting for completeness.

Summary

The implementation is clean and well-structured. Config plumbing follows existing patterns (DurationFlag + raw string for TOML). The benchmark test correctly handles the global metrics.Enable() one-way switch. The slow-tx aggregation (one log per block, sorted by duration) is a good design choice that avoids log spam. The ErrInterrupt case handler is a sensible addition for timeout diagnostics.

The only finding is the minor atomic ordering gap noted above, which has no impact on correctness — just a rare chance of omitting timing fields from a diagnostic log.

IMO, the per tx latency isn't much useful as it would just be a another metric. It doesn't particularly tell use which txs are slower vs which are faster. I don't see how this can help us.

IMO, the per tx latency isn't much useful as it would just be a another metric. It doesn't particularly tell use which txs are slower vs which are faster. I don't see how this can help us.

@kamuikatsurgi
I agree that this data has no good use for the things we have today.
But the goal is to help us refine which should be the ideal --miner.slowtxthreshold value to just capture relevant outliers. Today we have no idea on what is the avg time a tx take, how outliers behave, if they are common or not etc

Codecov Report

❌ Patch coverage is 69.23077% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.48%. Comparing base (a328aa9) to head (91d3170).
⚠️ Report is 4 commits behind head on develop.

❌ Your patch status has failed because the patch coverage (69.23%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

@@             Coverage Diff             @@
##           develop    #2134      +/-   ##
===========================================
+ Coverage    51.44%   51.48%   +0.04%     
===========================================
  Files          881      882       +1     
  Lines       153870   154004     +134     
===========================================
+ Hits         79157    79290     +133     
+ Misses       69542    69538       -4     
- Partials      5171     5176       +5     

@claude Do an analysis for the new changes I did. Checking if any bug, error or sec breach was introduced.

Claude finished @lucca30's task in 1m 13s —— View job

Analysis of New Changes

• Gather context — read the full diff and changed files
• Review miner/worker.go changes for bugs, errors, and security issues
• Review common/format.go and common/format_test.go
• Review test files (miner/slow_tx_tracker_test.go, miner/worker_metrics_benchmark_test.go)
• Review config/flag changes
• Post findings

0 bugs, 0 security issues found. Checked for bugs, data races, security breaches, and correctness issues across all 6 changed files.

What was checked

1. slowTxTopTracker data structure (worker.go:184-273)

• Min-heap logic is correct: O(1) fast reject, O(log K) replace, bounded at K=10.
• SnapshotAndReset correctly copies before resetting, returns descending order.
• No concurrency issue — the tracker is only accessed from mainLoop (single goroutine) and flushSlowTxWindow (also called from mainLoop via the ticker case). Safe without a mutex.

2. Tracker integration in commitTransactions (worker.go:1495-1505)

• w.slowTxTracker.Add(...) is only called on successful tx commit (errors.Is(err, nil) case). Correct — only tracks actually applied txs.
• Guarded by w.IsRunning() so non-producers don't track. Consistent with the flush logic.

3. Window ticker in mainLoop (worker.go:881, 975-981)

• Ticker created before the select loop and properly deferred for cleanup.
• When not running, calls Reset() to avoid stale carryover. Good.

4. ErrInterrupt handler (worker.go:1537-1546)

• The ltx.Hash reference on line 1545 is valid here — ltx was set earlier in the loop iteration (line 1391-1398) and hasn't been reassigned. This is the same pattern used by the default case on line 1551.
• txs.Pop() correctly drops the sender's remaining txs since the block-building timer has already fired.

5. Interrupt diagnostic context (worker.go:1313-1323, 1339-1365)

• lastTxIndex = env.tcount is captured before commitTransaction (line 1479), fixing the off-by-one from the previous review. Correct.
• lastCommitStart zero-check guards the log fields properly — if no tx was attempted, the tx-specific fields are omitted.

6. createInterruptTimer atomic ordering (worker.go:2229-2232)

• interruptFlagSetAt.Store(...) is now written before interruptBlockBuilding.Store(true), and the reader checks interruptBlockBuilding first then reads interruptFlagSetAt. This is the correct store-before-publish / load-after-observe pattern. The interruptCtx.Err() != context.Canceled guard ensures the timestamp is only set on actual timeout (not on cancel). Clean.
• On the reader side (worker.go:1344), flagSetAt > 0 still guards against the race where the goroutine hasn't written the timestamp yet (e.g., if the scheduler delays between the two stores). The fallback is just omitting the timing fields — no incorrect data.

7. PrettyTime (common/format.go:45-52)

• Simple UTC formatter. Test covers timezone conversion correctly.

8. Removed flag test (internal/cli/server/command_test.go:150-168)

• Correctly verifies --miner.slowtxthreshold is rejected. Uses io.Discard for UI writers to avoid test noise.

9. Benchmark test (miner/worker_metrics_benchmark_test.go)

• Sink variables prevent dead-code elimination. Benchmark modes cover all tracker code paths. No issues.

10. Security check

• No new RPC endpoints, no new peer message handlers, no new untrusted input paths.
• The slow tx tracker is bounded (K=10, fixed) — no unbounded allocation risk.
• formatSlowTxs uses fmt.Sprintf on bounded data (max 10 entries) — no memory concern.
• No secrets logged; tx hashes and sender addresses are already public data.

Previous review findings status

• Off-by-one in txIndex: Fixed in this version. lastTxIndex = env.tcount is captured before commitTransaction.
• Weak atomic ordering: Addressed — store order is now interruptFlagSetAt before interruptBlockBuilding, which is the correct pattern.

The implementation is solid. No bugs, errors, or security issues introduced.

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud