Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @tac0turtle's task in 2m 9s —— View job

Code Review

• Gather context and read changed files
• Review syncer.go changes
• Review syncer_test.go changes
• Post comprehensive review feedback

Overall this is a clean, well-structured addition. The processP2PHeight extraction gives the P2P worker loop a natural place to emit diagnostic logs without cluttering the call site. A few observations below.

syncer.go

Non-deterministic select when context cancels at success boundary

In processP2PHeight (syncer.go:520), if ProcessHeight completes successfully at the exact same instant the context is canceled, Go's non-deterministic select may pick ctx.Done() and return ctx.Err() instead of nil. The caller treats context.Canceled as a soft error and continues, which means the height would be retried. The retry is harmless (the store already has the block), but it silently drops the "P2P height became available" log and costs a small busy-loop iteration. A simple mitigation is to drain doneCh first before yielding to ctx.Done():

case <-ctx.Done():
    // Drain doneCh in case ProcessHeight also finished.
    select {
    case err := <-doneCh:
        return err
    default:
    }
    return ctx.Err()

ctx.Done() case vs. goroutine exit

The goroutine (syncer.go:513) receives the same ctx, so when the context is canceled ProcessHeight will also exit and write to the buffered doneCh. The explicit ctx.Done() case is a useful speed-up (avoids waiting for ProcessHeight to notice cancellation), but see the point above for the interaction edge case. No goroutine leak exists—doneCh is buffered(1) and there is exactly one writer.

Package-level var for interval (syncer.go:44)

var p2pWaitLogInterval = 5 * time.Second

This is the standard Go pattern for test-overridable durations and is fine here. Worth a brief comment explaining it is exposed for tests, e.g.:

// p2pWaitLogInterval controls how often a slow-P2P warning is emitted; var so tests can override it.
var p2pWaitLogInterval = 5 * time.Second

storeHeightOf / storeHeightReader (syncer.go:1160-1170)

Clean nil-guard. One minor note: the interface is unexported and declared at the bottom of the file, far from its only use site (processP2PHeight). Consider moving it adjacent to processP2PHeight or to the top of the file with the other local interfaces so it is easier to discover.

Log field naming consistency

The warning log uses target_height but the success log uses height. If these logs are consumed by a log aggregator/query tool, consistent field names make it easier to correlate them. Suggest using target_height in both, or height in both.

syncer_test.go

stubP2PHandler

Clean and minimal stub—exactly right for this use case.

TestSyncer_processP2PHeight_LogsWhenBlocked

The test is well-structured and validates the most important log fields. A few smaller observations:

• Real timers in a package that already uses synctest: The test file imports testing/synctest (used by other tests). Consider whether synctest could be used here to make the timing assertions deterministic and faster, avoiding the 35 ms wall-clock sleep in CI. If ProcessHeight is wrapped in a fake clock it becomes immune to CI slowness.

• Missing assertion for in_flight_events/total_pending/waited: The test asserts pending_events but not in_flight_events, height_events_buffered, total_pending, or waited. The in_flight counter starts at 0 on a bare Syncer struct, so \"in_flight_events\":0 would be a cheap extra assertion. waited is a duration field and harder to pin down exactly, but its presence can be asserted:

  require.Contains(t, logs, `"waited":`)

• s.inFlight zero-value: The test constructs s as a struct literal, leaving inFlight as its zero value (0). The log call s.inFlight.Load() will work correctly (returns 0), but it's worth a brief comment in the test to signal this is intentional.

• Global var mutation is safe here: The test does not call t.Parallel(), so concurrent mutation of p2pWaitLogInterval is not a race. Worth keeping it non-parallel to preserve this safety.

Minor nit

syncer.go:537 — s.store.Height(ctx) is called inside the ticker case on every tick. Height() is presumably cheap (a store metadata read), but if it ever becomes slow it would block the ticker case and delay receiving from doneCh. For a logging path this is fine, just worth being aware of.

Codecov Report

❌ Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.59%. Comparing base (889da9a) to head (9825d5e).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3208      +/-   ##
==========================================
+ Coverage   61.46%   61.59%   +0.12%     
==========================================
  Files         120      120              
  Lines       12462    12502      +40     
==========================================
+ Hits         7660     7700      +40     
+ Misses       3944     3941       -3     
- Partials      858      861       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.