From fb89af0ecfcb5d3a4221f55fdc79dfef5e900f3c Mon Sep 17 00:00:00 2001 From: diego Date: Thu, 28 Aug 2025 20:43:40 +0200 Subject: [PATCH 1/4] test: add test for commit message handling without proposal acceptance Add failing test that demonstrates QBFT incorrectly drops commit messages when no proposal has been accepted for the current round. This prevents proper catch-up scenarios where nodes should be able to achieve consensus based on commit quorum even without seeing the original proposal. The test shows that commit messages are dropped in the commit message handling logic, which should buffer these messages instead for later processing when a proposal arrives. --- anchor/common/qbft/src/tests.rs | 125 ++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/anchor/common/qbft/src/tests.rs b/anchor/common/qbft/src/tests.rs index a607b5c9b..fb3212b75 100644 --- a/anchor/common/qbft/src/tests.rs +++ b/anchor/common/qbft/src/tests.rs @@ -223,3 +223,128 @@ fn test_node_recovery() { let num_consensus = test_instance.wait_until_end(); assert_eq!(num_consensus, 5); // Should reach full consensus after recovery } + +#[test] +/// Test that demonstrates QBFT incorrectly drops commit messages when no proposal accepted +/// +/// In a proper QBFT implementation, commit messages should be buffered when they arrive +/// before a proposal, allowing catch-up scenarios where a node can achieve consensus +/// based on a commit quorum even without seeing the original proposal. +/// +/// Current bug: Individual commit messages are dropped when +/// proposal_accepted_for_current_round is false, preventing nodes from ever +/// reaching commit quorum in catch-up scenarios. +fn test_commit_messages_dropped_without_proposal_acceptance() { + if ENABLE_TEST_LOGGING { + let env_filter = EnvFilter::new("debug"); + let _ = tracing_subscriber::fmt() + .compact() + .with_env_filter(env_filter) + .try_init(); + } + + use ssv_types::{ + consensus::QbftMessage, + message::{MsgType, RSA_SIGNATURE_SIZE, SSVMessage, SignedSSVMessage}, + }; + + // Create QBFT instance with 3 nodes (f=0, quorum=3) + let config = ConfigBuilder::::new( + 1.into(), + InstanceHeight::default(), + (1..4).map(OperatorId::from).collect(), // 3 nodes, quorum = 3 + ) + .with_operator_id(OperatorId::from(1)) + .build() + .expect("config should be valid"); + + let test_data = TestData(789); + let mut qbft_instance = Qbft::new( + config, + test_data.clone(), + Box::new(NoDataValidation), + MessageId::from([0; 56]), + |_| {}, + ); + + // Verify initial state: no proposal accepted + assert!(!qbft_instance.proposal_accepted_for_current_round); + assert!(matches!( + qbft_instance.state, + InstanceState::AwaitingProposal + )); + + // STEP 1: Send commit messages BEFORE accepting any proposal (catch-up scenario) + // This simulates a node that missed the proposal but receives commit messages from other nodes + + let _commit_messages_before = qbft_instance + .commit_container + .get_messages_for_round(1.into()) + .len(); + + // Create 3 valid commit messages for the same data + for operator_id in [1, 2, 3] { + let commit_msg = QbftMessage { + qbft_message_type: QbftMessageType::Commit, + height: 0, + round: 1, + identifier: [0; 56].to_vec().into(), + root: test_data.hash(), + data_round: 0, + round_change_justification: vec![], + prepare_justification: vec![], + }; + + let commit_ssv_message = SSVMessage::new( + MsgType::SSVConsensusMsgType, + MessageId::from([0; 56]), + commit_msg.as_ssz_bytes(), + ) + .expect("should create commit SSVMessage"); + + let signed_commit = SignedSSVMessage::new( + vec![vec![0; RSA_SIGNATURE_SIZE]], + vec![OperatorId::from(operator_id)], + commit_ssv_message, + vec![], // no full_data for commit + ) + .expect("should create signed commit"); + + let wrapped_commit = WrappedQbftMessage { + signed_message: signed_commit, + qbft_message: commit_msg, + }; + + // Send the commit message - this should be buffered, not dropped + qbft_instance.receive(wrapped_commit); + } + + let commit_messages_after = qbft_instance + .commit_container + .get_messages_for_round(1.into()) + .len(); + + // The commit messages should be buffered for catch-up scenario processing + // This assertion FAILS due to the bug in commit message handling + assert_eq!( + commit_messages_after, 3, + "BUG: Commit messages should be buffered when no proposal accepted for catch-up scenarios. \ + Expected: 3 commit messages buffered. Actual: {} messages. \ + Commit messages are dropped when proposal_accepted_for_current_round \ + is false, preventing nodes from achieving consensus in catch-up scenarios where they \ + receive commits before proposals.", + commit_messages_after + ); + + // Instance should still be in AwaitingProposal state since commits were dropped + assert!( + matches!(qbft_instance.state, InstanceState::AwaitingProposal), + "Instance should remain in AwaitingProposal state since commits were incorrectly dropped" + ); + + // The instance should NOT have reached consensus due to dropped commits + assert!( + qbft_instance.completed.is_none(), + "Instance should not have completed consensus due to dropped commit messages" + ); +} From 45cbdaaa33cc9dcf91673dbcab9d9b01956117c4 Mon Sep 17 00:00:00 2001 From: diego Date: Fri, 29 Aug 2025 11:04:44 +0200 Subject: [PATCH 2/4] test: replace buffering test with comprehensive consensus test Replace the incomplete buffering test with a comprehensive test that verifies complete QBFT consensus behavior in out-of-order scenarios. The new test_consensus_with_commits_before_proposal() test: - Sends commit messages before proposal (realistic catch-up scenario) - Verifies actual consensus achievement with correct value - Tests complete distributed systems behavior rather than just buffering This provides better validation of QBFT message ordering independence and ensures the fix addresses the complete consensus protocol behavior. --- anchor/common/qbft/src/tests.rs | 136 ++++++++++++++++++++++---------- 1 file changed, 96 insertions(+), 40 deletions(-) diff --git a/anchor/common/qbft/src/tests.rs b/anchor/common/qbft/src/tests.rs index fb3212b75..61c580457 100644 --- a/anchor/common/qbft/src/tests.rs +++ b/anchor/common/qbft/src/tests.rs @@ -225,16 +225,19 @@ fn test_node_recovery() { } #[test] -/// Test that demonstrates QBFT incorrectly drops commit messages when no proposal accepted +/// Test that verifies QBFT can achieve consensus when commit messages arrive before proposal /// -/// In a proper QBFT implementation, commit messages should be buffered when they arrive -/// before a proposal, allowing catch-up scenarios where a node can achieve consensus -/// based on a commit quorum even without seeing the original proposal. +/// This test simulates a realistic catch-up scenario in distributed systems where a node +/// receives commit messages from other nodes before receiving the original proposal. +/// According to QBFT specification, if we have: +/// 1. A valid proposal for value X +/// 2. A quorum of commit messages for value X /// -/// Current bug: Individual commit messages are dropped when -/// proposal_accepted_for_current_round is false, preventing nodes from ever -/// reaching commit quorum in catch-up scenarios. -fn test_commit_messages_dropped_without_proposal_acceptance() { +/// Then we should achieve consensus on value X, regardless of message arrival order. +/// +/// Current bug: This test FAILS because commit messages are dropped instead of buffered, +/// preventing consensus achievement in out-of-order scenarios. +fn test_consensus_with_commits_before_proposal() { if ENABLE_TEST_LOGGING { let env_filter = EnvFilter::new("debug"); let _ = tracing_subscriber::fmt() @@ -267,23 +270,20 @@ fn test_commit_messages_dropped_without_proposal_acceptance() { |_| {}, ); - // Verify initial state: no proposal accepted + // Verify initial state: no proposal accepted, no consensus assert!(!qbft_instance.proposal_accepted_for_current_round); assert!(matches!( qbft_instance.state, InstanceState::AwaitingProposal )); + assert!(qbft_instance.completed.is_none()); - // STEP 1: Send commit messages BEFORE accepting any proposal (catch-up scenario) - // This simulates a node that missed the proposal but receives commit messages from other nodes - - let _commit_messages_before = qbft_instance - .commit_container - .get_messages_for_round(1.into()) - .len(); + // STEP 1: Send commit messages FIRST (out-of-order scenario) + // This simulates receiving commits from other nodes before seeing the proposal + println!("Sending commit messages before proposal..."); - // Create 3 valid commit messages for the same data - for operator_id in [1, 2, 3] { + for operator_id in [2, 3, 4] { + // From operators 2, 3, 4 (not from leader 1) let commit_msg = QbftMessage { qbft_message_type: QbftMessageType::Commit, height: 0, @@ -315,36 +315,92 @@ fn test_commit_messages_dropped_without_proposal_acceptance() { qbft_message: commit_msg, }; - // Send the commit message - this should be buffered, not dropped + // Send the commit message - should be buffered for later processing qbft_instance.receive(wrapped_commit); } - let commit_messages_after = qbft_instance - .commit_container - .get_messages_for_round(1.into()) - .len(); - - // The commit messages should be buffered for catch-up scenario processing - // This assertion FAILS due to the bug in commit message handling - assert_eq!( - commit_messages_after, 3, - "BUG: Commit messages should be buffered when no proposal accepted for catch-up scenarios. \ - Expected: 3 commit messages buffered. Actual: {} messages. \ - Commit messages are dropped when proposal_accepted_for_current_round \ - is false, preventing nodes from achieving consensus in catch-up scenarios where they \ - receive commits before proposals.", - commit_messages_after + // After commits, should still be awaiting proposal (no consensus yet) + assert!(matches!( + qbft_instance.state, + InstanceState::AwaitingProposal + )); + assert!(qbft_instance.completed.is_none()); + + // STEP 2: Now send the proposal (completing the consensus scenario) + println!("Sending proposal after commits..."); + + let proposal = QbftMessage { + qbft_message_type: QbftMessageType::Proposal, + height: 0, + round: 1, + identifier: [0; 56].to_vec().into(), + root: test_data.hash(), + data_round: 0, + round_change_justification: vec![], + prepare_justification: vec![], + }; + + let proposal_ssv_message = SSVMessage::new( + MsgType::SSVConsensusMsgType, + MessageId::from([0; 56]), + proposal.as_ssz_bytes(), + ) + .expect("should create proposal SSVMessage"); + + let signed_proposal = SignedSSVMessage::new( + vec![vec![0; RSA_SIGNATURE_SIZE]], + vec![OperatorId::from(1)], // From leader (operator 1) + proposal_ssv_message, + test_data.as_ssz_bytes(), // full_data for proposal + ) + .expect("should create signed proposal"); + + let wrapped_proposal = WrappedQbftMessage { + signed_message: signed_proposal, + qbft_message: proposal, + }; + + // Send the proposal - this should trigger re-evaluation of buffered commits + qbft_instance.receive(wrapped_proposal); + + // STEP 3: Verify consensus is achieved with correct value + // After receiving the proposal, the instance should: + // 1. Accept the proposal + // 2. Re-evaluate buffered commit messages + // 3. Detect commit quorum (3 commits for same value) + // 4. Achieve consensus with Success(test_data.hash()) + + println!("Verifying consensus achievement..."); + + // Should have accepted the proposal + assert!( + qbft_instance.proposal_accepted_for_current_round, + "Proposal should be accepted after receiving it" ); - // Instance should still be in AwaitingProposal state since commits were dropped + // Should have achieved consensus with the correct value assert!( - matches!(qbft_instance.state, InstanceState::AwaitingProposal), - "Instance should remain in AwaitingProposal state since commits were incorrectly dropped" + qbft_instance.completed.is_some(), + "BUG: Instance should have completed consensus after receiving proposal + buffered commits. \ + Current behavior drops commit messages when received before proposal, preventing \ + consensus in out-of-order scenarios." ); - // The instance should NOT have reached consensus due to dropped commits + // Verify the consensus result is correct + if let Some(completed) = qbft_instance.completed { + assert!( + matches!(completed, Completed::Success(hash) if hash == test_data.hash()), + "Consensus should succeed with the correct data hash. Got: {:?}, Expected: Success({})", + completed, + hex::encode(test_data.hash()) + ); + } + + // Should be in Complete state assert!( - qbft_instance.completed.is_none(), - "Instance should not have completed consensus due to dropped commit messages" + matches!(qbft_instance.state, InstanceState::Complete), + "Instance should be in Complete state after achieving consensus" ); + + println!("SUCCESS: Consensus achieved correctly despite out-of-order message delivery!"); } From 3e9996f6b7d5417a6cf2ff44748e9b3e966f3f06 Mon Sep 17 00:00:00 2001 From: diego Date: Fri, 29 Aug 2025 13:52:55 +0200 Subject: [PATCH 3/4] use a committee of 4 nodes --- anchor/common/qbft/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/anchor/common/qbft/src/tests.rs b/anchor/common/qbft/src/tests.rs index 61c580457..0e5bf0e7a 100644 --- a/anchor/common/qbft/src/tests.rs +++ b/anchor/common/qbft/src/tests.rs @@ -251,11 +251,11 @@ fn test_consensus_with_commits_before_proposal() { message::{MsgType, RSA_SIGNATURE_SIZE, SSVMessage, SignedSSVMessage}, }; - // Create QBFT instance with 3 nodes (f=0, quorum=3) + // Create QBFT instance with 4 nodes (f=1, quorum=3) let config = ConfigBuilder::::new( 1.into(), InstanceHeight::default(), - (1..4).map(OperatorId::from).collect(), // 3 nodes, quorum = 3 + (1..=4).map(OperatorId::from).collect(), // 4 nodes, quorum = 3 ) .with_operator_id(OperatorId::from(1)) .build() From cb6031ce40da667e1280411f84ef48f5fb885e37 Mon Sep 17 00:00:00 2001 From: diego Date: Fri, 29 Aug 2025 14:09:36 +0200 Subject: [PATCH 4/4] fmt --- anchor/common/qbft/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anchor/common/qbft/src/tests.rs b/anchor/common/qbft/src/tests.rs index 0e5bf0e7a..74dd49084 100644 --- a/anchor/common/qbft/src/tests.rs +++ b/anchor/common/qbft/src/tests.rs @@ -251,7 +251,7 @@ fn test_consensus_with_commits_before_proposal() { message::{MsgType, RSA_SIGNATURE_SIZE, SSVMessage, SignedSSVMessage}, }; - // Create QBFT instance with 4 nodes (f=1, quorum=3) + // Create QBFT instance with 4 nodes (f=1, quorum=3) let config = ConfigBuilder::::new( 1.into(), InstanceHeight::default(),