Integrate the MVBA with Chorus - #3242
michael-yxchen wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the MVBA/Chorus integration. The wiring itself (output conversion, unicast plumbing, prepare_qc removal, propose idempotency) is sound — update_prep_qc runs before the Committing transition, and the drain-before-finalize ordering correctly gets the decide-broadcast out before abandon() clears the MVBA queue. Findings inline; the two that matter most are a compile break in the sim tests and a liveness gap for laggards that the removed DecidedEcho used to cover.
|
|
||
| /// Shared resources needed to spawn a slot instance. | ||
| pub struct ChorusContext { | ||
| pub node_id: NodeId, |
There was a problem hiding this comment.
Compile break: the two ChorusContext literals in the sim tests were not updated for this new field — monad-mcp-chorus-sim/tests/chorus.rs:53 and monad-mcp-chorus-sim/tests/cadence_conductor.rs:140 still build ChorusContext { key, validator_data, da_handle }. cargo test -p monad-mcp-chorus-sim fails with E0063 (missing field \node_id`), so both integration-test binaries — including the fallback_mvba.rssuite this PR touches — never compile. (Couldn't run cargo in this environment becausemonad-execution/` isn't checked out, but the literals plainly have 3 of 4 fields.)
| } | ||
|
|
||
| self.decided = true; | ||
| self.fallback.abandon(); |
There was a problem hiding this comment.
Liveness gap for laggards (the main design question in this PR): with DecidedEcho removed, the commit certificate goes out exactly once per decider, and finalize immediately calls abandon() — plus runtime.rs closes the slot on SlotOutput::Finalize. After that, deciders drop all fallback messages, including BlockRequest.
Failure scenario: 4 validators enter fallback; A/B/C form the CommitQC, each broadcasts it once, finalizes, abandons. Laggard D receives a certificate but doesn't hold the metablock, so pending_decide issues BlockRequest — which every peer now ignores. BlockRetransmit re-sends into silence forever; D holds a valid 2f+1 certificate it can never complete, and the slot never finalizes for it. (A validator that never got an input at all is similarly wedged: decision_proof() requires Phase::Decided, which requires input.is_some(), so a stored decided_qc is never surfaced.)
The module doc says fetchability after decide is "the recovery layer's contract", but no such layer exists in the tree yet, and the MVBA-level sim tests can't catch this — they drive bare MonadMvba instances that never abandon, which is exactly why T5 passes while the Chorus wiring would deadlock. If the recovery layer is a planned follow-up, worth an explicit TODO/issue link here; otherwise consider keeping the instance answering BlockRequest (and re-broadcasting on request) until the slot is closed, rather than abandoning at decide.
| MVBAOutput::ScheduleTimer { | ||
| duration, | ||
| timer_event, | ||
| } => self.schedule_timer(duration, TimerEvent::Fallback(timer_event)), |
There was a problem hiding this comment.
The MVBA's timer contract isn't honored on this path. local_time_out (monad_mvba/mod.rs) relies on "scheduling replaces any pending timer for the same event, so an echo-triggered timeout still leaves exactly one live timer", and block_store.rs states "One arming is live per pending entry: re-arming replaces". The sim's MvbaRuntime::drain implements that (armed.retain(|_, armed| *armed != timer_event)), but CadenceDriver::schedule_slot_timer (driver.rs:146) mints a fresh WakeId per call and never evicts a superseded wake — this PR is what first routes MVBA timers through it.
Failure scenario: f+1 timeout messages for view v arrive before the view timer fires → local_time_out arms a second live ViewTimeout(v) alongside enter_view's. Both fire, each re-arms, so the duplicate is permanent and the node retransmits timeouts at twice the intended rate (and BlockRetransmit compounds the same way). Either implement replace-semantics in CadenceDriver or make the MVBA tolerate duplicate timer fires.
| self.enter_fallback(cert, block); | ||
| } | ||
| }, | ||
| TimerEvent::Fallback(event) => { |
There was a problem hiding this comment.
handle_timer has no if self.decided { return; } guard, unlike handle_message and handle_deadline — even though the decided field's comment says "ignore all subsequent messages and timer events". The new Fallback arm is safe only by accident (the MVBA's abandoned flag absorbs it), and the fast-path arms can still run after finalization: a late FallbackDecisionDelayElapsed can broadcast an EnterFallbackCert for an already-committed slot or reschedule itself indefinitely. Today slot_manager.close() masks this upstream, but the asymmetry is a latent trap for any driver that delivers timers directly. Suggest adding the guard.
| } | ||
|
|
||
| Message::Fallback(message) => { | ||
| self.fallback.handle_message(author, message); |
There was a problem hiding this comment.
Sender validation for the new fallback path: MonadMvba::handle_message only debug_assert!s that sender is in the validator set, and the prepare/commit vote arms then store the vote with no membership or signature check at ingress. In release, a vote from an unknown NodeId gets buffered and can reach stake aggregation (valset[node_id] indexing) at QC-formation time. If membership filtering is the network layer's job (as the sim's Validated::new_unchecked comment suggests), fine — but worth a note here, since handle_pre_prepare does verify sender/signature while the vote arms verify neither.
|
|
||
| // the pre-prepare and prepare votes are sent inside the window and lost, the | ||
| // commit certificates echoed after it and kept: block sync, not slow recovery | ||
| // the pre-prepare and prepare votes are sent inside the window and lost; the |
There was a problem hiding this comment.
With the echo gone, T5 now passes on a timing coincidence: the partition window is 0..2.5*LATENCY and the deciders' single CommitQc broadcast happens ~3 message-delays in, i.e. just outside the window. Widen the window, add jitter, or add one hop and the sole broadcast is lost with no second chance — the failure mode is a hang of run_until_all_decided, not a near-miss. Similarly, agreement_survives_a_lossy_network now depends on none of the three one-shot broadcasts being dropped for the straggler. Both tests assert an eventual-delivery property the protocol no longer guarantees (see the finalize/abandon comment in chorus.rs).
|
|
||
| /// Prepare certificate formed, commit vote sent | ||
| /// Prepare certificate formed, commit vote sent. The certificate itself is the | ||
| /// instance's lock, held in `high_prep_qc` |
There was a problem hiding this comment.
Two nits on this removal:
-
The stated invariant ("the certificate itself is the instance's lock, held in
high_prep_qc") isn't quite enforced:update_prep_qconly adopts on strictly-higher view, so if a harvested timeout already raisedhigh_prep_qcabove the current view, the just-formed prepare QC is dropped and held nowhere. Safety holds today because the retained lock is strictly higher, but the comment overstates what the code guarantees. -
Preparing::commitstill takesprepare_qcby value, now used only by thedebug_assert_eq!— soapply_prepare_qcclones a fullStrongQcper transition purely for a release-stripped assert. A reference (or dropping the param) avoids it.
0fe1d45 to
eb34d47
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It makes substantial consensus-path wiring changes (fallback MVBA integration, timers, and finalization proof flow) that warrant careful human validation beyond automated review.
Pull request overview
This PR wires the real Monad MVBA implementation into Chorus’s fallback path so that fallback agreement, message dispatch, timers, and finalization are handled end-to-end by Chorus rather than via a placeholder path.
Changes:
- Integrates
MonadMvba<Metablock, EnterFallbackCert>intoChorus, routing fallback wire messages and MVBA timers throughMessage::Fallback/TimerEvent::Fallbackand draining MVBA outputs into slot outputs. - Adds
SlotOutput::Unicastand driver/runtime support to enable MVBA block-request/response (peer-to-peer) traffic. - Cleans up MVBA internals and tests by removing the decided-echo mechanism and exposing
FallbackCommitQcas the fallback finalization proof.
File summaries
| File | Description |
|---|---|
| monad-mcp-chorus/src/slot/mod.rs | Adds SlotOutput::Unicast to support single-peer sends from slot consensus. |
| monad-mcp-chorus/src/slot/fast.rs | Removes the old FallbackPath spawn hook now that Chorus owns fallback instantiation. |
| monad-mcp-chorus/src/slot/fallback/monad_mvba/tests.rs | Updates MVBA tests to reflect one-shot decision broadcast semantics and removed decided-echo behavior. |
| monad-mcp-chorus/src/slot/fallback/monad_mvba/phases.rs | Removes unused prepare QC storage from the committing phase. |
| monad-mcp-chorus/src/slot/fallback/monad_mvba/mod.rs | Removes decided-echo timer/output and exports FallbackCommitQc; decision is now broadcast once per deciding node. |
| monad-mcp-chorus/src/slot/fallback/monad_mvba/certificates.rs | Makes FallbackCommitQc public for use as a finalization proof outside MVBA internals. |
| monad-mcp-chorus/src/slot/fallback/mod.rs | Removes the placeholder FallbackPath implementation and re-exports FallbackCommitQc. |
| monad-mcp-chorus/src/slot/chorus.rs | Instantiates and dispatches MVBA fallback within Chorus; converts MVBA outputs (broadcast/unicast/timers) into slot outputs and finalizes on decision_proof. |
| monad-mcp-chorus/src/runtime.rs | Handles SlotOutput::Unicast by delegating to the driver. |
| monad-mcp-chorus/src/driver.rs | Extends the driver trait and CadenceDriver to support unicast slot messages. |
| monad-mcp-chorus-sim/tests/fallback_mvba.rs | Updates simulation test descriptions/assertions to align with new decision broadcast behavior. |
| monad-mcp-chorus-sim/tests/chorus.rs | Updates test wiring to include the new node_id in ChorusContext. |
| monad-mcp-chorus-sim/tests/cadence_conductor.rs | Updates conductor test wiring to include the new node_id in ChorusContext. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// memory is unbounded until this validator proposes into it: the MVBA | ||
| /// buffers what arrives without entering views or garbage-collecting. I | ||
| /// have a proposal to conditionally start fallback without self propose |
The fallback path is the real MVBA instead of a placeholder. Chorus builds a MonadMvba<Metablock, EnterFallbackCert> at slot open and owns the dispatch: wrapped wire messages and timers route in through Message::Fallback and TimerEvent::Fallback, every touch is followed by a drain converting MVBA outputs into slot outputs, and the MVBA's CommitQC is the fallback finalization proof. SlotOutput::Unicast is new for the MVBA's block responses. Includes the MVBA cleanups this needed: the decided echo and the unread Committing::prepare_qc are gone, FallbackCommitQc is exported. This code was generated using Claude Opus 5.
eb34d47 to
944f6f3
Compare
The fallback path is the real MVBA instead of a placeholder. Chorus builds a MonadMvba<Metablock, EnterFallbackCert> at slot open and owns the dispatch: wrapped wire messages and timers route in through Message::Fallback and TimerEvent::Fallback, every touch is followed by a drain converting MVBA outputs into slot outputs, and the MVBA's CommitQC is the fallback finalization proof. SlotOutput::Unicast is new for the MVBA's block responses. Includes the MVBA cleanups this needed: the decided echo and the unread Committing::prepare_qc are gone, FallbackCommitQc is exported.
This code was generated using Claude Opus 5.