mpt: max version should return INVALID_BLOCK_NUM when rewind below its valid range

[Chen-Yifan]

Problem

Rewinding to a version before a ring's first valid root (e.g. a secondary
timeline activated after the rewind target) left next_version at
target + 1. The ring then reported a max_version() whose slot held no
root: get_latest_version() named a version that could not be loaded, and
readers of the latest root got null.

Change

• rewind_to_version on the root offsets ring now resets the ring to the
  empty state when the target predates its entire valid range, so
  max_version() reports INVALID_BLOCK_NUM, identical to a freshly
  activated secondary. The next commit reseeds it through the existing
  fast forward path.
• Rewinding into a gap between valid roots is refused with an assert.
  Together these restore the invariant that a non-empty ring always has a
  valid root at max_version(), and the sec_root != INVALID_OFFSET
  assert in UpdateAux::rewind_to_version stays as a corruption tripwire.
• Refactor: reset_all(version) is split into reset_all() (empty state)
  and advance_next_version(version) (write cursor positioning used by
  fast_forward_next_version). No behavior change.

[Copilot]

Pull request overview

This PR relaxes UpdateAux::rewind_to_version() by removing an assertion that treated a secondary timeline root offset as always valid. This aligns the function’s behavior with existing handling in post_root_fast_offset_() (which already treats INVALID_OFFSET as “no cutoff candidate” via std::nullopt) and with the cutoff selection logic that skips nullopt candidates.

Changes:

• Removed MONAD_ASSERT(sec_root != INVALID_OFFSET) when computing the secondary timeline’s contribution to the fast-list cutoff during rewind.
• Kept the existing behavior where post_root_fast_offset_() returns nullopt for INVALID_OFFSET, and the cutoff calculation ignores nullopt.

---

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

[github-actions]

No findings. The removed assertion was overly strict — secondary_ro[sec_max] can legitimately be INVALID_OFFSET, post_root_fast_offset_ already short-circuits to nullopt for that input (category/mpt/update_aux.cpp:270), and consider() skips nullopt candidates. The change also brings the secondary path in line with the primary path at category/mpt/update_aux.cpp:344-345, which never asserted this, and with the sibling check_root_past_offsets helper at category/mpt/update_aux.cpp:155-157 which treats INVALID_OFFSET as an expected early-return case.

Verdict: CORRECT

🤖 Generated with Claude Code

[Chen-Yifan]

@claude review the updated PR please

[github-actions]

No findings. The rewrite of root_offsets_delegator::rewind_to_version correctly handles the two ill-formed cases that previously left the ring reporting a max_version() with no root:

• Below range (secondary activated after the target): version < version_lower_bound_ → reset_all() and return, matching a freshly activated secondary. UpdateAux::rewind_to_version's cutoff computation guards on sec_max != INVALID_BLOCK_NUM (category/mpt/update_aux.cpp:350), so the empty state flows through correctly without hitting the sec_root != INVALID_OFFSET tripwire on line 352.
• Into a gap between valid roots: caught by the new (*this)[version] != INVALID_OFFSET assert, preserving the invariant that a non-empty ring always has a valid root at max_version().

The reset_all(version) → reset_all() + advance_next_version(version) split is behavior-preserving. advance_next_version's reset branch (curr_max == INVALID_BLOCK_NUM || version - curr_max >= capacity_) writes the same two stores as the old reset_all(version); the small-gap else branch reproduces the old push(INVALID_OFFSET) loop with the same iteration count.

Dropping the trailing update_version_lower_bound_() in rewind_to_version is safe: version < version_lower_bound_ short-circuits to the empty-state path, and in the surviving path version_lower_bound_ still points to a valid slot (its slot is unchanged, since only slots strictly above version are cleared).

The two new tests cover both cases directly, including the ASSERT_DEATH for the gap invariant. Existing consumers of reset_all (clear_ondisk_db, update_root_offset) get the same 0/0 empty state they did before.

Verdict: CORRECT

🤖 Generated with Claude Code