fix(types): validate the decoded epoch schedule in DynamicEpocher::read_cfg

[erkancamli]

Problem

Both query paths on DynamicEpocher document the invariant they depend on. bounds:

It could be optimized to O(log n) by using binary search, since the segments are sorted by both start_epoch and start_height.

and containing repeats it verbatim. bounds resolves a query by scanning the segments in reverse on start_epoch; containing scans in reverse on start_height. They agree only on a schedule shaped the way update_length builds one: segment 0 anchored at (epoch 0, height 0), and each later segment starting at exactly prev.start_height + (start_epoch - prev.start_epoch) * prev.length.

update_length maintains that, because it anchors a new segment at bounds(target_epoch).0. read_cfg does not check it. It validates only that the segment count is non-zero and that each length is non-zero:

if length == 0 {
    return Err(Error::Invalid("DynamicEpocher", "zero-length segment"));
}
segments.push(Segment { start_epoch, start_height, length });

read_cfg is the import path for untrusted bytes: ConsensusState::read_cfg calls it, and ConsensusState::try_from(&Checkpoint) decodes exactly those bytes, so it covers checkpoint bootstrap as well as every persisted-state load. The decode is already written with that threat model in mind, as the comment just above the loop shows:

Do not size the Vec from segments_len: it is an attacker-controlled u32 [...]

so the ordering invariant looks like an oversight rather than a deliberate omission.

What goes wrong

Feed it two segments: (epoch 0, height 0, length 100) and (epoch 5, height 50, length 100). update_length could never produce the second one, since it would anchor epoch 5 at height 500, not 50. The decode accepts it, and the two documented-as-consistent queries then disagree about the same height:

decode accepted a self-contradictory schedule:
  containing(50) = Some(EpochInfo { epoch: Epoch(5), height: Height(50),
                                    first: Height(50), last: Height(149) })
  while epoch 0 spans Some((Height(0), Height(99)))

That matters because the two sides are used for different jobs. Checkpoint's Step 5 exists to catch an epocher that disagrees with the rest of the state, and its comment names the threat directly:

a buggy (or colluding) checkpoint creator could sign a state whose position does not match

but Step 5 only consults epoch_bounds, the bounds side. Runtime epoch-boundary classification goes through the other side: is_first_block_of_epoch / is_last_block_of_epoch / is_penultimate_block_of_epoch in types/src/utils.rs all call containing, and those drive checkpoint creation and committee transitions in the finalizer, plus header_view_binds_to_round in the syncer. So a schedule can satisfy Step 5 while the importing node classifies epoch boundaries differently from the rest of the network.

The sibling that already does this

WithdrawalQueue::read_flat rejects an out-of-order decoded deque, with a comment that states the same reasoning:

Every runtime enqueue uses current_epoch + validator_withdrawal_num_epochs [...] with a monotonic current_epoch, so a legitimately serialized deque is non-decreasing by epoch; an out-of-order one is a tampered/corrupt artifact and is rejected here.

Same shape, same exposure. This PR gives the epocher decode the same treatment.

Fix

Reject in read_cfg anything update_length could not have produced: the first segment must start at (epoch 0, height 0); later segments must have a strictly increasing start_epoch and a start_height that continues the preceding segment's schedule exactly. The arithmetic is checked, so a crafted schedule cannot overflow its way past the comparison.

No legitimate schedule changes: every segment update_length appends satisfies this by construction.

Tests

read_cfg_rejects_a_schedule_the_builder_could_not_produce builds the crafted schedule above and asserts the decode is refused. If the decode succeeds, it prints both query results so the failure explains itself rather than just saying "expected Err".

It fails on main with the message quoted above, and passes with the fix.

• cargo test -p summit-types --lib → 478 passed, 0 failed, 1 ignored
• cargo test -p summit --lib tests::checkpointing → 19 passed, 0 failed (these exercise real checkpoint join and replay flows, which encode and decode live epochers, so they confirm legitimate schedules still decode)
• cargo fmt --check clean

A second, smaller thing I did not include

SszTree::verify_proof (types/src/ssz_tree.rs) omits the terminal idx == 1 check that its sibling verify_proof_gindex performs, and does not bound leaf_index against 1 << depth, so a proof generated for leaf i also verifies for i + 2^depth. I confirmed that by running it. I left it out of this PR because verify_proof has no production caller: every real proof path goes through verify_proof_gindex, which is correct. Happy to open a separate hardening PR if you want it closed anyway.

[erkancamli]

No workflow runs have been approved on this one yet, so there are no checks to look at. Could someone enable them?

Locally: cargo test -p summit-types --lib is 478 passed, and cargo test -p summit --lib tests::checkpointing is 19 passed, which matters here because those exercise real checkpoint join and replay flows that encode and decode live epochers, so they confirm legitimate schedules still decode under the new check. cargo fmt --check is clean.

Same question applies to #460 and #458 if it is easier to enable them together.

[erkancamli]

Two of the three guards I added were untested, and I only found that by mutating them rather than by reading the test.

Replacing each guard's condition with if false { on the previous head:

guard disabled	cargo test -p summit-types --lib dynamic_epocher
first segment must start at (0, 0)	green — nothing caught it
segment start epochs strictly increasing	green — nothing caught it
start height continues the preceding segment	failed

The single test buffer (0,0,100),(5,50,100) only trips the third one, so two thirds of the validation could have been deleted with CI staying green. That is the shape of thing a reviewer is entitled to point at, so I would rather fix it than have it found.

Pushed read_cfg_rejects_a_first_segment_that_is_not_anchored_at_zero and read_cfg_rejects_non_increasing_segment_start_epochs, each with several buffers, and re-ran the same mutation sweep. Now every guard is pinned:

guard disabled	result	failing test
first segment at (0, 0)	FAILED, 22 passed / 1 failed	read_cfg_rejects_a_first_segment_that_is_not_anchored_at_zero
strictly increasing start epochs	FAILED, 22 passed / 1 failed	read_cfg_rejects_non_increasing_segment_start_epochs
height continuity	FAILED, 22 passed / 1 failed	read_cfg_rejects_a_schedule_the_builder_could_not_produce

I also tightened the original assertion. It was if let Ok(..) { panic!(...) }, which would have passed on an EndOfBuffer too; it now pins Error::Invalid with the specific message, matching the style of the neighbouring tests in this module. The diagnostic panic is kept, because the contradiction it prints is the clearest explanation of why the decode must fail.

Current: cargo test -p summit-types --lib 480 passed, cargo test -p summit --lib tests::checkpointing 19 passed.

One thing worth stating plainly that the description left implicit: the reason this is worth doing at all is that ConsensusState::try_from(checkpoint) runs at node/src/args.rs:1308, which reaches read_cfg, before verify_checkpoint_chain_with_weak_subjectivity at node/src/args.rs:587. So an operator bootstrapping from a third-party or corrupted checkpoint directory decodes the schedule before anything has checked a signature. I should have led with that rather than with the type contract.

I also checked that nothing committed breaks: there are no .ssz, checkpoint or genesis artifacts in the tree encoding a DynamicEpocher (genesis config carries blocks_per_epoch, not a segment list), and the invariant is exactly what new and update_length produce, so no schedule the builder can make is rejected.