fix(light): stop double-verifying every failing bisection step

[gomesalexandre]

What it says on the box

verifySkipping's bisection loop appended every fetched pivot block twice, so each failing bisection step got re-verified with an identical, wasted second call. Performance/efficiency bug, not a correctness or security issue — explained below.

The bug

light/client.go, inside verifySkipping's ErrNewValSetCantBeTrusted case:

switch providerErr {
case nil:
    blockCache = append(blockCache, interimBlock)          // append #1
case provider.ErrLightBlockNotFound, provider.ErrNoResponse, provider.ErrHeightTooHigh:
    return nil, err
default:
    return nil, ErrVerificationFailed{From: verifiedBlock.Height, To: pivotHeight, Reason: providerErr}
}
blockCache = append(blockCache, interimBlock)               // append #2 — stray duplicate

Every other arm of the switch returns early, so the second append only ever runs alongside the first — it's pure duplication with no distinct purpose. Working through the loop's arithmetic: after both appends, depth++ lands on the array index holding the first copy of interimBlock, not a fresh entry, so the very next iteration re-verifies the identical block against the identical trusted block with the identical now — guaranteed to hit the same code path again — before the loop's depth == len(blockCache)-1 condition finally becomes true again and a genuinely new pivot gets fetched.

Evidence

Built a capturing log.Logger recording every "Verify non-adjacent" step's height, driven through a real bisection with a non-zero valVariation (so the validator set genuinely changes and VerifyNonAdjacent actually fails and retries — more on why this matters below):

Before fix: 33 verification rounds, 8 consecutive repeats
  [30 17 17 10 10 6 6 3 30 17 17 10 10 6 30 17 17 10 30 17 17 13 30 17 30 24 24 20 30 24 30 27 30]
                ^^    ^^    ^^                ^^    ^^          ^^    ^^          ^^    ^^

After fix:  25 verification rounds, 0 repeats
  [30 17 10 6 3 30 17 10 6 30 17 10 30 17 13 30 17 30 24 20 30 24 30 27 30]

Same final verified block, height and hash, before and after. This is a real A/B measurement against the actual unfixed/fixed source, not a theoretical count.

Why this is not a verification bypass

I checked this explicitly rather than assuming it from the shape of the fix. Verify is pure over its inputs:

• blockCache[d] is not mutated between the two (duplicate) visits — it's the same pointer.
• verifiedBlock is only reassigned on the case nil: success path, never on the duplicate no-op.
• trustingPeriod, now, maxClockDrift, and trustLevel are all fixed for the whole verifySkipping call — none of them change between the duplicate calls.

Given all of that, the duplicate call is provably a no-op returning the identical result to the first — it can't let anything through that the single call wouldn't have, and removing it can't skip anything the loop still needs. The A/B evidence above confirms this directly: same final block, fewer rounds, zero repeats.

Why no existing test caught this

Two separate blind spots, both fixed here:

1. TestClient_SkippingVerification's one case that reaches the failing-pivot arm succeeds at depth == 1, and the success branch immediately does blockCache = blockCache[:depth], truncating the duplicate entry before it would ever be revisited.
2. TestClientLargeBisectionVerification — the repo's only large-scale bisection test — uses genMockNode(chainID, 100, 3, 0, bTime), i.e. valVariation = 0. With a validator set that never changes, VerifyNonAdjacent succeeds immediately at depth == 0 and the bisection loop never iterates through a failing pivot at all. This test structurally cannot exercise the buggy branch.

The fix

Delete the stray duplicate append. One line.

New test

TestClientLargeBisectionVerificationWithValSetChanges (light/client_test.go) uses a non-zero valVariation specifically to force genuine multi-step bisection through ErrNewValSetCantBeTrusted, and asserts on the number of verification rounds (zero consecutive repeats), not just the end result — so this can't silently regress again. Backed by a small verifyStepLogger helper in light/helpers_test.go.

Confirmed genuine red-before/green-after: stashed just the client.go fix and reran this exact new test — it fails on the unfixed source with the predicted duplicate pattern, passes after restoring the fix.

Testing

$ go test ./light/... -v
ok  github.com/cometbft/cometbft/light          71.5s
ok  github.com/cometbft/cometbft/light/provider/http  20.1s
ok  github.com/cometbft/cometbft/light/store/db       1.0s

Full light/ suite green, zero regressions. gofmt -l and go vet ./light/... clean.

Codex adversarial review

Ran synchronously, verdict SHIP. It independently mutation-tested the new regression test — 20/20 passes against the fixed source, fails with the exact 8-repeat pattern when the duplicate append is restored — confirming the test is non-vacuous rather than just trusting the author's own claim. It also suggested one hardening (guard against a future rename of the debug log message the test's collector keys on silently making the assertion vacuous), which is included as a separate small commit.

Scope note

Present identically on main and several release branches back to v0.34.x — this PR targets main only per standard convention; noting the multi-version presence here in case maintainers want to consider a backport.

[greptile-apps]

PR author is not in the allowed authors list.