fix

node/components/bft/src/chonky_bft/mod.rs

@@ -1,6 +1,5 @@
 use crate::{metrics, Config, FromNetworkMessage, ToNetworkMessage};
 use std::{
-    cmp::max,
     collections::{BTreeMap, HashMap},
     sync::Arc,
 };
@@ -276,7 +275,14 @@ impl StateMachine {
         ctx: &ctx::Ctx,
         qc: &validator::CommitQC,
     ) -> ctx::Result<()> {
-        self.high_commit_qc = max(Some(qc.clone()), self.high_commit_qc.clone());
+        if self
+            .high_commit_qc
+            .as_ref()
+            .map_or(false, |old| old.view().number >= qc.view().number)
+        {
+            return Ok(());
+        }
+        self.high_commit_qc = Some(qc.clone());
         self.save_block(ctx, qc).await.wrap("save_block()")
     }
 }

node/components/bft/src/chonky_bft/new_view.rs

@@ -1,5 +1,3 @@
-use std::cmp::max;
-
 use super::StateMachine;
 use crate::{chonky_bft::VIEW_TIMEOUT_DURATION, metrics};
 use zksync_concurrency::{ctx, error::Wrap, metrics::LatencyHistogramExt as _, time};
@@ -94,7 +92,13 @@ impl StateMachine {
                         .await
                         .wrap("process_commit_qc()")?;
                 }
-                self.high_timeout_qc = max(Some(qc.clone()), self.high_timeout_qc.clone());
+                if self
+                    .high_timeout_qc
+                    .as_ref()
+                    .map_or(true, |old| old.view.number < qc.view.number)
+                {
+                    self.high_timeout_qc = Some(qc.clone());
+                }
             }
         };
 

node/components/bft/src/chonky_bft/proposal.rs

@@ -1,7 +1,5 @@
-use crate::metrics;
-
 use super::StateMachine;
-use std::cmp::max;
+use crate::metrics;
 use zksync_concurrency::{ctx, error::Wrap, metrics::LatencyHistogramExt as _};
 use zksync_consensus_network::io::ConsensusInputMessage;
 use zksync_consensus_roles::validator::{self, BlockHeader, BlockNumber};
@@ -223,6 +221,7 @@ impl StateMachine {
 
         // Update the state machine.
         self.view_number = message.view().number;
+        metrics::METRICS.replica_view_number.set(self.view_number.0);
         self.phase = validator::Phase::Commit;
         self.high_vote = Some(commit_vote.clone());
         match &message.justification {
@@ -236,7 +235,13 @@ impl StateMachine {
                         .await
                         .wrap("process_commit_qc()")?;
                 }
-                self.high_timeout_qc = max(Some(qc.clone()), self.high_timeout_qc.clone());
+                if self
+                    .high_timeout_qc
+                    .as_ref()
+                    .map_or(true, |old| old.view.number < qc.view.number)
+                {
+                    self.high_timeout_qc = Some(qc.clone());
+                }
             }
         };
 

node/components/bft/src/chonky_bft/proposer.rs

@@ -1,11 +1,10 @@
+use super::VIEW_TIMEOUT_DURATION;
 use crate::{metrics, Config, ToNetworkMessage};
 use std::sync::Arc;
 use zksync_concurrency::{ctx, error::Wrap as _, sync};
 use zksync_consensus_network::io::ConsensusInputMessage;
 use zksync_consensus_roles::validator;
 
-use super::VIEW_TIMEOUT_DURATION;
-
 /// The proposer loop is responsible for proposing new blocks to the network. It watches for new
 /// justifications from the replica and if it is the leader for the view, it proposes a new block.
 pub(crate) async fn run_proposer(

node/components/bft/src/chonky_bft/testonly.rs

@@ -1,13 +1,12 @@
-use crate::testonly::RandomPayload;
 use crate::{
     chonky_bft::{self, commit, new_view, proposal, timeout, StateMachine},
-    Config, PayloadManager,
+    create_input_channel,
+    testonly::RandomPayload,
+    Config, FromNetworkMessage, PayloadManager, ToNetworkMessage,
 };
-use crate::{create_input_channel, FromNetworkMessage, ToNetworkMessage};
 use assert_matches::assert_matches;
 use std::sync::Arc;
-use zksync_concurrency::sync::prunable_mpsc;
-use zksync_concurrency::{ctx, sync};
+use zksync_concurrency::{ctx, sync, sync::prunable_mpsc};
 use zksync_consensus_roles::validator;
 use zksync_consensus_storage::{
     testonly::{in_memory, TestMemoryStorage},

node/components/bft/src/chonky_bft/tests/timeout.rs

@@ -1,9 +1,52 @@
 use crate::chonky_bft::{testonly::UTHarness, timeout};
 use assert_matches::assert_matches;
-use rand::Rng;
+use rand::{seq::SliceRandom as _, Rng};
 use zksync_concurrency::{ctx, scope};
 use zksync_consensus_roles::validator;
 
+#[test]
+fn timeout_qc_aggregation() {
+    zksync_concurrency::testonly::abort_on_panic();
+    let ctx = &ctx::test_root(&ctx::RealClock);
+    let rng = &mut ctx.rng();
+    let setup = validator::testonly::Setup::new(rng, 10);
+    let view = validator::View {
+        number: rng.gen(),
+        genesis: setup.genesis.hash(),
+    };
+    let commit = validator::ReplicaCommit {
+        view,
+        proposal: validator::BlockHeader {
+            number: rng.gen(),
+            payload: rng.gen(),
+        },
+    };
+    let mut timeout_qc = validator::TimeoutQC::new(view);
+    for k in &setup.validator_keys {
+        // Generate ReplicaTimeout which differ just by the high_qc signer set.
+        let mut commit_qc = validator::CommitQC::new(commit.clone(), &setup.genesis);
+        // Add signatures in random order until the CommitQC is valid.
+        let mut keys = setup.validator_keys.clone();
+        keys.shuffle(rng);
+        for k in &keys {
+            if commit_qc.verify(&setup.genesis).is_ok() {
+                break;
+            }
+            commit_qc
+                .add(&k.sign_msg(commit.clone()), &setup.genesis)
+                .unwrap();
+        }
+        // Add vote to the TimeoutQC.
+        let vote = validator::ReplicaTimeout {
+            view,
+            high_vote: None,
+            high_qc: Some(commit_qc.clone()),
+        };
+        timeout_qc.add(&k.sign_msg(vote), &setup.genesis).unwrap();
+    }
+    timeout_qc.verify(&setup.genesis).unwrap();
+}
+
 #[tokio::test]
 async fn timeout_yield_new_view_sanity() {
     zksync_concurrency::testonly::abort_on_panic();

node/components/bft/src/chonky_bft/timeout.rs

@@ -1,6 +1,5 @@
 use super::StateMachine;
-use crate::metrics;
-use std::{cmp::max, collections::HashSet};
+use std::collections::HashSet;
 use zksync_concurrency::{ctx, error::Wrap, time};
 use zksync_consensus_network::io::ConsensusInputMessage;
 use zksync_consensus_roles::validator;
@@ -142,7 +141,13 @@ impl StateMachine {
                 .await
                 .wrap("process_commit_qc()")?;
         }
-        self.high_timeout_qc = max(Some(timeout_qc.clone()), self.high_timeout_qc.clone());
+        if self
+            .high_timeout_qc
+            .as_ref()
+            .map_or(true, |old| old.view.number < timeout_qc.view.number)
+        {
+            self.high_timeout_qc = Some(timeout_qc.clone());
+        }
 
         // Start a new view.
         self.start_new_view(ctx, message.view.number.next()).await?;
@@ -180,7 +185,6 @@ impl StateMachine {
 
         // Log the event.
         tracing::info!("Timed out at view {}", self.view_number);
-        metrics::METRICS.replica_view_number.set(self.view_number.0);
 
         Ok(())
     }

node/components/bft/src/metrics.rs

@@ -54,7 +54,6 @@ pub(crate) struct ProcessingLatencyLabels {
 #[metrics(prefix = "consensus")]
 pub(crate) struct ConsensusMetrics {
     /// Number of the current view of the replica.
-    #[metrics(unit = Unit::Seconds)]
     pub(crate) replica_view_number: Gauge<u64>,
     /// Number of the last finalized block observed by the node.
     pub(crate) finalized_block_number: Gauge<u64>,

node/components/bft/src/testonly/node.rs

@@ -1,8 +1,7 @@
 use crate::{testonly, FromNetworkMessage, PayloadManager, ToNetworkMessage};
 use anyhow::Context as _;
 use std::sync::Arc;
-use zksync_concurrency::ctx::channel;
-use zksync_concurrency::{ctx, scope, sync};
+use zksync_concurrency::{ctx, ctx::channel, scope, sync};
 use zksync_consensus_network as network;
 use zksync_consensus_storage as storage;
 use zksync_consensus_storage::testonly::in_memory;

node/components/network/src/testonly.rs

@@ -1,8 +1,9 @@
 //! Testonly utilities.
 #![allow(dead_code)]
 use crate::{
-    gossip::attestation, io::ConsensusInputMessage, io::ConsensusReq, Config, GossipConfig,
-    Network, RpcConfig, Runner,
+    gossip::attestation,
+    io::{ConsensusInputMessage, ConsensusReq},
+    Config, GossipConfig, Network, RpcConfig, Runner,
 };
 use rand::{
     distributions::{Distribution, Standard},

node/libs/roles/src/validator/messages/replica_commit.rs

@@ -31,7 +31,7 @@ pub enum ReplicaCommitVerifyError {
 
 /// A Commit Quorum Certificate. It is an aggregate of signed ReplicaCommit messages.
 /// The Commit Quorum Certificate is over identical messages, so we only need one message.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub struct CommitQC {
     /// The ReplicaCommit message that the QC is for.
     pub message: ReplicaCommit,
@@ -137,18 +137,6 @@ impl CommitQC {
     }
 }
 
-impl Ord for CommitQC {
-    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-        self.message.view.number.cmp(&other.message.view.number)
-    }
-}
-
-impl PartialOrd for CommitQC {
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 /// Error returned by `CommitQC::add()`.
 #[derive(thiserror::Error, Debug)]
 pub enum CommitQCAddError {

node/libs/roles/src/validator/messages/tests/versions.rs

@@ -3,9 +3,8 @@ use anyhow::Context as _;
 use zksync_consensus_crypto::Text;
 
 mod version1 {
-    use zksync_consensus_utils::enum_util::Variant as _;
-
     use super::*;
+    use zksync_consensus_utils::enum_util::Variant as _;
 
     /// Note that genesis is NOT versioned by ProtocolVersion.
     /// Even if it was, ALL versions of genesis need to be supported FOREVER,