feat(new-protocol): tear down legacy stack after cutover decides

[bfish713]

Tear down the legacy consensus stack after cutover

Once the coordinator has emitted LEGACY_SHUTDOWN_DECIDE_COUNT (100) decide events, handle_events triggers ConsensusHandle::shut_down_legacy(): the legacy→coordinator forwarders are aborted and the legacy tasks + network are shut down via a new SystemContextHandle::shut_down_tasks_and_network(), which — unlike shut_down() — leaves in-flight DRB computations on the shared membership coordinator running and legacy in-memory state readable. CombinedNetworks::shut_down now also drops its dedup caches and per-view channels since the network object stays alive after the cutover.

When the base version already starts at the cutover (≥ 0.6), the legacy stack never runs at all: start_consensus tears it down instead of starting legacy consensus, and init_node no longer blocks boot waiting for CDN/libp2p readiness (a pure-0.6 network needs no legacy infra deployed). The legacy network objects are still constructed — the production network type is fixed to CombinedNetworks — but are shut down before consensus starts. Upgraded networks (base < 0.6) keep the 100-decide window so lagging peers can finish crossing the boundary.

Outbound external (request-response) messages permanently switch to the coordinator's network once it's running. They're framed in the same versioned Message envelope as the legacy path because the coordinator's network sends external payloads verbatim and the receiver's fallback decoder expects that framing — bare payload bytes made receivers parse garbage lengths and abort on exabyte-scale allocations (caught by the new test). VID-share fetches (request_vid_shares) switch from RequestType::Broadcast to RequestType::Batched — the last broadcast user — because the coordinator path only carries direct external messages; batched requests reach up to 100 peers within the request's 40s window (5 every 2s), covering the full committee.

Test: test_new_protocol_survives_legacy_shutdown runs a base-0.6 network (legacy stack torn down at startup on every node), re-issues shut_down_legacy() mid-run to cover the decide-count trigger path being a harmless second teardown, and verifies decides continue across two epoch boundaries.

Review focus: the external-message framing in send_via_coordinator, and whether aborting the three forwarder tasks at teardown can race with a peer still crossing the cutover boundary.

---

Follow-ups tracked separately:

• versions::decode/ark-serialize preallocate lengths read off the wire with no bound, so any authenticated peer can abort a process with malformed bytes (pre-existing).
• Constructing the legacy networks at all on base ≥ 0.6 (libp2p binds/bootstraps briefly before teardown) could be avoided with a stub network type if desired.
• The automatic decide-count trigger in handle_events is not driven end-to-end by a test (the test calls shut_down_legacy() directly); a #[cfg(test)]-overridable threshold would allow that if we want the coverage.

🤖 Generated with Claude Code

[gemini-code-assist]

Caution

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

[claude]

Claude finished @bfish713's task in 3m 53s —— View job

---

Claude Code Review

Reviewed the full diff against origin/main. Overall this is a well-scoped, clearly-commented change and the design (keeping DRB + in-memory legacy state alive while tearing down tasks/network) matches the stated intent. Two findings worth addressing before merge, plus answers to the review-focus questions and some minor notes. I've left 2 inline comments.

---

Findings

1. send_via_coordinator silently drops broadcast external messages — and the comment is inaccurate (medium) — inline

The comment claims "Nothing sends these today", but broadcast external messages are produced today:

• request_vid_shares uses RequestType::Broadcast (api.rs:611-613)
• → Sender::send_broadcast_message (request_response/network.rs:40)
• → OutboundMessage::Broadcast(MessageKind::External(..)) (request-response/src/lib.rs:401-412)

Post-cutover these hit the other => arm and are dropped (warn + a 40s timeout in the caller). So an availability VID-share query that must fetch from peers will fail once legacy is torn down. This is exactly the follow-up "confirm nothing relies on request-response catchup post-cutover." At minimum correct the comment; ideally decide whether broadcast VID catchup must survive the cutover.

2. The automatic decide-count trigger is not covered by the test (low / test-coverage) — inline

test_new_protocol_survives_legacy_shutdown calls shut_down_legacy() directly, so the NewDecide counting, the == LEGACY_SHUTDOWN_DECIDE_COUNT threshold, and the detached spawn in handle_events (context.rs:644-653) are never exercised. An off-by-one (== vs >=), a wrong event variant, or broken spawn wiring would pass this test. Consider a cfg(test) override to lower the threshold and drive it through real decides.

---

Review-focus answers

External-message framing in send_via_coordinator — Looks correct. It wraps the payload in Message { sender, kind } and serializes with BincodeSerializer::<StaticVersion<0,0>>, byte-for-byte matching the legacy direct path (external_event_handler.rs:130-143). Since the coordinator's network ships external payloads verbatim and the receiver's fallback decoder expects that envelope, the framing is consistent. The MessageKind::External(_) guard on the match arm is a good touch — it means only external kinds take this path.

Forwarder-teardown vs. a peer crossing the cutover — I believe this is safe. The three forwarders (forward_legacy_timeout_votes / forward_legacy_high_qc / forward_legacy_epoch_changes) feed this node's own coordinator from this node's own legacy consensus; they don't serve peers directly. The peer-facing effect is the legacy network shutdown, and that's gated by the 100-decide margin (≈5 epochs at EPOCH_HEIGHT=20). Because decides are cluster-synchronized, once one honest node has 100 new-protocol decides every honest node is at roughly the same height, so no honest peer should still be mid-cutover 100 decides later. The one scenario to keep in mind is a peer that crashes/restarts right at the boundary and then needs legacy request-response catchup from others after they've already torn legacy down — worth confirming that new-protocol catchup fully covers that (your second follow-up).

---

Minor notes (non-blocking)

• Per-message client_api().await in the outbound loop (external_event_handler.rs:116): pre-cutover this calls activate() every message, taking legacy_handle.read() + cur_view().await. Low volume (catchup) so probably fine, but it's new per-message work on that path.
• Counter resets on restart: new_protocol_decides is in-memory, so a node restarting post-cutover relaunches legacy and needs another 100 new decides before tearing it down again. Likely acceptable, just flagging.
• Double shutdown: after shut_down_legacy, a later shut_down() re-runs shut_down_tasks_and_network on the legacy handle (broadcasts Shutdown again, network.shut_down() again, registry shutdowns again). Worth a quick confirm these are idempotent — CombinedNetworks::shut_down now also re-runs drop_contents/clear, which is fine.
• CombinedNetworks::shut_down cleanup (combined_network.rs:352-354): dropping the dedup caches and per-view channels once the object outlives shutdown is the right call. drop_contents shrinking to NonZeroUsize::MIN rather than truly freeing is fine since no messages flow post-shutdown.

Nice

• error! → info! for the shutdown log lines is correct now that shutdown is a normal cutover step.
• The shut_down refactor cleanly preserves prior behavior (cancel_all_drb + shut_down_tasks_and_network) while exposing the DRB-preserving variant.

[claude]

The comment "Nothing sends these today" is not accurate — broadcast external messages are sent today.

request_vid_shares issues a RequestType::Broadcast request (api.rs:611-613), which the request-response protocol routes through Sender::send_broadcast_message (request_response/network.rs:40) → OutboundMessage::Broadcast(MessageKind::External(..)) (request-response/src/lib.rs:401-412).

Post-cutover, those broadcasts land in this other => arm and are silently dropped (warn + eventual 40s timeout in the caller). So an availability VID-share query that needs to fetch shares from peers will fail once the legacy stack is torn down. This is precisely the follow-up "confirm nothing relies on request-response catchup post-cutover."

Two things worth doing:

1. Fix the comment so it doesn't claim broadcasts never occur.
2. Decide whether broadcast VID-share catchup must keep working post-cutover; if so it needs a real path through the coordinator's network, if not, a clearer/rate-limited error would beat a silent drop + timeout.

[claude]

This test calls shut_down_legacy() directly, which is great for verifying the teardown itself. But it bypasses the production trigger entirely: the NewDecide counting, the == LEGACY_SHUTDOWN_DECIDE_COUNT threshold, and the detached spawn in handle_events (context.rs:644-653) are never exercised.

Consider either (a) a #[cfg(test)] override that lowers LEGACY_SHUTDOWN_DECIDE_COUNT so a test can drive real decides past the threshold and confirm the event-handler path fires exactly once, or (b) a note that the automatic trigger is covered elsewhere. As written, an off-by-one in the counter (== vs >=), a wrong event variant, or the spawn wiring would not be caught.

[github-actions]

Successfully created backport PR for release-ff:

• [Backport release-ff] feat(new-protocol): tear down legacy stack after cutover decides #4720