@claude review

Claude finished @marcello33's task in 1m 50s —— View job

Code Review: Deterministic State Syncs

• Gather context and read changed files
• Review consensus logic changes (bor.go)
• Review Heimdall client changes (HTTP, gRPC, app)
• Review config and fork ID changes
• Review tests
• Post final review

Overall this is a well-structured PR. The fork-gating approach is correct, the error handling post-fork is properly strict (returning errors instead of silently swallowing), and the test coverage for the fork switch is good. Below are findings organized by severity.

HIGH: HTTP client does client-side filtering that gRPC/app clients do not

In consensus/bor/heimdall/client.go:329-343, the HTTP StateSyncEventsAtHeight applies a client-side filter:

if e.Id >= fromID && e.RecordTime.Before(time.Unix(toTime, 0)) {

But the gRPC implementation (heimdallgrpc/state_sync.go:125-137) and the in-process app implementation (heimdallapp/state_sync.go) trust the server response and append all returned events without filtering.

This means the three transport implementations have different behavior if the Heimdall endpoint returns events outside the expected range. For a deterministic state sync feature, all paths must produce identical results. Either:

1. All clients should apply the same client-side filter (defensive), or
2. All clients should trust the server (if the server guarantees correct filtering)

The existing StateSyncEvents has the same asymmetry (HTTP filters, gRPC doesn't), but for a feature explicitly targeting determinism, this inconsistency is worth resolving.

Fix this →

MEDIUM: queryCtx variable is unnecessary

In consensus/bor/bor.go:1763, queryCtx is assigned c.ctx and then used everywhere instead of c.ctx directly. This adds no value — it's not a derived context (no timeout, no cancellation). Consider removing the alias to reduce noise, or if the intent is to add a timeout later, add a TODO.

MEDIUM: log.Info in hot pagination loop

consensus/bor/heimdall/client.go:319:

log.Info("Fetching state sync events at height", "queryParams", u.RawQuery)

This logs at Info level inside the pagination loop, so it fires on every page. For large state sync batches this will be noisy. Consider log.Debug for per-page logs, keeping Info for the initial call only (similar to how the gRPC path logs once before the loop).

MEDIUM: TODO comments left in consensus-critical code

There are two identical TODO comments in bor.go:1769-1771 and bor.go:1779-1781:

// TODO marcello double check: if we silently return "no state syncs" here,
//  different validators could end up deriving different state sync sets...

The code already does the right thing (returns an error instead of silently swallowing). The TODOs are now misleading since they describe a concern that the code already addresses. These should be removed or replaced with a brief comment confirming the intentional design choice (e.g., // Post-fork: fail hard to preserve determinism).

LOW: Typo in test comment

consensus/bor/bor_test.go:5369:

// deterministi state sync

Should be deterministic state sync. (Also flagged by Copilot.)

Fix this →

LOW: to_time query parameter is not URL-encoded

In visibleAtHeightURL (client.go:537-540), the RFC3339Nano-formatted time is inserted into the query string via fmt.Sprintf without URL-encoding. RFC3339 timestamps contain + characters for positive timezone offsets (though UTC uses Z). Since the code explicitly uses .UTC(), this is safe today, but it's fragile — if the .UTC() call were ever removed, + would be interpreted as a space by the server. Consider using url.QueryEscape or url.Values for robustness.

LOW: No retry for GetBlockHeightByTime in HTTP client

StateSyncEventsAtHeight uses Fetch (no retry) while GetBlockHeightByTime uses FetchWithRetry. This is a reasonable design choice since retrying the height lookup is valuable (it's a single call), but it means a transient failure on the events endpoint will immediately fail the block. Given this is consensus-critical and a transient 503 from Heimdall would halt block production, consider whether FetchWithRetry would be more appropriate for the events fetch too, matching the pattern used in the existing StateSyncEvents (which also uses FetchWithRetry — I see it's actually calling Fetch directly here, not FetchWithRetry).

Actually, looking more carefully: the existing StateSyncEvents also uses bare Fetch, not FetchWithRetry, so this is consistent. Disregard.

Observation: go.mod replace directive

The replace directive for heimdall-v2 pins a pseudo-version. As Copilot noted, if this is temporary (waiting for a tagged release), it would be good to track when this can be cleaned up. This is fine for development but should not ship to mainnet without a proper version pin.

Summary

The core design is sound — anchoring state sync queries to a resolved Heimdall height is the right approach for determinism. The error handling post-fork correctly fails loudly rather than silently returning empty results. The main actionable finding is the client-side filtering inconsistency across HTTP/gRPC/app transports, which should be resolved to ensure all paths produce identical event sets.

test

Codecov Report

❌ Patch coverage is 24.71910% with 134 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.91%. Comparing base (438596d) to head (5c6214f).
⚠️ Report is 20 commits behind head on develop.

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

@@             Coverage Diff             @@
##           develop    #2177      +/-   ##
===========================================
- Coverage    51.93%   51.91%   -0.02%     
===========================================
  Files          884      884              
  Lines       155446   155616     +170     
===========================================
+ Hits         80731    80795      +64     
- Misses       69509    69620     +111     
+ Partials      5206     5201       -5     

@claude addressed your points.
Here's a recap.

• HTTP client-side filtering inconsistency: since the server already filters correctly, removed the client-side filter for consistency across all three transports.
• Unnecessary queryCtx alias: removed, using c.ctx directly.
• log.Info in pagination loop: changed to log.Debug to avoid noise.
• Misleading TODO comments: removed.
• to_time not URL-encoded in visibleAtHeightURL: switched from fmt.Sprintf to url.Values for proper encoding.
• Typo in test comment: fixed.

Re-review.

Claude finished @marcello33's task in 1m 14s —— View job

Re-review: Deterministic State Syncs

• Verify previous review fixes
• Review full diff for new issues
• Post final review

All six items from the previous review have been addressed correctly:

1. Client-side filtering removed from HTTP — client.go:328-340 now appends all server-returned events without filtering, matching gRPC and app paths. ✅
2. queryCtx alias removed — bor.go:1764 uses c.ctx directly. ✅
3. log.Info → log.Debug in pagination loop — client.go:318. ✅
4. TODO comments removed — replaced with clear // Post-fork: fail hard to preserve determinism comments at bor.go:1766,1774. ✅
5. url.Values for URL encoding — visibleAtHeightURL (client.go:536-541) now uses url.Values properly. ✅
6. Typo fixed — bor_test.go:5370. ✅

Remaining finding: Sort inconsistency across transports

The HTTP client sorts events after fetching (client.go:350-352):

sort.SliceStable(eventRecords, func(i, j int) bool {
    return eventRecords[i].ID < eventRecords[j].ID
})

But neither the gRPC client (heimdallgrpc/state_sync.go:90-148) nor the app client (heimdallapp/state_sync.go:42-69) sort the results. If the server returns events in a different order across transports — or if pagination produces different orderings — this breaks the determinism guarantee that is the entire point of this PR.

Either all three should sort, or none should (if the server guarantees ordering). Given this feature targets determinism, I'd recommend all three apply the same sort for defense-in-depth.

Fix this →

Everything else looks good. The core design is sound and the error handling is correct.

View job

@claude addressed your points. Review once more and resolve the related open conversations if the comments are addressed.

Code Review

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

Bug: Missing global timeout on StateSyncEventsAtHeight HTTP client pagination loop

File: consensus/bor/heimdall/client.go L310-313

StateSyncEventsAtHeight has no context.WithTimeout bounding its pagination loop. Every sibling method added in this PR wraps its loop with a 1-minute timeout (HTTP StateSyncEventsByTime at L365-368, gRPC StateSyncEventsAtHeight at state_sync.go L95-97, gRPC StateSyncEventsByTime at state_sync.go L161-163). This method is the only one missing it. If the Heimdall server consistently returns full pages, the loop runs indefinitely. The comment on L364-365 of StateSyncEventsByTime states the intent to match the gRPC implementation, suggesting this was an oversight.

Suggested fix: Add a global timeout at the top of StateSyncEventsAtHeight: ctx, cancel := context.WithTimeout(ctx, 1*time.Minute) / defer cancel()

Flagged per consensus-security.md (timeout handling for Heimdall RPC calls) and security-common.md (context propagation for operations that can block).

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: Per-attempt timeout (30s) silently caps the intended 1-minute pagination budget

File: consensus/bor/heimdall/failover_client.go:113-122

MultiHeimdallClient.StateSyncEventsByTime sets a 1-minute global timeout (line 116), intending it to bound the entire paginated fetch. However, callWithFailover (line 183) applies attemptTimeout (default 30s, line 20) as a child context on each attempt. Since context.WithTimeout uses min(parent_deadline, new_timeout), the entire paginated loop within each attempt is capped at 30s, not 1 minute.

Additionally, HeimdallClient.StateSyncEventsByTime (client.go:278) sets its own redundant 1-minute timeout inside the already-tighter 30s context - this inner timeout can never fire.

Effect: With default settings, the paginated fetch for all pages must complete within 30s per failover attempt, not the intended 1 minute. During periods with many queued state sync events requiring multiple pages, this could cause premature timeouts, triggering unnecessary failover cascading and ultimately returning empty state syncs (the proposer misses a slot).

Note: The test TestMultiFailover_StateSyncEventsByTime_CapsAttemptTimeoutWithGlobalPaginationDeadline only validates the case where attemptTimeout (set to 2 min) > global timeout (1 min). It does not test the default case where attemptTimeout (30s) < global timeout (1 min), which is the actual production behavior.

Suggested fix: Remove the 1-minute timeout from HeimdallClient.StateSyncEventsByTime (redundant), and either increase attemptTimeout for this method or pass the 1-minute context directly to the fn closure bypassing the per-attempt cap.

Quality Gate failed

Failed conditions
19.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.