Skip to content

refactor(app): route the duty path through ConsensusController - #575

Open
iamquang95 wants to merge 1 commit into
mainfrom
feat/port-priority
Open

refactor(app): route the duty path through ConsensusController#575
iamquang95 wants to merge 1 commit into
mainfrom
feat/port-priority

Conversation

@iamquang95

Copy link
Copy Markdown
Collaborator

Problem

Charon constructs QBFT through ConsensusController, but Pluto wired QBFT directly.

Fix

Match Charon by constructing consensus through ConsensusController, wiring its default QBFT instance to P2P and its swappable current-consensus wrapper to core.

@emlautarom1

Copy link
Copy Markdown
Collaborator

@claude

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @emlautarom1's task in 5m 18s —— View job


Review: route the duty path through ConsensusController

  • Gather context (PR diff, commits)
  • Compare against Charon Go source (core/consensus/controller.go, wrapper.go, app/app.go)
  • Read node/mod.rs, node/wire.rs, controller.rs, wrapper.rs, qbft/component.rs
  • Check test changes + CI status (7/7 green)
  • Post review

Verdict

The refactor is faithful to Charon and the shape is right. ConsensusController::newdefault_qbft() into the QBFT p2p behaviour, current_consensus() into the core duty path mirrors app.go:608-649 (DefaultConsensus()wirePrioritise, CurrentConsensus()core.Wire), and start() starting only the default impl matches controller.go:64-77. Exposing a concrete default_qbft() is the correct Rust analogue of Charon's coreCons.(*qbft.Consensus) type assertion (app.go:678) — Rust can't downcast Arc<dyn Consensus>, so a typed accessor is the idiomatic equivalent. Single-instance sharing is asserted by the new Arc::ptr_eq test, which is the assertion that actually matters here. No blocking issues found; everything below is a follow-up or a nit.

1. Debugger loses its only wiring point (parity gap) — crates/consensus/src/controller.rs:49

Replacing debugger: Debugger with sniffer: SnifferSink diverges from Charon, where NewConsensusController takes Debugger and passes debugger.AddInstance as the sniffer (controller.go:42-46), and the same debugger is handed to wireMonitoringAPI to serve /debug/consensus (app.go:319-322). After this PR, Debugger has zero callers outside debugger.rs and its own tests, and the app passes sniffer: Arc::new(|_| {}) (node/mod.rs:454) — so completed-instance capture is dead.

This is not a regression from this PR (the app already passed a no-op sniffer straight to qbft), but the PR removes the one place in Pluto that connected Debugger → sniffer, so the parity gap is now invisible from the wiring. Either keep debugger: Debugger in controller::Config (Charon-shaped, config.debugger.sniffer() internally) and have the app construct one + mount debugger.router("/debug/consensus") on the monitoring router, or leave the generic sink and file a tracked follow-up next to the existing TODO(#402 part B). Fix this →

2. ConsensusWrapper doesn't implement the Consensus trait — crates/consensus/src/wrapper.rs:46

Charon asserts var _ core.Consensus = (*consensusWrapper)(nil) (wrapper.go:19) and CurrentConsensus() returns the interface, so the duty path cannot reach SetImpl. Pluto's wrapper only has inherent methods, which forces current_consensus() -> Arc<ConsensusWrapper> (controller.rs:110) and hands wire_core_workflow a type on which set_impl() and start() are both callable — start() in particular would panic, since qbft::Consensus::start expects to be called exactly once (qbft/component.rs:468-474).

Adding impl Consensus for ConsensusWrapper and returning Arc<dyn Consensus> would restore Charon's encapsulation and drop the concrete type out of WireInputs. Pre-existing to this diff, but this PR is what puts the wrapper on the duty path, so it's the natural moment. Related: nothing today calls wrapper.start() or default_qbft().start(), so a short doc line on ConsensusController::start ("call exactly once; do not also start the wrapper or the default qbft directly") would be cheap insurance now that the same instance is reachable from three owners.

3. Comment is inaccurate about Go — crates/consensus/src/controller.rs:60-62

Go has no equivalent: qbft.NewConsensus registers its own stream handler.

Half right. Go has no equivalent for the p2p half, but it does for the priority half: wirePrioritise receives DefaultConsensus() and asserts coreCons.(*qbft.Consensus) (app.go:674-682). Since the doc comment lists the priority protocol as a reason for the field, pointing at that assertion is more useful than claiming no equivalent exists.

4. Nits

  • controller.rs:84let default_consensus: Arc<dyn Consensus> = Arc::clone(&qbft) as Arc<dyn Consensus>; — the as is redundant given the annotation. Line 88's default_consensus.clone() is also inconsistent with the Arc::clone style used everywhere else in this file.
  • controller.rs:99default_consensus() now has no non-test caller (the app uses default_qbft()). Keeping it for Charon parity is defensible; just noting the two accessors return the same object.
  • tests/wiring.rs:284-313build_consensus builds qbft and wraps it in ConsensusWrapper by hand, bypassing the controller the app now uses. ConsensusController::new(...).current_consensus() takes the same config fields and would exercise the real path.
  • wire.rs:793-796 — boxing the consensus error is fine (nothing in the workspace matches on qbft::RunnerError variants, and #[error(transparent)] preserves Display), but #[from] Box<dyn Error + Send + Sync> will now silently absorb any already-boxed error into the Consensus variant. Harmless today; worth remembering if more error sources join that function.

Known limitation carried over from Charon

ConsensusWrapper::subscribe forwards to whatever impl is current at registration time, so a future set_impl swap silently drops dutydb.store. Charon's wrapper has exactly the same behaviour, so this is correct parity — but whoever lands TODO(#402 part B) (node/mod.rs:415) should decide deliberately rather than inherit it by accident.
· branch feat/port-priority

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants