fix(light-client): verify boundary 2-chain QCs against their own epoch

[bfish713]

Problem

A 2-chain proving the last leaf of epoch E always includes a deciding QC produced in epoch E+1, signed by that epoch's quorum. verify_qc_chain_and_get_version verified every certificate against the single quorum of the leaf's epoch, so fetching the last leaf of an epoch failed with invalid threshold signature: Pairing check failed whenever adjacent epochs have different stake tables.

First observed on milk at height 6000 (epoch_height = 3000): the boundary between the genesis stake table (epochs 1–2, from known_nodes_with_stake, all stakes 0x1) and the first contract-sourced stake table (epoch 3, real stakes, different ordering). Same members, same size, different order — so the signer bitvec aggregated the wrong keys and the pairing check failed. A freshly-syncing node can never backfill past this height. The bug is invisible at every other epoch boundary because adjacent contract-sourced tables are usually identical.

Same bug family as #4594 (wrong quorum for pre-PoS QCs) and #4669 (cert2 verified against wrong epoch).

Fix

Dispatch each certificate on the epoch committed in its signed payload: certs from the leaf's epoch verify against the proof's quorum as before; certs from the following epoch verify against the next epoch's stake table (which StakeTableQuorum already carries for dual-signed transition certs); any other epoch is rejected. The epoch number is part of the signed vote data, so a certificate claiming the wrong epoch fails signature verification — the dispatch is binding.

Pre-epoch certificates (epoch None) keep the existing behavior, leaving the #4594 HotStuff1 path untouched.

Review focus: the epoch-dispatch match in verify_qc_chain_and_get_version, and the next_epoch_qc().is_none() restriction in StakeTableQuorum::verify_next_epoch_static.

Tests

Regression tests build the exact boundary shape with real BLS signatures over disjoint per-epoch quorums (leaf 10 = last leaf of epoch 1, dual-signed transition cert, deciding QC from epoch 2). Both fail without the fix:

• test_epoch_boundary_quorum_change: a valid boundary proof must verify (the old code rejected it — the milk symptom)
• test_epoch_boundary_deciding_qc_wrong_quorum: a deciding QC claiming the next epoch but signed by the current epoch's quorum must fail (the old code accepted it)

🤖 Generated with Claude Code

[claude]

Claude finished @bfish713's task in 2m 34s —— View job

---

Code Review: PR #4713 — verify boundary 2-chain QCs against their own epoch

• Read changed files (leaf.rs, quorum.rs)
• Analyze epoch-dispatch logic in verify_qc_chain_and_get_version
• Review verify_next_epoch_static restriction
• Review test coverage
• Post review feedback

---

Verdict: LGTM. This is a correct, well-scoped, and well-tested fix. The root-cause analysis in the description (identical-membership but reordered stake tables aggregating the wrong keys in the signer bitvec) is convincing and matches the code.

Correctness — the fix is sound

The core question is whether dispatching on cert.epoch() is binding, i.e. can a malicious cert get routed to a quorum it isn't actually signed by? I verified it cannot:

• CertificatePair::epoch() (simple_certificate.rs:999) returns self.qc.data.epoch.
• That epoch field is part of the VersionedVoteData commitment the quorum signs over (see boundary_signed_qc in the tests).
• So a cert claiming epoch = leaf_epoch + 1 to route itself to the next quorum will fail the pairing check unless it was genuinely signed by that quorum. test_epoch_boundary_deciding_qc_wrong_quorum proves exactly this. ✅

The match in verify_qc_chain_and_get_version handles the epoch space cleanly:

• (None, _) | (_, None) → current quorum, preserving the pre-PoS / Fix a bug in light client fetching leaves from HotStuff1 #4594 path untouched. ✅
• first cert always satisfies cert_epoch == leaf_epoch (it is leaf_epoch), so the committing QC — including its dual-signed next-epoch QC via verify_next_epoch_qc — is verified against the proof's quorum + next-epoch table as before. ✅
• leaf_epoch + 1 → verify_next_epoch, and the catch-all bail! correctly rejects both earlier epochs and leaf_epoch + 2. ✅

verify_next_epoch_static restriction — agree

The ensure!(cert.next_epoch_qc().is_none()) is justified: a cert from leaf_epoch + 1 sits at the start of the next epoch, never at its transition boundary, so it carries no legitimate next-epoch QC — and checking one would require the stake table two epochs ahead of the proof's, which isn't available. Rejecting is both correct and defensive.

Tests — high quality, two minor gaps

Strong regression tests: real BLS signatures over genuinely disjoint per-epoch quorums, reproducing the exact milk boundary shape, with both positive and negative (wrong-quorum) cases that each fail without the fix. 👍

Two non-blocking coverage suggestions (posted inline, both in the PR's stated review-focus areas):

1. The explicit bail! arm (non-adjacent / earlier epoch) isn't directly exercised — both fixtures only produce leaf_epoch and leaf_epoch + 1 certs.
2. The ensure!(next_epoch_qc().is_none()) isn't hit — the deciding QC is always built with non_epoch_change, so the field is always None.

Both are "lock in the behavior so a refactor can't silently drop it" nits, not correctness concerns.

Security / performance

No new concerns. Per-cert stake_table() / next_epoch_stake_table() calls follow the existing pattern; the extra table is only fetched when a cert is actually from the next epoch. The leaf.rs change is a doc-comment update accurately describing the new behavior.
• Branch: fix/light-client-boundary-qc-epoch

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

The dispatch is correct and, as the PR notes, binding — cert.epoch() reads qc.data.epoch, which is part of the signed VersionedVoteData, so a cert lying about its epoch fails the signature check against whichever quorum it is routed to.

Minor test-coverage note: this explicit bail! arm (a cert from a non-adjacent epoch, e.g. leaf_epoch + 2, or an earlier epoch than the leaf) isn't directly exercised by the two new tests. Both new fixtures only produce certs from leaf_epoch and leaf_epoch + 1. A small third case that hands verify_qc_chain_and_get_version a deciding QC tagged epoch 3 (or epoch 0) and asserts it hits this branch would lock in the rejection behavior independently of the signature check.

[claude]

The reasoning holds: a cert from leaf_epoch + 1 sits at the start of the next epoch (block 11 in the fixture), never at that epoch's transition boundary, so it legitimately carries no next-epoch QC — and verifying one here would need the stake table two epochs past the proof's, which isn't available. Rejecting is the right call.

Coverage nit (matches the PR's stated review focus): this ensure! isn't hit by either new test — the deciding QC in epoch_boundary_fixture is built with Certificate::non_epoch_change, so next_epoch_qc() is always None. Consider a variant that attaches a next-epoch QC to the deciding cert and asserts this specific error fires, so a future refactor can't silently drop the check.