From f2e1913a8a967a631669c01a5b22d330f8a00635 Mon Sep 17 00:00:00 2001 From: Quang Le Date: Thu, 3 Sep 2026 22:08:31 +0700 Subject: [PATCH 1/4] refactor(qbft): make MessageType/UponRule enums and return errors instead of bug panics --- crates/consensus/src/qbft/component.rs | 7 +- crates/consensus/src/qbft/msg.rs | 20 +- crates/consensus/src/qbft/transport.rs | 11 +- crates/core/src/qbft/internal_test.rs | 308 +++++++++++++-- crates/core/src/qbft/mod.rs | 505 ++++++++++++++++++------- 5 files changed, 671 insertions(+), 180 deletions(-) diff --git a/crates/consensus/src/qbft/component.rs b/crates/consensus/src/qbft/component.rs index 27052a93..b6b51567 100644 --- a/crates/consensus/src/qbft/component.rs +++ b/crates/consensus/src/qbft/component.rs @@ -431,9 +431,10 @@ impl Consensus { return Err(Error::InvalidConsensusMessage); } - if !qbft::MessageType::from_wire(msg.r#type).valid() { - return Err(Error::InvalidConsensusMessageType); - } + // The conversion is the admission check: `TryFrom` accepts exactly the + // wire values `MessageType::valid` accepted, so there is no separate + // validity test to keep in sync. + qbft::MessageType::try_from(msg.r#type).map_err(|_| Error::InvalidConsensusMessageType)?; let duty = msg.duty.as_ref().ok_or(Error::InvalidConsensusMessage)?; let duty_type = diff --git a/crates/consensus/src/qbft/msg.rs b/crates/consensus/src/qbft/msg.rs index 5517d38e..e9e24979 100644 --- a/crates/consensus/src/qbft/msg.rs +++ b/crates/consensus/src/qbft/msg.rs @@ -21,9 +21,10 @@ //! protobuf bytes of the inner message. //! //! Inbound callers validate message type, duty type, peer membership, rounds, -//! and signatures before constructing `Msg`. This adapter preserves raw -//! message types, while invalid duty wire values project to -//! `DutyType::Unknown`. +//! and signatures before constructing `Msg`. Only the stored protobuf keeps +//! the raw wire integers: `Msg::type_` collapses any message type outside +//! `1..=5` to `MessageType::Unknown`, and invalid duty wire values project +//! to `DutyType::Unknown`. use std::{any, collections::HashMap, fmt, sync}; @@ -203,7 +204,11 @@ impl Msg { } impl SomeMsg for Msg { - /// Returns the QBFT message type preserved from the wire value. + /// Returns the QBFT message type decoded from the stored protobuf. + /// + /// Wire values outside `1..=5` collapse to `MessageType::Unknown`; the + /// raw integer survives only in the protobuf that `to_consensus_msg` + /// rebuilds. fn type_(&self) -> MessageType { MessageType::from_wire(self.msg.r#type) } @@ -537,7 +542,12 @@ mod tests { let debug = format!("{msg:?}"); - assert!(debug.contains("type: \"\"")); + // `MessageType` can no longer hold `99`, so the label is `unknown` + // rather than charon's empty string for an out-of-range `MsgType` + // (`typeLabels` map miss, `core/qbft/qbft.go:89-101`). A message with + // such a type never reaches production: `verify_msg` rejects it before + // `Msg::new` is called. + assert!(debug.contains("type: \"unknown\""), "got {debug}"); } #[test] diff --git a/crates/consensus/src/qbft/transport.rs b/crates/consensus/src/qbft/transport.rs index bd355536..6cc9c519 100644 --- a/crates/consensus/src/qbft/transport.rs +++ b/crates/consensus/src/qbft/transport.rs @@ -396,8 +396,13 @@ mod tests { assert!(msg::verify_msg_sig(msg.msg(), &key.public_key()).unwrap()); } + /// `MessageType` can no longer hold an out-of-range wire value, so the old + /// "unknown wire value is preserved verbatim" behaviour is gone: an + /// unrepresentable value collapses to `Unknown` before it reaches + /// `create_msg`, and `create_msg` writes `Unknown`'s wire value `0`. + /// `verify_msg` then rejects `0` on the receiving side. #[test] - fn create_msg_preserves_unknown_message_type() { + fn create_msg_writes_unknown_message_type_as_zero() { let key = secret_key(); let duty = duty(); let mut request = create_msg_request(&duty, &key); @@ -405,9 +410,11 @@ mod tests { request.peer_idx = 2; request.round = 3; + assert_eq!(request.type_, qbft::MessageType::Unknown); + let msg = create_msg(request).unwrap(); - assert_eq!(msg.msg().r#type, 99); + assert_eq!(msg.msg().r#type, 0); } #[test] diff --git a/crates/core/src/qbft/internal_test.rs b/crates/core/src/qbft/internal_test.rs index d534b723..e35f33a8 100644 --- a/crates/core/src/qbft/internal_test.rs +++ b/crates/core/src/qbft/internal_test.rs @@ -18,7 +18,6 @@ use crossbeam::channel as mpmc; use std::{ collections::{BTreeMap, HashMap, VecDeque}, fmt::Write as _, - panic::{self, AssertUnwindSafe}, sync::{ Arc, Mutex, atomic::{AtomicIsize, AtomicUsize, Ordering}, @@ -47,7 +46,13 @@ const TEST_WAIT_TIMEOUT: Duration = Duration::from_secs(1); // protocol progress, so slow-but-progressing parallel runs should not fail. const TEST_STALL_TIMEOUT: Duration = Duration::from_secs(20); -type RunOutcome = std::thread::Result>; +/// What one `qbft::run` thread reported back. +/// +/// `run` no longer panics on an internal sanity-check failure — it returns +/// [`QbftError::SanityCheck`] — so the harness collects a plain [`Result`] +/// instead of catching unwinds. Charon's `qbft.Run` behaves the same way after +/// its `recover` in `core/qbft/qbft.go:187-198`. +type RunOutcome = Result<()>; type TestMsgRef = Msg; struct TestQbft; @@ -393,17 +398,15 @@ fn test_qbft(test: Test) { } let keepalive = (keep_value_sender, vs_chan_tx); - let run_result = panic::catch_unwind(AssertUnwindSafe(|| { - qbft::run( - &token, - &defs, - &trans, - &test.instance, - i, - input_value_rx, - vs_chan_rx, - ) - })); + let run_result = qbft::run( + &token, + &defs, + &trans, + &test.instance, + i, + input_value_rx, + vs_chan_rx, + ); drop(keepalive); run_chan_tx.send((i, run_result)).expect(WRITE_CHAN_ERR); }); @@ -606,7 +609,7 @@ fn test_qbft(test: Test) { let (node, outcome) = res.expect(READ_CHAN_ERR); last_progress = time::Instant::now(); - if !matches!(outcome, Ok(Ok(()))) { + if outcome.is_err() { if !decided { cts.cancel(); clock.cancel(); @@ -710,22 +713,13 @@ impl Trace { fn format_run_outcome(outcome: &RunOutcome) -> String { match outcome { - Ok(Ok(())) => "ok".to_string(), - Ok(Err(err)) => format!("error {err:?}"), - Err(payload) => { - if let Some(msg) = payload.downcast_ref::<&str>() { - format!("panic {msg}") - } else if let Some(msg) = payload.downcast_ref::() { - format!("panic {msg}") - } else { - "panic ".to_string() - } - } + Ok(()) => "ok".to_string(), + Err(err) => format!("error {err:?}"), } } fn outcome_is_error(outcome: &RunOutcome, expected: fn(&QbftError) -> bool) -> bool { - matches!(outcome, Ok(Err(err)) if expected(err)) + matches!(outcome, Err(err) if expected(err)) } fn assert_upon_rule(expected: UponRule, actual: UponRule) { @@ -954,7 +948,7 @@ fn deterministic_unit(seed: u64, msg: &Msg, target: i64, stream_id: u6 fn deterministic_msg_u64(seed: u64, msg: &Msg, target: i64, stream_id: u64) -> u64 { let mut value = splitmix64(seed ^ stream_id); - value = splitmix64(value ^ i64_to_u64(msg.type_().0)); + value = splitmix64(value ^ i64_to_u64(i64::from(msg.type_()))); value = splitmix64(value ^ i64_to_u64(msg.instance())); value = splitmix64(value ^ i64_to_u64(msg.source())); value = splitmix64(value ^ i64_to_u64(msg.round())); @@ -1470,7 +1464,7 @@ fn is_justified_pre_prepare_mixed_round_change_prepare_fixture() { def.nodes = 4; def.is_leader = Box::new(make_is_leader(4)); - assert!(is_justified_pre_prepare(&def, &1, &preprepare, 0)); + assert!(is_justified_pre_prepare(&def, &1, &preprepare, 0).unwrap()); } // Tests duplicate PRE-PREPARE rule handling after compare failure. @@ -1699,14 +1693,16 @@ fn classify_rules() { let preprepare = new_msg(MSG_PRE_PREPARE, 0, 1, 1, 1, 0, 0, 0, None); assert_upon_rule( UPON_JUSTIFIED_PRE_PREPARE, - classify(&def, &0, 1, 2, &HashMap::new(), &preprepare).0, + classify(&def, &0, 1, 2, &HashMap::new(), &preprepare) + .unwrap() + .0, ); let prepares = new_prepare_quorum(1, 2); let buffer = buffer_by_source(&prepares); assert_upon_rule( UPON_QUORUM_PREPARES, - classify(&def, &0, 1, 2, &buffer, &prepares[2]).0, + classify(&def, &0, 1, 2, &buffer, &prepares[2]).unwrap().0, ); let commits = vec![ @@ -1717,7 +1713,7 @@ fn classify_rules() { let buffer = buffer_by_source(&commits); assert_upon_rule( UPON_QUORUM_COMMITS, - classify(&def, &0, 1, 2, &buffer, &commits[2]).0, + classify(&def, &0, 1, 2, &buffer, &commits[2]).unwrap().0, ); let future_round_changes = vec![ @@ -1726,14 +1722,19 @@ fn classify_rules() { ]; let buffer = buffer_by_source(&future_round_changes); assert!( - classify(&def, &0, 1, 2, &buffer, &future_round_changes[1]).0 == UPON_F_PLUS1_ROUND_CHANGES + classify(&def, &0, 1, 2, &buffer, &future_round_changes[1]) + .unwrap() + .0 + == UPON_F_PLUS1_ROUND_CHANGES ); let unjust_round_changes = new_round_change_quorum(1, 2, 9); let buffer = buffer_by_source(&unjust_round_changes); assert_upon_rule( UPON_UNJUST_QUORUM_ROUND_CHANGES, - classify(&def, &0, 1, 2, &buffer, &unjust_round_changes[2]).0, + classify(&def, &0, 1, 2, &buffer, &unjust_round_changes[2]) + .unwrap() + .0, ); } @@ -1803,7 +1804,7 @@ fn invalid_round_change_prepared_rounds_are_filtered_from_call_sites(invalid_pr: value, Some(&invalid_prepares), ); - assert!(!is_justified_round_change(&def, &invalid_round_change)); + assert!(!is_justified_round_change(&def, &invalid_round_change).unwrap()); let mut only_invalid = new_round_change_quorum(target_round, invalid_pr, value); only_invalid.extend(invalid_prepares); @@ -2356,9 +2357,7 @@ fn test_qbft_chain_split(test: ChainSplitTest) { let (vs_tx, vs_rx) = mpmc::bounded::(1); v_tx.send(value_source).expect(WRITE_CHAN_ERR); vs_tx.send(value_source).expect(WRITE_CHAN_ERR); - let run_result = panic::catch_unwind(AssertUnwindSafe(|| { - qbft::run(&token, &defs, &transport, &instance, i, v_rx, vs_rx) - })); + let run_result = qbft::run(&token, &defs, &transport, &instance, i, v_rx, vs_rx); drop(v_tx); drop(vs_tx); run_chan_tx.send((i, run_result)).expect(WRITE_CHAN_ERR); @@ -2547,3 +2546,238 @@ fn test_qbft_chain_split(test: ChainSplitTest) { } }); } + +// === Regression tests for the former `panic!("bug: ...")` sites ============ +// +// Each of the eleven internal sanity checks in `mod.rs` used to abort the +// process; they now return `QbftError::SanityCheck`, mirroring charon's +// `recover` in `core/qbft/qbft.go:187-198`. Ten of them are reachable from a +// test; the eleventh (`run`'s `match rule` arm) is unreachable by construction +// because `classify` can return neither `Nothing` (filtered directly above the +// match) nor `RoundTimeout` (produced only by the timer branch), which is +// exactly what the exhaustive match now records in the type system. + +fn assert_sanity_check(result: Result, expected: &str) { + match result { + Err(QbftError::SanityCheck(msg)) => assert_eq!(msg, expected), + other => panic!("want sanity check {expected:?}, got {other:?}"), + } +} + +/// Builds a message whose justification entries themselves carry +/// justifications. `new_msg` deliberately strips that nesting, so the nested +/// shape has to be assembled by hand. +fn new_nested_msg(type_: MessageType, source: i64, round: i64) -> Msg { + let leaf = TestMsg { + msg_type: MSG_PREPARE, + instance: 0, + peer_idx: source, + round, + value: 1, + value_source: 0, + pr: 0, + pv: 0, + justify: Some(vec![]), + }; + let mut inner = leaf.clone(); + inner.msg_type = MSG_ROUND_CHANGE; + inner.justify = Some(vec![leaf]); + + Arc::new(TestMsg { + msg_type: type_, + instance: 0, + peer_idx: source, + round, + value: 1, + value_source: 0, + pr: 0, + pv: 0, + justify: Some(vec![inner]), + }) +} + +/// Former `mod.rs:398`. The round-1 leader caches its PRE-PREPARE +/// justification while it has no input value; a quorum of `ROUND_CHANGE` for +/// round 1 then drives a second `broadcast_own_pre_prepare` with the cache +/// still set. +/// +/// This is the only one of the eleven sites reachable from the wire, and +/// reaching it needs a quorum of distinctly-signed messages — above the +/// Byzantine threshold, so it is not a single-peer trigger. What changes here +/// is that the failure is now a typed `QbftError` the caller can classify +/// instead of an unwind. +#[test] +fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { + let cts = CancellationTokenSource::new(); + let token = cts.token().clone(); + let (receive_tx, receive_rx) = mpmc::bounded::>(4); + for source in 1..=3 { + receive_tx + .send(new_round_change(source, 1, 0, 0)) + .expect(WRITE_CHAN_ERR); + } + // Close the channel: if the sanity check ever stops firing, `run` gets + // `ChannelError` once the queue drains instead of blocking forever. + drop(receive_tx); + + let mut def = noop_definition(); + def.nodes = 4; + def.fifo_limit = 100; + def.is_leader = Box::new(make_is_leader(4)); + + let transport = Transport { + broadcast: Box::new(|_| Ok(())), + receive: receive_rx, + }; + + // Process 1 leads instance 0 round 1 and never receives an input value, so + // its Algorithm 1:11 PRE-PREPARE only fills the justification cache. + let outcome = qbft::run( + &token, + &def, + &transport, + &0, + 1, + mpmc::never(), + mpmc::never(), + ); + + assert_sanity_check(outcome, "bug: justification cache must be nil"); +} + +/// Former `mod.rs:902`. Unreachable from the wire — `verify_msg` rejects every +/// type outside `1..=5` — so it is driven through the internal API. +#[test] +fn classify_reports_sanity_check_for_unknown_message_type() { + let mut def = noop_definition(); + def.nodes = 4; + + let msg = new_msg(MessageType::Unknown, 0, 1, 1, 1, 0, 0, 0, None); + + assert_sanity_check( + classify(&def, &0, 1, 2, &HashMap::new(), &msg), + "bug: invalid type", + ); +} + +/// Former `mod.rs:913`. `classify` only produces `UPON_F_PLUS1_ROUND_CHANGES` +/// with at least `f+1` messages, so a shorter set is caller error. +#[test] +fn next_min_round_reports_sanity_check_for_short_justification() { + let mut def = noop_definition(); + def.nodes = 4; // f+1 == 2 + + let frc = vec![new_round_change(1, 3, 0, 0)]; + + assert_sanity_check(next_min_round(&def, &frc, 1), "bug: Frc too short"); +} + +/// Former `mod.rs:921`. `get_fplus1_round_changes` filters on +/// `MSG_ROUND_CHANGE`, so a foreign type can only arrive by caller error. +#[test] +fn next_min_round_reports_sanity_check_for_non_round_change() { + let mut def = noop_definition(); + def.nodes = 4; + + let frc = vec![ + new_msg(MSG_PREPARE, 0, 1, 3, 1, 0, 0, 0, None), + new_round_change(2, 3, 0, 0), + ]; + + assert_sanity_check( + next_min_round(&def, &frc, 1), + "bug: Frc contain non-round change", + ); +} + +/// Former `mod.rs:923`. `get_fplus1_round_changes` only collects rounds above +/// the current one. +#[test] +fn next_min_round_reports_sanity_check_for_non_future_round() { + let mut def = noop_definition(); + def.nodes = 4; + + let frc = vec![new_round_change(1, 1, 0, 0), new_round_change(2, 1, 0, 0)]; + + assert_sanity_check( + next_min_round(&def, &frc, 1), + "bug: Frc round not in future", + ); +} + +/// Former `mod.rs:947`. The first of the two core type gates; like +/// `classify`'s, unreachable from the wire. +#[test] +fn is_justified_reports_sanity_check_for_unknown_message_type() { + let mut def = noop_definition(); + def.nodes = 4; + + let msg = new_msg(MessageType::Unknown, 0, 1, 1, 1, 0, 0, 0, None); + + assert_sanity_check(is_justified(&def, &0, &msg, 0), "bug: invalid message type"); +} + +/// Former `mod.rs:955`. Only `is_justified`'s `RoundChange` arm calls this. +#[test] +fn is_justified_round_change_reports_sanity_check_for_other_types() { + let mut def = noop_definition(); + def.nodes = 4; + + let msg = new_msg(MSG_PREPARE, 0, 1, 1, 1, 0, 0, 0, None); + + assert_sanity_check( + is_justified_round_change(&def, &msg), + "bug: not a round change message", + ); +} + +/// Former `mod.rs:1024`. Only `is_justified`'s `Decided` arm calls this. +#[test] +fn is_justified_decided_reports_sanity_check_for_other_types() { + let mut def = noop_definition(); + def.nodes = 4; + + let msg = new_msg(MSG_COMMIT, 0, 1, 1, 1, 0, 0, 0, None); + + assert_sanity_check( + is_justified_decided(&def, &msg), + "bug: not a decided message", + ); +} + +/// Former `mod.rs:1048`. Only `is_justified`'s `PrePrepare` arm calls this. +#[test] +fn is_justified_pre_prepare_reports_sanity_check_for_other_types() { + let mut def = noop_definition(); + def.nodes = 4; + def.is_leader = Box::new(make_is_leader(4)); + + let msg = new_msg(MSG_ROUND_CHANGE, 0, 1, 1, 1, 0, 0, 0, None); + + assert_sanity_check( + is_justified_pre_prepare(&def, &0, &msg, 0), + "bug: not a preprepare message", + ); +} + +/// Former `mod.rs:1397`. Nesting is not expressible on the wire: `QBFTMsg` has +/// no justification field and `consensus::qbft::msg::Msg::new` builds every +/// justification with an empty justification list. The nested shape therefore +/// has to be built directly. +#[test] +fn flatten_reports_sanity_check_for_nested_justifications() { + let nested = new_nested_msg(MSG_ROUND_CHANGE, 1, 1); + let buffer = buffer_by_source(&[nested]); + + assert_sanity_check(flatten(&buffer), "bug: nested justifications"); +} + +/// `flatten` accepts the one-level shape the wire actually produces. +#[test] +fn flatten_accepts_single_level_justifications() { + let prepares = new_prepare_quorum(1, 1); + let round_change = new_msg(MSG_ROUND_CHANGE, 0, 1, 2, 0, 0, 1, 1, Some(&prepares)); + let buffer = buffer_by_source(&[round_change]); + + assert_eq!(flatten(&buffer).unwrap().len(), 4); +} diff --git a/crates/core/src/qbft/mod.rs b/crates/core/src/qbft/mod.rs index 7da05eb2..80bbf331 100644 --- a/crates/core/src/qbft/mod.rs +++ b/crates/core/src/qbft/mod.rs @@ -63,6 +63,15 @@ pub enum QbftError { #[error("bug: expected only comparison or timeout error, got {0}")] UnexpectedCompareError(Box), + /// An internal sanity check failed. + /// + /// Mirrors charon `core/qbft/qbft.go:187-198`, which recovers exactly the + /// panics whose message contains "bug" and turns them into + /// `fmt.Errorf("qbft sanity check: %v", r)`. The payload keeps charon's + /// original panic string so log output stays greppable. + #[error("qbft sanity check: {0}")] + SanityCheck(&'static str), + /// Parent cancellation token was canceled. #[error("context canceled")] ContextCanceled, @@ -187,56 +196,104 @@ impl Definition { } } -/// Defines the QBFT message types +/// Defines the QBFT message types. +/// +/// NOTE: message type ordering MUST not change, since it breaks backwards +/// compatibility. The discriminants are the wire values, so they are frozen and +/// always written explicitly; they mirror charon `core/qbft/qbft.go` `MsgType`. #[derive(PartialEq, Eq, Clone, Copy, Debug)] -pub struct MessageType(i64); +#[repr(i64)] +pub enum MessageType { + /// Unknown message type. + Unknown = 0, + /// PRE-PREPARE message type. + PrePrepare = 1, + /// PREPARE message type. + Prepare = 2, + /// COMMIT message type. + Commit = 3, + /// ROUND-CHANGE message type. + RoundChange = 4, + /// DECIDED catch-up message type. + Decided = 5, +} -// NOTE: message type ordering MUST not change, since it breaks backwards -// compatibility. /// Unknown message type. -pub const MSG_UNKNOWN: MessageType = MessageType(0); +pub const MSG_UNKNOWN: MessageType = MessageType::Unknown; /// PRE-PREPARE message type. -pub const MSG_PRE_PREPARE: MessageType = MessageType(1); +pub const MSG_PRE_PREPARE: MessageType = MessageType::PrePrepare; /// PREPARE message type. -pub const MSG_PREPARE: MessageType = MessageType(2); +pub const MSG_PREPARE: MessageType = MessageType::Prepare; /// COMMIT message type. -pub const MSG_COMMIT: MessageType = MessageType(3); +pub const MSG_COMMIT: MessageType = MessageType::Commit; /// ROUND-CHANGE message type. -pub const MSG_ROUND_CHANGE: MessageType = MessageType(4); +pub const MSG_ROUND_CHANGE: MessageType = MessageType::RoundChange; /// DECIDED catch-up message type. -pub const MSG_DECIDED: MessageType = MessageType(5); +pub const MSG_DECIDED: MessageType = MessageType::Decided; -const MSG_SENTINEL: MessageType = MessageType(6); // intentionally not public +/// Wire integer that does not name a valid QBFT message type. +#[derive(Debug, PartialEq, Eq, thiserror::Error)] +#[error("invalid QBFT message type: {0}")] +pub struct InvalidMessageType( + /// The rejected wire value. + pub i64, +); impl MessageType { - /// Converts a stable wire integer into a message type without clamping. + /// Converts a stable wire integer into a message type, mapping every value + /// outside `1..=5` (including `0`) to [`MessageType::Unknown`]. + /// + /// This is the lossy, infallible conversion for the places that must + /// produce a message type without failing: [`SomeMsg::type_`] and debug + /// formatting. Use [`TryFrom`] to reject an unknown wire value. pub fn from_wire(value: i64) -> Self { - Self(value) + Self::try_from(value).unwrap_or(Self::Unknown) } /// Returns true when the message type is one of the known QBFT wire types. + /// + /// Equivalent to charon's `t > MsgUnknown && t < msgSentinel`: + /// `msgSentinel` (wire value 6) is not representable as a variant, so + /// the only invalid value left is [`MessageType::Unknown`]. pub fn valid(&self) -> bool { - self.0 > MSG_UNKNOWN.0 && self.0 < MSG_SENTINEL.0 + !matches!(self, Self::Unknown) + } +} + +impl TryFrom for MessageType { + type Error = InvalidMessageType; + + /// Converts a wire integer into a message type, rejecting exactly what + /// [`MessageType::valid`] rejects. This is the wire boundary's single + /// admission check. + fn try_from(value: i64) -> std::result::Result { + match value { + 1 => Ok(Self::PrePrepare), + 2 => Ok(Self::Prepare), + 3 => Ok(Self::Commit), + 4 => Ok(Self::RoundChange), + 5 => Ok(Self::Decided), + _ => Err(InvalidMessageType(value)), + } } } impl From for i64 { fn from(value: MessageType) -> Self { - value.0 + value as Self } } impl Display for MessageType { /// Formats the message type using the stable wire/debug label. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let s = match self.0 { - 0 => "unknown", - 1 => "pre_prepare", - 2 => "prepare", - 3 => "commit", - 4 => "round_change", - 5 => "decided", - _ => "", + let s = match self { + Self::Unknown => "unknown", + Self::PrePrepare => "pre_prepare", + Self::Prepare => "prepare", + Self::Commit => "commit", + Self::RoundChange => "round_change", + Self::Decided => "decided", }; write!(f, "{s}") } @@ -271,42 +328,71 @@ pub trait SomeMsg: Send + Sync + fmt::Debug { pub type Msg = sync::Arc>; /// Defines the event based rules that are triggered when messages are received. -#[derive(PartialEq, Eq, Hash, Clone, Copy)] -pub struct UponRule(i64); +/// +/// NOTE: rule ordering MUST not change: the discriminants mirror charon +/// `core/qbft/qbft.go` `UponRule`'s `iota` order, and the [`Display`] labels +/// appear in operator logs. Discriminants are always written explicitly. +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)] +#[repr(i64)] +pub enum UponRule { + /// No upon-rule fired. + Nothing = 0, + /// PRE-PREPARE was justified. + JustifiedPrePrepare = 1, + /// Quorum PREPARE messages was received. + QuorumPrepares = 2, + /// Quorum COMMIT messages was received. + QuorumCommits = 3, + /// Quorum ROUND-CHANGE messages was received but not justified. + UnjustQuorumRoundChanges = 4, + /// F+1 future ROUND-CHANGE messages was received. + FPlus1RoundChanges = 5, + /// Quorum ROUND-CHANGE messages was received. + QuorumRoundChanges = 6, + /// DECIDED message was justified. + JustifiedDecided = 7, + /// Round timer expired. This is not triggered by a message, but by a timer. + RoundTimeout = 8, +} /// No upon-rule fired. -pub const UPON_NOTHING: UponRule = UponRule(0); +pub const UPON_NOTHING: UponRule = UponRule::Nothing; /// PRE-PREPARE was justified. -pub const UPON_JUSTIFIED_PRE_PREPARE: UponRule = UponRule(1); +pub const UPON_JUSTIFIED_PRE_PREPARE: UponRule = UponRule::JustifiedPrePrepare; /// Quorum PREPARE messages was received. -pub const UPON_QUORUM_PREPARES: UponRule = UponRule(2); +pub const UPON_QUORUM_PREPARES: UponRule = UponRule::QuorumPrepares; /// Quorum COMMIT messages was received. -pub const UPON_QUORUM_COMMITS: UponRule = UponRule(3); +pub const UPON_QUORUM_COMMITS: UponRule = UponRule::QuorumCommits; /// Quorum ROUND-CHANGE messages was received but not justified. -pub const UPON_UNJUST_QUORUM_ROUND_CHANGES: UponRule = UponRule(4); +pub const UPON_UNJUST_QUORUM_ROUND_CHANGES: UponRule = UponRule::UnjustQuorumRoundChanges; /// F+1 future ROUND-CHANGE messages was received. -pub const UPON_F_PLUS1_ROUND_CHANGES: UponRule = UponRule(5); +pub const UPON_F_PLUS1_ROUND_CHANGES: UponRule = UponRule::FPlus1RoundChanges; /// Quorum ROUND-CHANGE messages was received. -pub const UPON_QUORUM_ROUND_CHANGES: UponRule = UponRule(6); +pub const UPON_QUORUM_ROUND_CHANGES: UponRule = UponRule::QuorumRoundChanges; /// DECIDED message was justified. -pub const UPON_JUSTIFIED_DECIDED: UponRule = UponRule(7); +pub const UPON_JUSTIFIED_DECIDED: UponRule = UponRule::JustifiedDecided; /// Round timer expired. -pub const UPON_ROUND_TIMEOUT: UponRule = UponRule(8); // This is not triggered by a message, but by a timer. +pub const UPON_ROUND_TIMEOUT: UponRule = UponRule::RoundTimeout; // This is not triggered by a message, but by a timer. + +impl From for i64 { + fn from(value: UponRule) -> Self { + value as Self + } +} impl Display for UponRule { /// Formats the upon-rule using the stable debug label. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let s = match self.0 { - 0 => "nothing", - 1 => "justified_pre_prepare", - 2 => "quorum_prepares", - 3 => "quorum_commits", - 4 => "unjust_quorum_round_changes", - 5 => "f_plus_1_round_changes", - 6 => "quorum_round_changes", - 7 => "justified_decided", - 8 => "round_timeout", - _ => "", + let s = match self { + Self::Nothing => "nothing", + Self::JustifiedPrePrepare => "justified_pre_prepare", + Self::QuorumPrepares => "quorum_prepares", + Self::QuorumCommits => "quorum_commits", + Self::UnjustQuorumRoundChanges => "unjust_quorum_round_changes", + Self::FPlus1RoundChanges => "f_plus_1_round_changes", + Self::QuorumRoundChanges => "quorum_round_changes", + Self::JustifiedDecided => "justified_decided", + Self::RoundTimeout => "round_timeout", }; write!(f, "{s}") } @@ -395,7 +481,9 @@ pub fn run( // to be used when the input value becomes available. let broadcast_own_pre_prepare = |justification: Vec>| { if ppj_cache.borrow().is_some() { - panic!("bug: justification cache must be none") + return Err(QbftError::SanityCheck( + "bug: justification cache must be none", + )); } if *input_value.borrow() == Default::default() { @@ -504,7 +592,7 @@ pub fn run( } // Drop unjust messages - if !is_justified(d, instance, &msg, compare_failure_round) { + if !is_justified(d, instance, &msg, compare_failure_round)? { (d.logger.unjust)(UnjustLog { instance, process, @@ -516,7 +604,7 @@ pub fn run( buffer_msg(&msg); let (rule, justification) = - classify(d, instance, round.get(), process, &buffer.borrow(), &msg); + classify(d, instance, round.get(), process, &buffer.borrow(), &msg)?; if rule == UPON_NOTHING || is_duplicated_rule(rule, msg.round()) { // Do nothing more if no rule or duplicate rule was triggered continue; @@ -532,7 +620,7 @@ pub fn run( match rule { // Algorithm 2:1 - UPON_JUSTIFIED_PRE_PREPARE => { + UponRule::JustifiedPrePrepare => { change_round(msg.round(), rule); stop_timer(); @@ -585,7 +673,7 @@ pub fn run( } } } - UPON_QUORUM_PREPARES => { + UponRule::QuorumPrepares => { // Algorithm 2:4 // Only applicable to current round prepared_round.set(round.get()); /* == msg.round() */ @@ -594,7 +682,7 @@ pub fn run( broadcast_msg(MSG_COMMIT, &prepared_value.borrow(), None)?; } - UPON_QUORUM_COMMITS | UPON_JUSTIFIED_DECIDED => { + UponRule::QuorumCommits | UponRule::JustifiedDecided => { // Algorithm 2:8 change_round(msg.round(), rule); q_commit = justification; @@ -611,7 +699,7 @@ pub fn run( qcommit: justification, }); } - UPON_F_PLUS1_ROUND_CHANGES => { + UponRule::FPlus1RoundChanges => { // Algorithm 3:5 let justification = justification.expect( @@ -620,7 +708,7 @@ pub fn run( // Only applicable to future rounds change_round( - next_min_round(d, &justification, round.get() /* < msg.round() */), + next_min_round(d, &justification, round.get() /* < msg.round() */)?, rule, ); @@ -631,7 +719,7 @@ pub fn run( broadcast_round_change()?; } - UPON_QUORUM_ROUND_CHANGES => { + UponRule::QuorumRoundChanges => { // Algorithm 3:11 let justification = justification @@ -645,10 +733,15 @@ pub fn run( _ => broadcast_own_pre_prepare(justification)?, } } - UPON_UNJUST_QUORUM_ROUND_CHANGES => { + UponRule::UnjustQuorumRoundChanges => { // Ignore bug or byzantine } - _ => panic!("bug: invalid rule"), + // `classify` never returns these two: `Nothing` is + // filtered above and `RoundTimeout` is timer-only. Charon + // recovers the equivalent panic into an error. + UponRule::Nothing | UponRule::RoundTimeout => { + return Err(QbftError::SanityCheck("bug: invalid rule")); + } } }, @@ -821,71 +914,71 @@ fn classify( process: i64, buffer: &HashMap>>, msg: &Msg, -) -> (UponRule, Option>>) { +) -> Result<(UponRule, Option>>)> { match msg.type_() { - MSG_DECIDED => (UPON_JUSTIFIED_DECIDED, Some(msg.justification())), - MSG_PRE_PREPARE => { + MessageType::Decided => Ok((UPON_JUSTIFIED_DECIDED, Some(msg.justification()))), + MessageType::PrePrepare => { if msg.round() < round { - (UPON_NOTHING, None) + Ok((UPON_NOTHING, None)) } else { - (UPON_JUSTIFIED_PRE_PREPARE, None) + Ok((UPON_JUSTIFIED_PRE_PREPARE, None)) } } - MSG_PREPARE => { + MessageType::Prepare => { // Ignore other rounds, since PREPARE isn't justified. if msg.round() != round { - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } let prepares = - filter_by_round_and_value(&flatten(buffer), MSG_PREPARE, msg.round(), msg.value()); + filter_by_round_and_value(&flatten(buffer)?, MSG_PREPARE, msg.round(), msg.value()); if prepares.len() >= d.quorum_count() { - (UPON_QUORUM_PREPARES, Some(prepares)) + Ok((UPON_QUORUM_PREPARES, Some(prepares))) } else { - (UPON_NOTHING, None) + Ok((UPON_NOTHING, None)) } } - MSG_COMMIT => { + MessageType::Commit => { // Ignore other rounds, since COMMIT isn't justified. if msg.round() != round { - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } let commits = - filter_by_round_and_value(&flatten(buffer), MSG_COMMIT, msg.round(), msg.value()); + filter_by_round_and_value(&flatten(buffer)?, MSG_COMMIT, msg.round(), msg.value()); if commits.len() >= d.quorum_count() { - (UPON_QUORUM_COMMITS, Some(commits)) + Ok((UPON_QUORUM_COMMITS, Some(commits))) } else { - (UPON_NOTHING, None) + Ok((UPON_NOTHING, None)) } } - MSG_ROUND_CHANGE => { + MessageType::RoundChange => { // Only ignore old rounds. if msg.round() < round { - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } - let all = flatten(buffer); + let all = flatten(buffer)?; if msg.round() > round { // Jump ahead if we received F+1 higher ROUND-CHANGEs. if let Some(frc) = get_fplus1_round_changes(d, &all, round) { - return (UPON_F_PLUS1_ROUND_CHANGES, Some(frc)); + return Ok((UPON_F_PLUS1_ROUND_CHANGES, Some(frc))); } - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } /* else msg.round() == round */ let qrc = filter_round_change(&all, msg.round()); if qrc.len() < d.quorum_count() { - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } let Some(qrc) = get_justified_qrc(d, &all, msg.round()) else { - return (UPON_UNJUST_QUORUM_ROUND_CHANGES, None); + return Ok((UPON_UNJUST_QUORUM_ROUND_CHANGES, None)); }; if !(d.is_leader)(LeaderRequest { @@ -893,24 +986,24 @@ fn classify( round: msg.round(), process, }) { - return (UPON_NOTHING, None); + return Ok((UPON_NOTHING, None)); } - (UPON_QUORUM_ROUND_CHANGES, Some(qrc)) - } - _ => { - panic!("bug: invalid type"); + Ok((UPON_QUORUM_ROUND_CHANGES, Some(qrc))) } + // Unreachable from the wire: `verify_msg` rejects any type outside + // `1..=5`, and `is_justified` sees the same value first. + MessageType::Unknown => Err(QbftError::SanityCheck("bug: invalid type")), } } /// Implements algorithm 3:6 and returns the next minimum round from received /// round change messages. -fn next_min_round(d: &Definition, frc: &Vec>, round: i64) -> i64 { +fn next_min_round(d: &Definition, frc: &Vec>, round: i64) -> Result { // Get all RoundChange messages with round (rj) higher than current round // (ri) if frc.len() < d.faulty_plus_one_count() { - panic!("bug: Frc too short"); + return Err(QbftError::SanityCheck("bug: Frc too short")); } // Get the smallest round in the set. @@ -918,9 +1011,9 @@ fn next_min_round(d: &Definition, frc: &Vec>, round: i64 for msg in frc { if msg.type_() != MSG_ROUND_CHANGE { - panic!("bug: Frc contain non-round change"); + return Err(QbftError::SanityCheck("bug: Frc contain non-round change")); } else if msg.round() <= round { - panic!("bug: Frc round not in future"); + return Err(QbftError::SanityCheck("bug: Frc round not in future")); } if rmin > msg.round() { @@ -928,7 +1021,7 @@ fn next_min_round(d: &Definition, frc: &Vec>, round: i64 } } - rmin + Ok(rmin) } /// Returns true if message is justified or if it does not need justification. @@ -937,22 +1030,26 @@ fn is_justified( instance: &T::Instance, msg: &Msg, compare_failure_round: i64, -) -> bool { +) -> Result { match msg.type_() { - MSG_PRE_PREPARE => is_justified_pre_prepare(d, instance, msg, compare_failure_round), - MSG_PREPARE => true, - MSG_COMMIT => true, - MSG_ROUND_CHANGE => is_justified_round_change(d, msg), - MSG_DECIDED => is_justified_decided(d, msg), - _ => panic!("bug: invalid message type"), + MessageType::PrePrepare => { + is_justified_pre_prepare(d, instance, msg, compare_failure_round) + } + MessageType::Prepare => Ok(true), + MessageType::Commit => Ok(true), + MessageType::RoundChange => is_justified_round_change(d, msg), + MessageType::Decided => is_justified_decided(d, msg), + // Unreachable from the wire: `verify_msg` rejects any type outside + // `1..=5`. This is the first of the two type gates in the core. + MessageType::Unknown => Err(QbftError::SanityCheck("bug: invalid message type")), } } /// Returns true if the ROUND_CHANGE message's prepared round and value is /// justified. -fn is_justified_round_change(d: &Definition, msg: &Msg) -> bool { +fn is_justified_round_change(d: &Definition, msg: &Msg) -> Result { if msg.type_() != MSG_ROUND_CHANGE { - panic!("bug: not a round change message"); + return Err(QbftError::SanityCheck("bug: not a round change message")); } // ROUND-CHANGE justification contains quorum PREPARE messages that @@ -965,40 +1062,40 @@ fn is_justified_round_change(d: &Definition, msg: &Msg) -> b // round`. See `valid_round_change_prepared_round` for the full parity // note. if !valid_round_change_prepared_round(msg) { - return false; + return Ok(false); } if prepares.is_empty() { - return pr == 0 && pv == Default::default(); + return Ok(pr == 0 && pv == Default::default()); } // No need to check for all possible combinations, since justified should // only contain a one. if prepares.len() < d.quorum_count() { - return false; + return Ok(false); } let mut uniq = uniq_source(); for prepare in prepares { if !uniq(&prepare) { - return false; + return Ok(false); } if prepare.type_() != MSG_PREPARE { - return false; + return Ok(false); } if prepare.round() != pr { - return false; + return Ok(false); } if prepare.value() != pv { - return false; + return Ok(false); } } - true + Ok(true) } /// Returns true if a ROUND-CHANGE's prepared round is in the valid range @@ -1019,9 +1116,9 @@ fn valid_round_change_prepared_round(msg: &Msg) -> bool { /// Returns true if the decided message is justified by quorum COMMIT messages /// of identical round and value. -fn is_justified_decided(d: &Definition, msg: &Msg) -> bool { +fn is_justified_decided(d: &Definition, msg: &Msg) -> Result { if msg.type_() != MSG_DECIDED { - panic!("bug: not a decided message"); + return Err(QbftError::SanityCheck("bug: not a decided message")); } let v = msg.value(); @@ -1034,7 +1131,7 @@ fn is_justified_decided(d: &Definition, msg: &Msg) -> bool { None, ); - commits.len() >= d.quorum_count() + Ok(commits.len() >= d.quorum_count()) } /// Returns true if the PRE-PREPARE message is justified. @@ -1043,9 +1140,9 @@ fn is_justified_pre_prepare( instance: &T::Instance, msg: &Msg, compare_failure_round: i64, -) -> bool { +) -> Result { if msg.type_() != MSG_PRE_PREPARE { - panic!("bug: not a preprepare message"); + return Err(QbftError::SanityCheck("bug: not a preprepare message")); } if !(d.is_leader)(LeaderRequest { @@ -1053,7 +1150,7 @@ fn is_justified_pre_prepare( round: msg.round(), process: msg.source(), }) { - return false; + return Ok(false); } // Justified if PrePrepare is the first round OR if comparison failed @@ -1062,18 +1159,18 @@ fn is_justified_pre_prepare( .checked_add(1) .expect("compare failure round permits increment"); if msg.round() == 1 || (msg.round() == next_compare_round) { - return true; + return Ok(true); } let Some(pv) = contains_justified_qrc(d, &msg.justification(), msg.round()) else { - return false; + return Ok(false); }; if pv == Default::default() { - return true; // New value being proposed + return Ok(true); // New value being proposed } - msg.value() == pv // Ensure Pv is being proposed + Ok(msg.value() == pv) // Ensure Pv is being proposed } /// Implements algorithm 4:1 and returns true and pv if the messages contains a @@ -1385,7 +1482,7 @@ fn filter_msgs( /// Produce a vector containing all the buffered messages as well as all their /// justifications. -fn flatten(buffer: &HashMap>>) -> Vec> { +fn flatten(buffer: &HashMap>>) -> Result>> { let mut resp: Vec> = Vec::new(); for msgs in buffer.values() { @@ -1394,13 +1491,17 @@ fn flatten(buffer: &HashMap>>) -> Vec> { for j in msg.justification() { resp.push(j.clone()); if !j.justification().is_empty() { - panic!("bug: nested justifications"); + // Unreachable from the wire: `QBFTMsg` has no justification + // field, so nesting is not expressible, and + // `consensus::qbft::msg::Msg::new` builds every wire + // justification with an empty justification list. + return Err(QbftError::SanityCheck("bug: nested justifications")); } } } } - resp + Ok(resp) } /// Construct a function that returns true if the message is from a unique @@ -1414,29 +1515,167 @@ fn uniq_source() -> impl FnMut(&Msg) -> bool { mod tests { use super::*; + /// The wire discriminants are frozen: changing one breaks backwards + /// compatibility with every deployed peer. Pin all six. + #[test] + fn message_type_discriminants_are_frozen() { + assert_eq!(i64::from(MessageType::Unknown), 0); + assert_eq!(i64::from(MessageType::PrePrepare), 1); + assert_eq!(i64::from(MessageType::Prepare), 2); + assert_eq!(i64::from(MessageType::Commit), 3); + assert_eq!(i64::from(MessageType::RoundChange), 4); + assert_eq!(i64::from(MessageType::Decided), 5); + } + + /// The `MSG_*` aliases must keep naming the same wire values they did when + /// they were `MessageType(i64)` constants. + #[test] + fn message_type_aliases_match_variants() { + assert_eq!(MSG_UNKNOWN, MessageType::Unknown); + assert_eq!(MSG_PRE_PREPARE, MessageType::PrePrepare); + assert_eq!(MSG_PREPARE, MessageType::Prepare); + assert_eq!(MSG_COMMIT, MessageType::Commit); + assert_eq!(MSG_ROUND_CHANGE, MessageType::RoundChange); + assert_eq!(MSG_DECIDED, MessageType::Decided); + } + + /// `Display` output reaches logs and metric labels, so the strings are as + /// frozen as the discriminants. + #[test] + fn message_type_display_strings_are_frozen() { + assert_eq!(MessageType::Unknown.to_string(), "unknown"); + assert_eq!(MessageType::PrePrepare.to_string(), "pre_prepare"); + assert_eq!(MessageType::Prepare.to_string(), "prepare"); + assert_eq!(MessageType::Commit.to_string(), "commit"); + assert_eq!(MessageType::RoundChange.to_string(), "round_change"); + assert_eq!(MessageType::Decided.to_string(), "decided"); + } + + #[test] + fn message_type_try_from_accepts_known_wire_values() { + assert_eq!(MessageType::try_from(1), Ok(MessageType::PrePrepare)); + assert_eq!(MessageType::try_from(2), Ok(MessageType::Prepare)); + assert_eq!(MessageType::try_from(3), Ok(MessageType::Commit)); + assert_eq!(MessageType::try_from(4), Ok(MessageType::RoundChange)); + assert_eq!(MessageType::try_from(5), Ok(MessageType::Decided)); + } + + /// `TryFrom` rejects exactly what `valid` rejects, including the old + /// `msgSentinel` value `6`, the unknown value `0`, and both extremes. + #[test] + fn message_type_try_from_rejects_out_of_range_wire_values() { + for value in [0, 6, 99, -1, i64::MIN, i64::MAX] { + assert_eq!( + MessageType::try_from(value), + Err(InvalidMessageType(value)), + "wire value {value} must be rejected" + ); + } + + assert_eq!( + InvalidMessageType(6).to_string(), + "invalid QBFT message type: 6" + ); + } + + /// `from_wire` is the lossy sibling of `TryFrom`: it maps every rejected + /// value onto `Unknown` rather than failing. #[test] - fn message_type_from_wire_preserves_known_types() { - assert_eq!(MessageType::from_wire(0), MSG_UNKNOWN); - assert_eq!(MessageType::from_wire(1), MSG_PRE_PREPARE); - assert_eq!(MessageType::from_wire(2), MSG_PREPARE); - assert_eq!(MessageType::from_wire(3), MSG_COMMIT); - assert_eq!(MessageType::from_wire(4), MSG_ROUND_CHANGE); - assert_eq!(MessageType::from_wire(5), MSG_DECIDED); + fn message_type_from_wire_maps_unknown_values_to_unknown() { + assert_eq!(MessageType::from_wire(1), MessageType::PrePrepare); + assert_eq!(MessageType::from_wire(5), MessageType::Decided); + + for value in [0, 6, 99, -1, i64::MIN, i64::MAX] { + assert_eq!(MessageType::from_wire(value), MessageType::Unknown); + } + } + + /// `valid` accepts precisely the set `TryFrom` accepts, which is what lets + /// the wire boundary do one conversion instead of a conversion plus a + /// separate validity check. + #[test] + fn message_type_valid_matches_try_from() { + assert!(!MessageType::Unknown.valid()); + + for message_type in [ + MessageType::PrePrepare, + MessageType::Prepare, + MessageType::Commit, + MessageType::RoundChange, + MessageType::Decided, + ] { + assert!(message_type.valid()); + assert_eq!( + MessageType::try_from(i64::from(message_type)), + Ok(message_type) + ); + } } + /// `UponRule` is never serialised, but its discriminants are the values the + /// original `UponRule(i64)` constants carried; keep them stable so the + /// `Debug`/`Display` output in logs does not silently shift. #[test] - fn message_type_from_wire_preserves_unknown_wire_value() { - let message_type = MessageType::from_wire(99); + fn upon_rule_discriminants_are_frozen() { + assert_eq!(i64::from(UponRule::Nothing), 0); + assert_eq!(i64::from(UponRule::JustifiedPrePrepare), 1); + assert_eq!(i64::from(UponRule::QuorumPrepares), 2); + assert_eq!(i64::from(UponRule::QuorumCommits), 3); + assert_eq!(i64::from(UponRule::UnjustQuorumRoundChanges), 4); + assert_eq!(i64::from(UponRule::FPlus1RoundChanges), 5); + assert_eq!(i64::from(UponRule::QuorumRoundChanges), 6); + assert_eq!(i64::from(UponRule::JustifiedDecided), 7); + assert_eq!(i64::from(UponRule::RoundTimeout), 8); + } + + #[test] + fn upon_rule_aliases_match_variants() { + assert_eq!(UPON_NOTHING, UponRule::Nothing); + assert_eq!(UPON_JUSTIFIED_PRE_PREPARE, UponRule::JustifiedPrePrepare); + assert_eq!(UPON_QUORUM_PREPARES, UponRule::QuorumPrepares); + assert_eq!(UPON_QUORUM_COMMITS, UponRule::QuorumCommits); + assert_eq!( + UPON_UNJUST_QUORUM_ROUND_CHANGES, + UponRule::UnjustQuorumRoundChanges + ); + assert_eq!(UPON_F_PLUS1_ROUND_CHANGES, UponRule::FPlus1RoundChanges); + assert_eq!(UPON_QUORUM_ROUND_CHANGES, UponRule::QuorumRoundChanges); + assert_eq!(UPON_JUSTIFIED_DECIDED, UponRule::JustifiedDecided); + assert_eq!(UPON_ROUND_TIMEOUT, UponRule::RoundTimeout); + } - assert_eq!(message_type, MessageType(99)); - assert_eq!(i64::from(message_type), 99); - assert!(!message_type.valid()); - assert_eq!(message_type.to_string(), ""); + #[test] + fn upon_rule_display_strings_are_frozen() { + assert_eq!(UponRule::Nothing.to_string(), "nothing"); + assert_eq!( + UponRule::JustifiedPrePrepare.to_string(), + "justified_pre_prepare" + ); + assert_eq!(UponRule::QuorumPrepares.to_string(), "quorum_prepares"); + assert_eq!(UponRule::QuorumCommits.to_string(), "quorum_commits"); + assert_eq!( + UponRule::UnjustQuorumRoundChanges.to_string(), + "unjust_quorum_round_changes" + ); + assert_eq!( + UponRule::FPlus1RoundChanges.to_string(), + "f_plus_1_round_changes" + ); + assert_eq!( + UponRule::QuorumRoundChanges.to_string(), + "quorum_round_changes" + ); + assert_eq!(UponRule::JustifiedDecided.to_string(), "justified_decided"); + assert_eq!(UponRule::RoundTimeout.to_string(), "round_timeout"); } + /// The sanity-check error keeps charon's original panic text so operator + /// greps written against charon logs keep matching. #[test] - fn upon_rule_display_unknown_value_does_not_panic() { - assert_eq!(UponRule(99).to_string(), ""); + fn sanity_check_error_mirrors_charon_wording() { + let err = QbftError::SanityCheck("bug: invalid rule"); + + assert_eq!(err.to_string(), "qbft sanity check: bug: invalid rule"); } } From ff5fca0dfae154273081a762e8583078c0a01160 Mon Sep 17 00:00:00 2001 From: Quang Le Date: Thu, 3 Sep 2026 22:28:23 +0700 Subject: [PATCH 2/4] fix: simplify comments --- crates/consensus/src/qbft/msg.rs | 5 --- crates/consensus/src/qbft/transport.rs | 5 --- crates/core/src/qbft/internal_test.rs | 54 ++++++-------------------- crates/core/src/qbft/mod.rs | 25 +++--------- 4 files changed, 18 insertions(+), 71 deletions(-) diff --git a/crates/consensus/src/qbft/msg.rs b/crates/consensus/src/qbft/msg.rs index e9e24979..cddb7e33 100644 --- a/crates/consensus/src/qbft/msg.rs +++ b/crates/consensus/src/qbft/msg.rs @@ -542,11 +542,6 @@ mod tests { let debug = format!("{msg:?}"); - // `MessageType` can no longer hold `99`, so the label is `unknown` - // rather than charon's empty string for an out-of-range `MsgType` - // (`typeLabels` map miss, `core/qbft/qbft.go:89-101`). A message with - // such a type never reaches production: `verify_msg` rejects it before - // `Msg::new` is called. assert!(debug.contains("type: \"unknown\""), "got {debug}"); } diff --git a/crates/consensus/src/qbft/transport.rs b/crates/consensus/src/qbft/transport.rs index 6cc9c519..f435388a 100644 --- a/crates/consensus/src/qbft/transport.rs +++ b/crates/consensus/src/qbft/transport.rs @@ -396,11 +396,6 @@ mod tests { assert!(msg::verify_msg_sig(msg.msg(), &key.public_key()).unwrap()); } - /// `MessageType` can no longer hold an out-of-range wire value, so the old - /// "unknown wire value is preserved verbatim" behaviour is gone: an - /// unrepresentable value collapses to `Unknown` before it reaches - /// `create_msg`, and `create_msg` writes `Unknown`'s wire value `0`. - /// `verify_msg` then rejects `0` on the receiving side. #[test] fn create_msg_writes_unknown_message_type_as_zero() { let key = secret_key(); diff --git a/crates/core/src/qbft/internal_test.rs b/crates/core/src/qbft/internal_test.rs index e35f33a8..7942585f 100644 --- a/crates/core/src/qbft/internal_test.rs +++ b/crates/core/src/qbft/internal_test.rs @@ -2548,14 +2548,6 @@ fn test_qbft_chain_split(test: ChainSplitTest) { } // === Regression tests for the former `panic!("bug: ...")` sites ============ -// -// Each of the eleven internal sanity checks in `mod.rs` used to abort the -// process; they now return `QbftError::SanityCheck`, mirroring charon's -// `recover` in `core/qbft/qbft.go:187-198`. Ten of them are reachable from a -// test; the eleventh (`run`'s `match rule` arm) is unreachable by construction -// because `classify` can return neither `Nothing` (filtered directly above the -// match) nor `RoundTimeout` (produced only by the timer branch), which is -// exactly what the exhaustive match now records in the type system. fn assert_sanity_check(result: Result, expected: &str) { match result { @@ -2564,9 +2556,7 @@ fn assert_sanity_check(result: Result, expected: &str) { } } -/// Builds a message whose justification entries themselves carry -/// justifications. `new_msg` deliberately strips that nesting, so the nested -/// shape has to be assembled by hand. +/// Builds a message whose justifications themselves carry justifications. fn new_nested_msg(type_: MessageType, source: i64, round: i64) -> Msg { let leaf = TestMsg { msg_type: MSG_PREPARE, @@ -2596,16 +2586,7 @@ fn new_nested_msg(type_: MessageType, source: i64, round: i64) -> Msg }) } -/// Former `mod.rs:398`. The round-1 leader caches its PRE-PREPARE -/// justification while it has no input value; a quorum of `ROUND_CHANGE` for -/// round 1 then drives a second `broadcast_own_pre_prepare` with the cache -/// still set. -/// -/// This is the only one of the eleven sites reachable from the wire, and -/// reaching it needs a quorum of distinctly-signed messages — above the -/// Byzantine threshold, so it is not a single-peer trigger. What changes here -/// is that the failure is now a typed `QbftError` the caller can classify -/// instead of an unwind. +/// Former `mod.rs:398`. #[test] fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { let cts = CancellationTokenSource::new(); @@ -2616,8 +2597,7 @@ fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { .send(new_round_change(source, 1, 0, 0)) .expect(WRITE_CHAN_ERR); } - // Close the channel: if the sanity check ever stops firing, `run` gets - // `ChannelError` once the queue drains instead of blocking forever. + // Closed so a regression fails with `ChannelError` instead of hanging. drop(receive_tx); let mut def = noop_definition(); @@ -2630,8 +2610,6 @@ fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { receive: receive_rx, }; - // Process 1 leads instance 0 round 1 and never receives an input value, so - // its Algorithm 1:11 PRE-PREPARE only fills the justification cache. let outcome = qbft::run( &token, &def, @@ -2645,8 +2623,7 @@ fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { assert_sanity_check(outcome, "bug: justification cache must be nil"); } -/// Former `mod.rs:902`. Unreachable from the wire — `verify_msg` rejects every -/// type outside `1..=5` — so it is driven through the internal API. +/// Former `mod.rs:902`. #[test] fn classify_reports_sanity_check_for_unknown_message_type() { let mut def = noop_definition(); @@ -2660,8 +2637,7 @@ fn classify_reports_sanity_check_for_unknown_message_type() { ); } -/// Former `mod.rs:913`. `classify` only produces `UPON_F_PLUS1_ROUND_CHANGES` -/// with at least `f+1` messages, so a shorter set is caller error. +/// Former `mod.rs:913`. #[test] fn next_min_round_reports_sanity_check_for_short_justification() { let mut def = noop_definition(); @@ -2672,8 +2648,7 @@ fn next_min_round_reports_sanity_check_for_short_justification() { assert_sanity_check(next_min_round(&def, &frc, 1), "bug: Frc too short"); } -/// Former `mod.rs:921`. `get_fplus1_round_changes` filters on -/// `MSG_ROUND_CHANGE`, so a foreign type can only arrive by caller error. +/// Former `mod.rs:921`. #[test] fn next_min_round_reports_sanity_check_for_non_round_change() { let mut def = noop_definition(); @@ -2690,8 +2665,7 @@ fn next_min_round_reports_sanity_check_for_non_round_change() { ); } -/// Former `mod.rs:923`. `get_fplus1_round_changes` only collects rounds above -/// the current one. +/// Former `mod.rs:923`. #[test] fn next_min_round_reports_sanity_check_for_non_future_round() { let mut def = noop_definition(); @@ -2705,8 +2679,7 @@ fn next_min_round_reports_sanity_check_for_non_future_round() { ); } -/// Former `mod.rs:947`. The first of the two core type gates; like -/// `classify`'s, unreachable from the wire. +/// Former `mod.rs:947`. #[test] fn is_justified_reports_sanity_check_for_unknown_message_type() { let mut def = noop_definition(); @@ -2717,7 +2690,7 @@ fn is_justified_reports_sanity_check_for_unknown_message_type() { assert_sanity_check(is_justified(&def, &0, &msg, 0), "bug: invalid message type"); } -/// Former `mod.rs:955`. Only `is_justified`'s `RoundChange` arm calls this. +/// Former `mod.rs:955`. #[test] fn is_justified_round_change_reports_sanity_check_for_other_types() { let mut def = noop_definition(); @@ -2731,7 +2704,7 @@ fn is_justified_round_change_reports_sanity_check_for_other_types() { ); } -/// Former `mod.rs:1024`. Only `is_justified`'s `Decided` arm calls this. +/// Former `mod.rs:1024`. #[test] fn is_justified_decided_reports_sanity_check_for_other_types() { let mut def = noop_definition(); @@ -2745,7 +2718,7 @@ fn is_justified_decided_reports_sanity_check_for_other_types() { ); } -/// Former `mod.rs:1048`. Only `is_justified`'s `PrePrepare` arm calls this. +/// Former `mod.rs:1048`. #[test] fn is_justified_pre_prepare_reports_sanity_check_for_other_types() { let mut def = noop_definition(); @@ -2760,10 +2733,7 @@ fn is_justified_pre_prepare_reports_sanity_check_for_other_types() { ); } -/// Former `mod.rs:1397`. Nesting is not expressible on the wire: `QBFTMsg` has -/// no justification field and `consensus::qbft::msg::Msg::new` builds every -/// justification with an empty justification list. The nested shape therefore -/// has to be built directly. +/// Former `mod.rs:1397`. #[test] fn flatten_reports_sanity_check_for_nested_justifications() { let nested = new_nested_msg(MSG_ROUND_CHANGE, 1, 1); diff --git a/crates/core/src/qbft/mod.rs b/crates/core/src/qbft/mod.rs index 80bbf331..4f81114a 100644 --- a/crates/core/src/qbft/mod.rs +++ b/crates/core/src/qbft/mod.rs @@ -482,7 +482,7 @@ pub fn run( let broadcast_own_pre_prepare = |justification: Vec>| { if ppj_cache.borrow().is_some() { return Err(QbftError::SanityCheck( - "bug: justification cache must be none", + "bug: justification cache must be nil", )); } @@ -1515,8 +1515,7 @@ fn uniq_source() -> impl FnMut(&Msg) -> bool { mod tests { use super::*; - /// The wire discriminants are frozen: changing one breaks backwards - /// compatibility with every deployed peer. Pin all six. + /// Wire discriminants are frozen; changing one breaks deployed peers. #[test] fn message_type_discriminants_are_frozen() { assert_eq!(i64::from(MessageType::Unknown), 0); @@ -1527,8 +1526,6 @@ mod tests { assert_eq!(i64::from(MessageType::Decided), 5); } - /// The `MSG_*` aliases must keep naming the same wire values they did when - /// they were `MessageType(i64)` constants. #[test] fn message_type_aliases_match_variants() { assert_eq!(MSG_UNKNOWN, MessageType::Unknown); @@ -1539,8 +1536,7 @@ mod tests { assert_eq!(MSG_DECIDED, MessageType::Decided); } - /// `Display` output reaches logs and metric labels, so the strings are as - /// frozen as the discriminants. + /// `Display` strings reach logs, so they are frozen too. #[test] fn message_type_display_strings_are_frozen() { assert_eq!(MessageType::Unknown.to_string(), "unknown"); @@ -1560,8 +1556,6 @@ mod tests { assert_eq!(MessageType::try_from(5), Ok(MessageType::Decided)); } - /// `TryFrom` rejects exactly what `valid` rejects, including the old - /// `msgSentinel` value `6`, the unknown value `0`, and both extremes. #[test] fn message_type_try_from_rejects_out_of_range_wire_values() { for value in [0, 6, 99, -1, i64::MIN, i64::MAX] { @@ -1578,8 +1572,6 @@ mod tests { ); } - /// `from_wire` is the lossy sibling of `TryFrom`: it maps every rejected - /// value onto `Unknown` rather than failing. #[test] fn message_type_from_wire_maps_unknown_values_to_unknown() { assert_eq!(MessageType::from_wire(1), MessageType::PrePrepare); @@ -1590,9 +1582,7 @@ mod tests { } } - /// `valid` accepts precisely the set `TryFrom` accepts, which is what lets - /// the wire boundary do one conversion instead of a conversion plus a - /// separate validity check. + /// Same accepted set as `TryFrom`, so the wire boundary needs one check. #[test] fn message_type_valid_matches_try_from() { assert!(!MessageType::Unknown.valid()); @@ -1612,9 +1602,7 @@ mod tests { } } - /// `UponRule` is never serialised, but its discriminants are the values the - /// original `UponRule(i64)` constants carried; keep them stable so the - /// `Debug`/`Display` output in logs does not silently shift. + /// Never serialised, but #637 freezes these values. #[test] fn upon_rule_discriminants_are_frozen() { assert_eq!(i64::from(UponRule::Nothing), 0); @@ -1669,8 +1657,7 @@ mod tests { assert_eq!(UponRule::RoundTimeout.to_string(), "round_timeout"); } - /// The sanity-check error keeps charon's original panic text so operator - /// greps written against charon logs keep matching. + /// Payloads match charon's panic text so log greps keep working. #[test] fn sanity_check_error_mirrors_charon_wording() { let err = QbftError::SanityCheck("bug: invalid rule"); From af9283bf11da5ce408633ff93bc2b8b72d31720b Mon Sep 17 00:00:00 2001 From: Quang Le Date: Thu, 3 Sep 2026 22:43:23 +0700 Subject: [PATCH 3/4] fix: remove no-value tests --- crates/core/src/qbft/internal_test.rs | 114 -------------------------- 1 file changed, 114 deletions(-) diff --git a/crates/core/src/qbft/internal_test.rs b/crates/core/src/qbft/internal_test.rs index 7942585f..6527dad2 100644 --- a/crates/core/src/qbft/internal_test.rs +++ b/crates/core/src/qbft/internal_test.rs @@ -2547,8 +2547,6 @@ fn test_qbft_chain_split(test: ChainSplitTest) { }); } -// === Regression tests for the former `panic!("bug: ...")` sites ============ - fn assert_sanity_check(result: Result, expected: &str) { match result { Err(QbftError::SanityCheck(msg)) => assert_eq!(msg, expected), @@ -2586,7 +2584,6 @@ fn new_nested_msg(type_: MessageType, source: i64, round: i64) -> Msg }) } -/// Former `mod.rs:398`. #[test] fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { let cts = CancellationTokenSource::new(); @@ -2623,117 +2620,6 @@ fn run_reports_sanity_check_when_pre_prepare_justification_is_already_cached() { assert_sanity_check(outcome, "bug: justification cache must be nil"); } -/// Former `mod.rs:902`. -#[test] -fn classify_reports_sanity_check_for_unknown_message_type() { - let mut def = noop_definition(); - def.nodes = 4; - - let msg = new_msg(MessageType::Unknown, 0, 1, 1, 1, 0, 0, 0, None); - - assert_sanity_check( - classify(&def, &0, 1, 2, &HashMap::new(), &msg), - "bug: invalid type", - ); -} - -/// Former `mod.rs:913`. -#[test] -fn next_min_round_reports_sanity_check_for_short_justification() { - let mut def = noop_definition(); - def.nodes = 4; // f+1 == 2 - - let frc = vec![new_round_change(1, 3, 0, 0)]; - - assert_sanity_check(next_min_round(&def, &frc, 1), "bug: Frc too short"); -} - -/// Former `mod.rs:921`. -#[test] -fn next_min_round_reports_sanity_check_for_non_round_change() { - let mut def = noop_definition(); - def.nodes = 4; - - let frc = vec![ - new_msg(MSG_PREPARE, 0, 1, 3, 1, 0, 0, 0, None), - new_round_change(2, 3, 0, 0), - ]; - - assert_sanity_check( - next_min_round(&def, &frc, 1), - "bug: Frc contain non-round change", - ); -} - -/// Former `mod.rs:923`. -#[test] -fn next_min_round_reports_sanity_check_for_non_future_round() { - let mut def = noop_definition(); - def.nodes = 4; - - let frc = vec![new_round_change(1, 1, 0, 0), new_round_change(2, 1, 0, 0)]; - - assert_sanity_check( - next_min_round(&def, &frc, 1), - "bug: Frc round not in future", - ); -} - -/// Former `mod.rs:947`. -#[test] -fn is_justified_reports_sanity_check_for_unknown_message_type() { - let mut def = noop_definition(); - def.nodes = 4; - - let msg = new_msg(MessageType::Unknown, 0, 1, 1, 1, 0, 0, 0, None); - - assert_sanity_check(is_justified(&def, &0, &msg, 0), "bug: invalid message type"); -} - -/// Former `mod.rs:955`. -#[test] -fn is_justified_round_change_reports_sanity_check_for_other_types() { - let mut def = noop_definition(); - def.nodes = 4; - - let msg = new_msg(MSG_PREPARE, 0, 1, 1, 1, 0, 0, 0, None); - - assert_sanity_check( - is_justified_round_change(&def, &msg), - "bug: not a round change message", - ); -} - -/// Former `mod.rs:1024`. -#[test] -fn is_justified_decided_reports_sanity_check_for_other_types() { - let mut def = noop_definition(); - def.nodes = 4; - - let msg = new_msg(MSG_COMMIT, 0, 1, 1, 1, 0, 0, 0, None); - - assert_sanity_check( - is_justified_decided(&def, &msg), - "bug: not a decided message", - ); -} - -/// Former `mod.rs:1048`. -#[test] -fn is_justified_pre_prepare_reports_sanity_check_for_other_types() { - let mut def = noop_definition(); - def.nodes = 4; - def.is_leader = Box::new(make_is_leader(4)); - - let msg = new_msg(MSG_ROUND_CHANGE, 0, 1, 1, 1, 0, 0, 0, None); - - assert_sanity_check( - is_justified_pre_prepare(&def, &0, &msg, 0), - "bug: not a preprepare message", - ); -} - -/// Former `mod.rs:1397`. #[test] fn flatten_reports_sanity_check_for_nested_justifications() { let nested = new_nested_msg(MSG_ROUND_CHANGE, 1, 1); From d2c84566c6de0a98e9bed73e9cf5913c66cf79df Mon Sep 17 00:00:00 2001 From: Quang Le Date: Fri, 4 Sep 2026 10:33:00 +0700 Subject: [PATCH 4/4] fix: remove comments --- crates/core/src/qbft/internal_test.rs | 6 ------ crates/core/src/qbft/mod.rs | 5 ----- 2 files changed, 11 deletions(-) diff --git a/crates/core/src/qbft/internal_test.rs b/crates/core/src/qbft/internal_test.rs index 6527dad2..88f75ead 100644 --- a/crates/core/src/qbft/internal_test.rs +++ b/crates/core/src/qbft/internal_test.rs @@ -46,12 +46,6 @@ const TEST_WAIT_TIMEOUT: Duration = Duration::from_secs(1); // protocol progress, so slow-but-progressing parallel runs should not fail. const TEST_STALL_TIMEOUT: Duration = Duration::from_secs(20); -/// What one `qbft::run` thread reported back. -/// -/// `run` no longer panics on an internal sanity-check failure — it returns -/// [`QbftError::SanityCheck`] — so the harness collects a plain [`Result`] -/// instead of catching unwinds. Charon's `qbft.Run` behaves the same way after -/// its `recover` in `core/qbft/qbft.go:187-198`. type RunOutcome = Result<()>; type TestMsgRef = Msg; diff --git a/crates/core/src/qbft/mod.rs b/crates/core/src/qbft/mod.rs index 4f81114a..84cadccb 100644 --- a/crates/core/src/qbft/mod.rs +++ b/crates/core/src/qbft/mod.rs @@ -64,11 +64,6 @@ pub enum QbftError { UnexpectedCompareError(Box), /// An internal sanity check failed. - /// - /// Mirrors charon `core/qbft/qbft.go:187-198`, which recovers exactly the - /// panics whose message contains "bug" and turns them into - /// `fmt.Errorf("qbft sanity check: %v", r)`. The payload keeps charon's - /// original panic string so log output stays greppable. #[error("qbft sanity check: {0}")] SanityCheck(&'static str),