fix(consensus): persist high QC and restore lock on restart (V0.5)

[lukaszrzasik]

Closes #<ISSUE_NUMBER>

This PR:

• update_high_qc2 is no longer no-op. It now uses the existing append_high_qc2 to persist the high QC.
• update_next_epoch_high_qc2 is also no longer no-op and now uses the existing append_next_epoch_high_qc2
• append_next_epoch_high_qc2 is a new method that does what store_next_epoch_quorum_certificate did but atomically (using a transaction for sql and a write lock for fs)
• store_next_epoch_quorum_certificate has been removed
• load_consensus_state now loads the high QC and the next epoch QC from the persistent storage during startup, making sure the high qc and the next epoch qc correspond to each other.
• the locked view is now based on the persisted high qc
• load_consensus_state now builds the initializer via HotShotInitializer::load, which restores undecided_leaves/undecided_state from the persisted proposals. Without this, the recovered high QC pointed at leaves consensus couldn't see, so heights between the anchor and the high QC were never re-decided — the cause of the api::test::test_restart hang (3×360s timeout → ~17s pass).
• HotShotInitializer::load optionally takes the anchor's validated state and delta; the sequencer passes its full genesis state, hotshot tests pass None (unchanged behavior).
• HotShotInitializer fields are now private — construction goes through from_genesis/load only, so recovery can never again skip restoring the undecided maps; external readers use new accessors.

This PR does not:

Key places to review:

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

[claude]

Claude finished @lukaszrzasik's task in 3m 18s —— View job

---

Code Review: Persist high QC and restore lock on restart

• Gather context and read existing review comments
• Read changed files and understand the PR
• Review code quality, security, performance
• Post detailed review feedback

---

Summary

This PR fixes a correctness bug where update_high_qc2 and update_next_epoch_high_qc2 on the Storage<SeqTypes> for Arc<P> impl were no-ops (Ok(())), meaning the high QC was never persisted during normal consensus voting — only during decide events via store_eqc. On restart, locked_view regressed to the anchor leaf's view, potentially allowing the node to vote for conflicting proposals it should have been locked out of.

The fix has three parts:

1. Route update_high_qc2 to append_high_qc2 (which has monotonic CAS at the backend).
2. Route update_next_epoch_high_qc2 to store_next_epoch_quorum_certificate with an application-level monotonicity guard.
3. Recover locked_view on restart by folding the persisted high QC into the initializer and using max(high_qc.view, anchor.view) for the lock.

Verdict

The fix is well-motivated and the implementation is correct. The tests are solid — both the round-trip persistence test and the consensus recovery test cover the essential invariants. I left three inline comments:

File	Severity	Issue
traits.rs:1230-1239	Medium	TOCTOU in update_next_epoch_high_qc2 — load-then-store is not atomic unlike the CAS in append_high_qc2. Consistent with existing update_eqc pattern, but worth hardening.
traits.rs:731-747	Low	Recovery logic correctly replaces next_epoch_high_qc when switching to a newer running high QC, but a clarifying comment would help future readers.
lib.rs:385-388	Nit	Direct field access view_number vs accessor view_number() inconsistency.

No blocking issues found. The tests thoroughly verify both the persistence path and the recovery path.

[claude]

Potential TOCTOU race on the next-epoch QC monotonicity check.

Unlike update_high_qc2 which routes to append_high_qc2 (atomic compare-and-set at the FS/SQL backend level), this performs a load-then-store at the application layer. Between the load_next_epoch_quorum_certificate and store_next_epoch_quorum_certificate calls, a concurrent call with a newer QC could complete its store, only to be overwritten by this call's stale value — store_next_epoch_quorum_certificate blindly overwrites in both the FS and SQL backends.

I see update_eqc has the same pattern, so this is consistent with existing code, but it's worth considering whether store_next_epoch_quorum_certificate should gain an atomic CAS like append_high_qc2 has, especially now that it's called from a hot path (every update_next_epoch_high_qc2 invocation).

Nit: the condition < existing.view_number() allows equal-view writes (no-op but triggers a write). Matching append_high_qc2's strictly-newer semantic (existing.view_number() < view) would avoid the redundant store:

Suggested change

	if let Some(existing) = (**self).load_next_epoch_quorum_certificate().await?
	&& next_epoch_high_qc.view_number() < existing.view_number()
	{
	return Ok(());
	}
	(**self)
	.store_next_epoch_quorum_certificate(next_epoch_high_qc)
	.await
	}
	if let Some(existing) = (**self).load_next_epoch_quorum_certificate().await?
	&& next_epoch_high_qc.view_number() <= existing.view_number()
	{
	return Ok(());
	}
	(**self)
	.store_next_epoch_quorum_certificate(next_epoch_high_qc)
	.await

[claude]

Nice recovery logic. One question on the semantics: when the running_high_qc path is taken, next_epoch_high_qc is replaced unconditionally — even if the loaded next epoch QC is None or fails verification. This could discard a valid next_epoch_high_qc that was already set from the eQC path on lines 724–729.

I believe this is correct because a next-epoch QC must verify against its paired high QC (the .filter on line 742–746 enforces this), so if the high QC has advanced past the eQC, the old next-epoch QC is no longer valid anyway. But it might be worth a brief comment confirming this intent, since a reader might wonder why a valid next_epoch_high_qc is being thrown away.

[claude]

This is the key safety fix — locked_view now correctly tracks the highest QC seen before the restart instead of regressing to the anchor. Looks correct.

Minor: initializer.high_qc.view_number accesses the field directly while anchored_leaf.view_number() uses the accessor method. Both work, but using the accessor consistently would be slightly more uniform:

Suggested change

	let recovered_locked_view = initializer
	.high_qc
	.view_number
	.max(anchored_leaf.view_number());
	let recovered_locked_view = initializer
	.high_qc
	.view_number()
	.max(anchored_leaf.view_number());

(Only if view_number() exists on QuorumCertificate2 — otherwise ignore.)

[lukaszrzasik]

@claude review again

[claude]

Claude finished @lukaszrzasik's task in 1m 45s —— View job

---

Re-review: Persist high QC and restore lock on restart

• Gather context and read all changed files
• Review code changes against previous feedback
• Assess new commits (atomic CAS fix, monotonic recovery fix)
• Post detailed review feedback

---

All three issues from my first review have been addressed across the two follow-up commits. Here's the updated assessment:

Previous issues — resolved

Issue	Status	How it was fixed
TOCTOU in update_next_epoch_high_qc2	Fixed	Replaced store_next_epoch_quorum_certificate (blind overwrite) with append_next_epoch_high_qc2 — an atomic CAS that mirrors append_high_qc2. Both FS (write-lock + read-before-write) and SQL (read + conditional upsert within a single write tx) backends now prevent regression. The old store_next_epoch_quorum_certificate has been removed entirely.
Recovery could clobber a valid next_epoch_high_qc	Fixed	load_consensus_state now uses is_none_or(|existing| stored.view > existing.view) — it only replaces the next-epoch QC when the stored one is strictly newer than what was already recovered from the eQC path.
view_number field vs accessor inconsistency	Not changed	Still uses initializer.high_qc.view_number (field) at lib.rs:387. This is cosmetic only and not a bug since view_number() just returns the field. Fine to leave.

Current state of the code — looks good

Trait layer (traits.rs):

• update_high_qc2 → append_high_qc2 (atomic CAS, already existed). Correct.
• update_next_epoch_high_qc2 → append_next_epoch_high_qc2 (new atomic CAS). Correct.
• append_next_epoch_high_qc2 has a default no-op impl (Ok(())), which is the right pattern — callers that don't need persistence (like NoStorage) get the default.
• Recovery in load_consensus_state (lines 731–748) correctly layers three sources in priority order: anchor leaf cert pair → eQC checkpoint → running high QC, each gated by view_number() > the current best. The nested next-epoch QC update only fires when the running high QC wins, and only replaces when strictly newer.

FS backend (fs.rs:1217–1243):

• Mirrors append_high_qc2 exactly: acquires write lock, reads existing, compares, writes only if newer. Atomic under the inner write lock. Correct.

SQL backend (sql.rs:2664–2697):

• Read + conditional upsert within a single serializable_retry! write transaction. Matches the append_high_qc2 pattern. The stored >= view guard correctly skips equal-view writes (no redundant upsert). Correct.

Consensus init (lib.rs:385–388):

• locked_view = max(high_qc.view_number, anchor.view_number()) — this is the key safety fix. Prevents the lock from regressing to the anchor view on restart, which could allow voting for conflicting proposals. Correct.

Tests

The new tests are thorough and well-structured:

1. test_append_next_epoch_high_qc2_monotonic — verifies the CAS semantics: advance 5→6, then stale write at 4 is a no-op. Exercises the new atomic path directly.
2. test_storage_update_high_qc2_persists — tests through the Arc<P> Storage impl (the actual type consensus tasks hold), verifying both update_high_qc2 and update_next_epoch_high_qc2 are no longer no-ops. Also checks stale-write rejection.
3. test_load_consensus_state_recovers_high_qc — confirms recovery folds in the persisted high QC (would be view 0 without the fix, correctly view 9 with it).
4. test_load_consensus_state_keeps_newer_next_epoch_qc — the regression test for the clobber scenario: eQC at view 5, running high QC at view 9, stale standalone next-epoch QC at view 3. Verifies the eQC's view-5 next-epoch QC is kept (not regressed to 3, not dropped to None).

All four tests run against both FS and SQL via persistence_types, giving full coverage.

CI failures

The test failures in CI (test_all_restart_one_da, test_epoch_reward_upgrade, test_restart) — test_epoch_reward_upgrade is shown as flaky (17/20), and test_restart timed out at 360s. Worth investigating test_restart specifically since this PR changes restart recovery, but the 360s timeout pattern and 3 attempts suggests it may be a pre-existing flake rather than a regression from this PR. The test_all_restart_one_da test is also listed as "passing" in main history.

Verdict

No blocking issues. The TOCTOU concern from my first review has been fully addressed — append_next_epoch_high_qc2 now has proper atomic CAS in both backends. The recovery logic correctly prevents regression of both high_qc and next_epoch_high_qc. Tests are comprehensive. LGTM.

---

View job

[lukaszrzasik]

@claude review again

[claude]

Claude encountered an error after 1m 11s —— View job

---

Re-review: Persist high QC and restore lock on restart

• Gather context and read all changed files
• Review storage trait changes (traits.rs)
• Review FS backend changes (fs.rs)
• Review SQL backend changes (sql.rs)
• Review HotShot initializer changes (lib.rs)
• Review persistence / test changes (persistence.rs)
• Review remaining files (context.rs, no_storage.rs, coordinator.rs, spinning_task.rs)
• Post final review