fix: address consensus review feedback

crates/consensus/Cargo.toml

@@ -26,6 +26,7 @@ malachite = [
 [dependencies]
 anyhow = { workspace = true }
 async-trait = { workspace = true }
+thiserror = { workspace = true }
 tracing = { workspace = true }
 hex = { workspace = true }
 

crates/consensus/src/config.rs

@@ -47,3 +47,36 @@ impl ConsensusConfig {
         &self.chain_id
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_new_with_defaults() {
+        let config = ConsensusConfig::new("test-chain");
+        assert_eq!(config.chain_id(), "test-chain");
+        assert_eq!(config.propose_timeout, Duration::from_secs(1));
+        assert_eq!(config.prevote_timeout, Duration::from_secs(1));
+        assert_eq!(config.precommit_timeout, Duration::from_secs(1));
+    }
+
+    #[test]
+    fn test_builder_pattern() {
+        let config = ConsensusConfig::new("my-chain")
+            .with_propose_timeout(Duration::from_millis(500))
+            .with_prevote_timeout(Duration::from_millis(300))
+            .with_precommit_timeout(Duration::from_millis(200));
+
+        assert_eq!(config.chain_id(), "my-chain");
+        assert_eq!(config.propose_timeout, Duration::from_millis(500));
+        assert_eq!(config.prevote_timeout, Duration::from_millis(300));
+        assert_eq!(config.precommit_timeout, Duration::from_millis(200));
+    }
+
+    #[test]
+    fn test_chain_id_from_string() {
+        let config = ConsensusConfig::new(String::from("dynamic-chain"));
+        assert_eq!(config.chain_id(), "dynamic-chain");
+    }
+}

crates/consensus/src/context.rs

@@ -3,6 +3,7 @@ use informalsystems_malachitebft_core_types::{
 };
 
 use crate::config::ConsensusConfig;
+use crate::error::ConsensusError;
 use crate::proposal::{CutProposal, CutProposalPart};
 use crate::signing::Ed25519SigningScheme;
 use crate::types::{ConsensusHeight, ConsensusValue};
@@ -34,17 +35,36 @@ pub struct CipherBftContext {
 }
 
 impl CipherBftContext {
-    /// Create a new context.
-    pub fn new(
+    /// Create a new context with validation.
+    ///
+    /// # Errors
+    /// Returns `ConsensusError::EmptyValidatorSet` if the validator set is empty.
+    pub fn try_new(
         config: ConsensusConfig,
         validator_set: ConsensusValidatorSet,
         initial_height: ConsensusHeight,
-    ) -> Self {
-        Self {
+    ) -> Result<Self, ConsensusError> {
+        if validator_set.is_empty() {
+            return Err(ConsensusError::EmptyValidatorSet);
+        }
+        Ok(Self {
             config,
             validator_set,
             initial_height,
-        }
+        })
+    }
+
+    /// Create a new context.
+    ///
+    /// # Panics
+    /// Panics if the validator set is empty. Use `try_new` for fallible construction.
+    pub fn new(
+        config: ConsensusConfig,
+        validator_set: ConsensusValidatorSet,
+        initial_height: ConsensusHeight,
+    ) -> Self {
+        Self::try_new(config, validator_set, initial_height)
+            .expect("validator set must not be empty")
     }
 
     /// Access the initial height.

crates/consensus/src/engine.rs

@@ -17,11 +17,11 @@
 
 use anyhow::Result;
 use informalsystems_malachitebft_config::{
-    ConsensusConfig as EngineConsensusConfig, ValuePayload as EngineValuePayload,
+    ConsensusConfig as EngineConsensusConfig, TimeoutConfig, ValuePayload as EngineValuePayload,
 };
 use informalsystems_malachitebft_core_consensus::Params as ConsensusParams;
-use informalsystems_malachitebft_core_types::ValuePayload;
 use informalsystems_malachitebft_core_driver::ThresholdParams;
+use informalsystems_malachitebft_core_types::ValuePayload;
 use informalsystems_malachitebft_app::types::core::SigningProvider;
 use informalsystems_malachitebft_engine::consensus::{Consensus, ConsensusRef};
 use informalsystems_malachitebft_engine::host::HostRef;
@@ -35,6 +35,7 @@ use tracing::info_span;
 
 use crate::config::ConsensusConfig;
 use crate::context::CipherBftContext;
+use crate::error::ConsensusError;
 use crate::types::ConsensusHeight;
 use crate::validator_set::{ConsensusAddress, ConsensusValidator, ConsensusValidatorSet};
 
@@ -56,6 +57,9 @@ use crate::validator_set::{ConsensusAddress, ConsensusValidator, ConsensusValida
 /// * `validators` - List of validators with Ed25519 public keys and voting power
 /// * `initial_height` - Starting height for consensus (defaults to 1 if None)
 ///
+/// # Errors
+/// Returns `ConsensusError::EmptyValidatorSet` if the validator list is empty.
+///
 /// # Example
 /// ```rust,ignore
 /// use cipherbft_consensus::{create_context, ConsensusValidator};
@@ -69,18 +73,18 @@ use crate::validator_set::{ConsensusAddress, ConsensusValidator, ConsensusValida
 ///         100, // voting power
 ///     ),
 /// ];
-/// let ctx = create_context("my-chain", validators, None);
+/// let ctx = create_context("my-chain", validators, None)?;
 /// ```
 pub fn create_context(
     chain_id: impl Into<String>,
     validators: Vec<ConsensusValidator>,
     initial_height: Option<ConsensusHeight>,
-) -> CipherBftContext {
+) -> Result<CipherBftContext, ConsensusError> {
     let config = ConsensusConfig::new(chain_id);
     let validator_set = ConsensusValidatorSet::new(validators);
     let initial_height = initial_height.unwrap_or_else(|| ConsensusHeight::from(1));
 
-    CipherBftContext::new(config, validator_set, initial_height)
+    CipherBftContext::try_new(config, validator_set, initial_height)
 }
 
 /// Create Malachite consensus params using our Context components.
@@ -107,6 +111,24 @@ pub fn default_engine_config_single_part() -> EngineConsensusConfig {
     cfg
 }
 
+/// Create engine config with timeouts from `ConsensusConfig`.
+///
+/// This wires the CipherBFT consensus timeouts into the Malachite engine config.
+pub fn create_engine_config(config: &ConsensusConfig) -> EngineConsensusConfig {
+    let timeout_config = TimeoutConfig {
+        propose_timeout: config.propose_timeout,
+        prevote_timeout: config.prevote_timeout,
+        precommit_timeout: config.precommit_timeout,
+        ..Default::default()
+    };
+
+    EngineConsensusConfig {
+        value_payload: EngineValuePayload::ProposalOnly,
+        timeouts: timeout_config,
+        ..Default::default()
+    }
+}
+
 /// Bundles all actor handles returned after spawning.
 pub struct EngineHandles {
     pub node: NodeRef,

crates/consensus/src/error.rs

@@ -0,0 +1,46 @@
+//! Consensus layer error types
+
+use thiserror::Error;
+
+/// Errors that can occur during consensus operations.
+#[derive(Debug, Error, Clone, PartialEq, Eq)]
+pub enum ConsensusError {
+    /// Validator set cannot be empty for consensus operations.
+    #[error("validator set cannot be empty")]
+    EmptyValidatorSet,
+
+    /// Invalid configuration provided.
+    #[error("invalid configuration: {0}")]
+    InvalidConfig(String),
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_empty_validator_set_error_display() {
+        let err = ConsensusError::EmptyValidatorSet;
+        assert_eq!(err.to_string(), "validator set cannot be empty");
+    }
+
+    #[test]
+    fn test_invalid_config_error_display() {
+        let err = ConsensusError::InvalidConfig("bad timeout".to_string());
+        assert_eq!(err.to_string(), "invalid configuration: bad timeout");
+    }
+
+    #[test]
+    fn test_error_equality() {
+        let err1 = ConsensusError::EmptyValidatorSet;
+        let err2 = ConsensusError::EmptyValidatorSet;
+        assert_eq!(err1, err2);
+
+        let err3 = ConsensusError::InvalidConfig("a".to_string());
+        let err4 = ConsensusError::InvalidConfig("a".to_string());
+        assert_eq!(err3, err4);
+
+        let err5 = ConsensusError::InvalidConfig("b".to_string());
+        assert_ne!(err3, err5);
+    }
+}

crates/consensus/src/lib.rs

@@ -5,8 +5,11 @@
 //! existing builds remain unaffected while we wire up the consensus layer.
 
 pub mod config;
+pub mod error;
 pub mod types;
 
+pub use error::ConsensusError;
+
 #[cfg(feature = "malachite")]
 pub mod context;
 #[cfg(feature = "malachite")]
@@ -35,6 +38,6 @@ pub use validator_set::ConsensusValidatorSet;
 pub use vote::ConsensusVote;
 #[cfg(feature = "malachite")]
 pub use engine::{
-    create_context, default_consensus_params, default_engine_config_single_part, EngineHandles,
-    MalachiteEngineBuilder,
+    create_context, create_engine_config, default_consensus_params,
+    default_engine_config_single_part, EngineHandles, MalachiteEngineBuilder,
 };

crates/consensus/src/proposal.rs

@@ -94,6 +94,8 @@ impl MalachiteProposalPart<CipherBftContext> for CutProposalPart {
 impl PartialEq for CutProposalPart {
     fn eq(&self, other: &Self) -> bool {
         self.cut.hash() == other.cut.hash()
+            && self.first == other.first
+            && self.last == other.last
     }
 }
 

crates/consensus/src/types.rs

@@ -133,3 +133,87 @@ pub use malachite_impls::ConsensusRound;
 #[cfg(not(feature = "malachite"))]
 /// Placeholder round type when Malachite is disabled.
 pub type ConsensusRound = i64;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_consensus_height_next() {
+        let h = ConsensusHeight(5);
+        assert_eq!(h.next(), ConsensusHeight(6));
+    }
+
+    #[test]
+    fn test_consensus_height_from_u64() {
+        let h: ConsensusHeight = 42u64.into();
+        assert_eq!(h.0, 42);
+    }
+
+    #[test]
+    fn test_consensus_height_into_u64() {
+        let h = ConsensusHeight(100);
+        let n: u64 = h.into();
+        assert_eq!(n, 100);
+    }
+
+    #[test]
+    fn test_consensus_height_ordering() {
+        let h1 = ConsensusHeight(1);
+        let h2 = ConsensusHeight(2);
+        assert!(h1 < h2);
+        assert!(h2 > h1);
+        assert_eq!(h1, ConsensusHeight(1));
+    }
+
+    #[test]
+    fn test_consensus_height_display() {
+        let h = ConsensusHeight(123);
+        assert_eq!(format!("{}", h), "123");
+    }
+
+    #[test]
+    fn test_consensus_value_id_display() {
+        let hash = Hash::compute(b"test");
+        let id = ConsensusValueId(hash);
+        // Display should delegate to Hash's Display
+        assert!(!format!("{}", id).is_empty());
+    }
+
+    #[test]
+    fn test_consensus_value_equality_by_hash() {
+        let cut1 = Cut::new(1);
+        let cut2 = Cut::new(1);
+        let cut3 = Cut::new(2);
+
+        let v1 = ConsensusValue(cut1);
+        let v2 = ConsensusValue(cut2);
+        let v3 = ConsensusValue(cut3);
+
+        // Same height empty cuts should have same hash
+        assert_eq!(v1, v2);
+        // Different height should differ
+        assert_ne!(v1, v3);
+    }
+
+    #[test]
+    fn test_consensus_value_ordering() {
+        let cut1 = Cut::new(1);
+        let cut2 = Cut::new(2);
+
+        let v1 = ConsensusValue(cut1);
+        let v2 = ConsensusValue(cut2);
+
+        // Ordering should be deterministic by hash
+        assert!(v1.cmp(&v2) != Ordering::Equal || v1 == v2);
+    }
+
+    #[test]
+    fn test_consensus_value_into_cut() {
+        let cut = Cut::new(42);
+        let height = cut.height;
+        let value = ConsensusValue(cut);
+        let recovered = value.into_cut();
+        assert_eq!(recovered.height, height);
+    }
+}

crates/consensus/src/validator_set.rs

@@ -82,6 +82,11 @@ impl ConsensusValidatorSet {
     pub fn len(&self) -> usize {
         self.validators.len()
     }
+
+    /// Check if the validator set is empty.
+    pub fn is_empty(&self) -> bool {
+        self.validators.is_empty()
+    }
 }
 
 #[cfg(feature = "malachite")]