bor: fall back to eligible producers when veblop candidate set is empty

[pratikspatil024]

Summary

veBlop span producer selection (SelectNextSpanProducer) could hand SelectProducer an empty candidate slice when the elected producer set collapsed to only the current producer and that producer was filtered out of the active/supporting set. In the future-span path the resulting error propagates out of PreBlocker before the milestone/span commit, so block finalization cannot make progress — a deterministic halt with no self-heal.

This adds a deterministic, Ithaca-gated fallback that fires only on the future-span path. SelectNextSpanProducer / AddNewVeBlopSpan take an opt-in fallbackToActiveSet flag, and only checkAndAddFutureSpan (the fatal PreBlocker path) passes it. When that path's candidate list would otherwise be empty, the fallback draws a replacement from the caller's active set (the milestone supporters), then from the full validator set as defense-in-depth, each sorted ascending and excluding the current and excluded producers. In the future-span path this is always non-empty — a >2/3 milestone that excludes the current producer leaves a non-empty supporting set — so producer selection no longer errors there.

The non-fatal rotation / pending-stall / side-message paths do not opt in and are unchanged: post-Ithaca they still hit SelectProducer([]) → "no candidates found" → skip-and-retry, exactly as before. Scoping the fallback to the future-span path (rather than sharing it across all callers) keeps the rotation paths' self-heal contract intact and avoids reinstating a known-failed producer there.

The fallback helpers live in x/bor/keeper/veblop_producer_fallback.go (eligibleProducerFallback, sortedEligibleProducers) to keep veblop.go within size/complexity budgets.

Executed tests

• Unit (x/bor/keeper): eligibleProducerFallback branch coverage (active-set preference, validator-set fallback, excluded-set handling, validator-set read error → nil), sortedEligibleProducers, and SelectNextSpanProducer's empty-candidate path — pre-Ithaca still errors; post-Ithaca without opt-in still errors (rotation-path behavior preserved); post-Ithaca with opt-in falls back to a non-current producer.
• App-level regression (app): PreBlocker future-span creation when the elected set has collapsed to the current producer and it withholds its milestone vote — pre-Ithaca still errors (no candidates found), post-Ithaca commits the milestone + span with a milestone-supporting validator as the new producer.
• Full x/bor/... and app/... suites, go vet, gofmt clean.
• diffguard diff-mode incl. mutation testing: passing (Tier-1 logic mutation kill 100%, Tier-2 100%).
• Kurtosis devnet: confirmed the fallback fires on a running network — the empty-candidate selection error no longer occurs and spans are created with the fallback producer. Note: a long-running devnet in which every validator votes for a single producer is intentionally pathological (a permanent single-elected-producer state oscillates regardless of this change) and is not required for verification; the deterministic unit/app tests above cover the behavior.

Rollout notes

• Consensus-affecting: changes deterministic producer selection in the degenerate empty-candidate case, on the future-span path only. Gated behind the Ithaca hardfork height, which is unset (0) on mainnet, Amoy, and local today, so behavior is unchanged until the coordinated Ithaca activation — it ships with that upgrade alongside the related forced-span-rotation work.
• No state schema, store, migration, proto, or genesis changes. Backwards-compatible until the fork height.

[pratikspatil024]

@claude review once

[codecov]

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.0%. Comparing base (c32976f) to head (55658b8).

Files with missing lines	Patch %	Lines
x/bor/keeper/side_msg_server.go	0.0%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop    #618   +/-   ##
=======================================
  Coverage     50.0%   50.0%           
=======================================
  Files          180     181    +1     
  Lines        19854   19877   +23     
=======================================
+ Hits          9934    9957   +23     
  Misses        8646    8646           
  Partials      1274    1274           

Files with missing lines	Coverage Δ
app/abci.go	63.7% <100.0%> (ø)
app/pending_stall.go	98.4% <100.0%> (ø)
x/bor/keeper/veblop.go	71.1% <100.0%> (+0.1%)	⬆️
x/bor/keeper/veblop_producer_fallback.go	100.0% <100.0%> (ø)
x/bor/keeper/side_msg_server.go	60.1% <0.0%> (ø)

... and 2 files with indirect coverage changes

Files with missing lines	Coverage Δ
app/abci.go	63.7% <100.0%> (ø)
app/pending_stall.go	98.4% <100.0%> (ø)
x/bor/keeper/veblop.go	71.1% <100.0%> (+0.1%)	⬆️
x/bor/keeper/veblop_producer_fallback.go	100.0% <100.0%> (ø)
x/bor/keeper/side_msg_server.go	60.1% <0.0%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR prevents veBlop span producer selection from failing (and halting PreBlocker progress) when the elected producer candidate set becomes empty in a degenerate “singleton incumbent” scenario. It introduces an Ithaca-gated, deterministic fallback to ensure SelectProducer is not called with an empty candidate list in the future-span (milestone) path.

Changes:

• Add an Ithaca-gated fallback in SelectNextSpanProducer when activeCandidates is empty.
• Introduce eligibleProducerFallback/sortedEligibleProducers to deterministically derive replacement candidates (prefer active set, then validator set; exclude current + excluded).
• Add unit and app-level regression tests covering the empty-candidate condition and gate boundary behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
x/bor/keeper/veblop.go	Adds Ithaca-gated fallback invocation when the active candidate list is empty.
x/bor/keeper/veblop_producer_fallback.go	Implements deterministic fallback candidate derivation and sorting.
x/bor/keeper/veblop_producer_fallback_test.go	Unit tests for fallback behavior and the post-Ithaca empty-candidate path.
app/veblop_producer_fallback_test.go	App-level regression test ensuring PreBlocker progresses post-Ithaca in the degenerate empty-candidate scenario.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[pratikspatil024]

@claude review once

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[claude]

Prior rotation-path concern is correctly addressed by the opt-in flag — only opts into the fallback, so the other three callers (rotation, pending-stall, side-message) keep their skip-and-retry contract and the previously-described thrash can't occur. Deferring final approval since this is consensus-affecting producer selection on the PreBlocker future-span path.

Extended reasoning...

Overview

This PR adds a deterministic, Ithaca-gated fallback in SelectNextSpanProducer that fires only on the future-span path in PreBlocker. SelectNextSpanProducer / AddNewVeBlopSpan now take an opt-in variadic fallbackToActiveSet flag; only checkAndAddFutureSpan (app/abci.go:964) passes true. The new helpers (eligibleProducerFallback, sortedEligibleProducers) live in a separate file under x/bor/keeper. Tests cover both the keeper-level branch coverage and an app-level PreBlocker regression staging the singleton-collapse scenario, pre- and post-Ithaca.

Re-review vs my prior comment

I verified the four call sites of AddNewVeBlopSpan: app/abci.go:964 opts in (true); app/abci.go:1072 (checkAndRotateCurrentSpan), app/pending_stall.go:205, and x/bor/keeper/side_msg_server.go:703 do not. Since the Ithaca gate inside SelectNextSpanProducer also requires fallbackToActiveSet[0] == true, eligibleProducerFallback is unreachable from the rotation / pending-stall / side-message paths — they still go SelectProducer([]) → "no candidates found" → AddNewVeBlopSpan errors → skip-and-retry, identical to pre-Ithaca behavior. The thrash-through-failed-validators scenario I described in my earlier review is therefore not reachable.

Security risks

No new authentication, cryptography, signature, or permission surface is touched. The change is purely about which producer the chain installs in a degenerate empty-candidate case on the future-span path. The fallback draws from the milestone supporters first and the full validator set second, with deterministic ascending sort — no nondeterminism is introduced.

Level of scrutiny

High. This is consensus-affecting code in PreBlocker (deterministic producer selection in a degenerate case), Ithaca-gated. Ithaca height is currently unset (0) on mainnet, Amoy, and local, so behavior is unchanged until the coordinated activation, which limits blast radius today. But the diff ships with the gate, so a human reviewer with consensus-path context should sign off before the Ithaca upgrade is rolled.

Other factors

Tests are thorough (pre-Ithaca still errors, post-Ithaca without opt-in still errors, post-Ithaca with opt-in falls back to a non-current producer; full app-level PreBlocker regression). Copilot's flaky-CI concern about global fork-height mutation was addressed convincingly: every fork-height-mutating test in app/ is sequential by convention, and go test -race was confirmed clean. Codecov shows new lines fully covered. Recommend a human pass focused on the Ithaca gate semantics and the docstring invariant before activation.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[adamdossa]

@claude review once

[claude]

My earlier concern about the rotation path reinstating failed producers was properly addressed by scoping the fallback to the future-span opt-in path — the reinstatement scenario is no longer reachable. No new issues found in this review, but the PR modifies consensus-critical producer selection in PreBlocker and should get a final human look before Ithaca activation.

Extended reasoning...

Overview

This PR adds a deterministic, Ithaca-gated empty-candidate fallback in SelectNextSpanProducer so the future-span PreBlocker path can't deterministically halt when the elected candidate set collapses to the current producer. The recovery is opt-in: only checkAndAddFutureSpan (app/abci.go:964) passes fallbackToActiveSet=true; the rotation, pending-stall, and side-message callers pass false and keep their existing skip-and-retry semantics.

Verification of the fix from my prior review

My earlier concern (rotation path reinstalling known-failed producers via the validator-set branch of eligibleProducerFallback) was addressed by removing the rotation path from the fallback entirely, rather than threading {currentProducer} ∪ latestFailedProducer through as excludedProducerIDs. I re-traced the four call sites:

• checkAndAddFutureSpan — true (fatal path, non-empty supporting set guaranteed by >2/3 milestone)
• checkAndRotateCurrentSpan — false (unchanged skip-and-retry)
• rotateSpanFromPendingHead — false (unchanged skip-and-retry, already carries its own exclusion set)
• PostHandleSetProducerDowntime — false (unchanged skip-and-retry)

The rotation caller can no longer reach eligibleProducerFallback, so the reinstatement dance I walked through cannot occur.

Consensus-critical checks

Determinism: sortedEligibleProducers uses slices.Sort on uint64, no map iteration into state, no time.Now, no external RPC, no goroutines. The checkAndAddFutureSpan inputs (supportingValidatorIDs, excludedProducerIDs=nil) come from GetMajorityMilestoneProposition, itself a deterministic tally over VE data. Ithaca gate is applied via helper.IsIthaca(ctx.BlockHeight()) at the single new branch in SelectNextSpanProducer.

Hardfork rollout: no state schema, store, migration, proto, or genesis changes. Ithaca is 0/unset on mainnet, Amoy, and local today, matching the PR's claim that behavior is unchanged until the coordinated activation. ABCI symmetry is preserved — no PrepareProposal/ProcessProposal divergence introduced, and the deterministic PreBlocker path is what changes.

Level of scrutiny

Deferring rather than approving because this modifies deterministic producer selection reachable from PreBlocker and ships with the Ithaca hardfork; even though the gate is off everywhere today, the surface warrants a final human sign-off before activation. The change itself looks well-scoped and the test coverage (unit + app-level replay of the degenerate scenario at both sides of the gate) is appropriate.

Other factors

Copilot's inline about global fork-height mutation and cffls' feedback on function size have both been discussed and resolved in the thread. No unresolved reviewer comments remain.