Skip to content

Conversation

diegomrsantos
Copy link
Contributor

Issue Addressed

This PR addresses future round PREPARE message buffering in QBFT consensus to ensure proper catch-up behavior during network partitions.

Proposed Changes

  • Add comprehensive test test_future_round_prepare_messages_rejected that verifies QBFT properly buffers PREPARE messages from future rounds
  • The test demonstrates expected behavior: nodes should buffer PREPARE messages from future rounds and make them available when advancing to those rounds
  • This ensures QBFT maintains liveness during network partitions where nodes may receive messages from different rounds out of order

Additional Info

The test scenario covers:

  1. Node in round 1 receives PREPARE messages from round 2 (future round)
  2. PREPARE messages are properly buffered for future processing
  3. When node advances to round 2, buffered messages become available for consensus

This buffering mechanism is essential for maintaining liveness during catch-up scenarios where nodes rejoin after network partitions or temporary outages.

Add test verifying that QBFT properly buffers PREPARE messages from future rounds for catch-up scenarios during network partitions.

The test ensures PREPARE messages from future rounds are buffered and available when advancing rounds, enabling proper catch-up during network partitions where nodes may receive messages from different rounds out of order.
@diegomrsantos diegomrsantos changed the base branch from stable to release-v0.3.0 August 29, 2025 10:36
@diegomrsantos diegomrsantos self-assigned this Aug 29, 2025
@diegomrsantos
Copy link
Contributor Author

@Zacholme7 @dknopik should we also buffer other messages?

@Zacholme7
Copy link
Member

seems like this also makes sense. We would need to refactor and implement a new mechanism to try to transition as many states as possible on each message receipt.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a comprehensive test case to verify QBFT's future round PREPARE message buffering behavior during catch-up scenarios. The test ensures that nodes properly buffer PREPARE messages from future rounds and make them available when advancing to those rounds, which is crucial for maintaining liveness during network partitions.

Key changes:

  • Added test test_future_round_prepare_messages_rejected that validates future round message buffering
  • Test simulates a realistic network partition scenario where a node receives messages from a future round
  • Verifies that buffered messages become available when the node catches up to that round

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +335 to +336
// Simulate advancing to round 2 (this would happen due to timeouts/round changes)
qbft_instance.current_round = Round::from(2);
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly manipulating the internal state current_round bypasses the normal QBFT state transition logic. This could mask bugs in the actual round advancement mechanism. Consider using the proper QBFT API to advance rounds (e.g., triggering a round change through timeouts or round change messages) to ensure the test validates realistic behavior.

Suggested change
// Simulate advancing to round 2 (this would happen due to timeouts/round changes)
qbft_instance.current_round = Round::from(2);
// Simulate advancing to round 2 by sending round change messages (this would happen due to timeouts/round changes)
// Instead of directly setting current_round, use the protocol's round change mechanism.
for operator_id in [2, 3, 4] {
let round_change_msg = QbftMessage {
qbft_message_type: QbftMessageType::RoundChange,
height: 0,
round: 2,
identifier: [0; 56].to_vec().into(),
root: test_data.hash(),
data_round: 0,
round_change_justification: vec![],
prepare_justification: vec![],
};
let round_change_ssv_message = SSVMessage::new(
MsgType::SSVConsensusMsgType,
MessageId::from([0; 56]),
round_change_msg.as_ssz_bytes(),
)
.expect("should create round change SSVMessage");
let signed_round_change = SignedSSVMessage::new(
vec![vec![0; RSA_SIGNATURE_SIZE]],
vec![OperatorId::from(operator_id)],
round_change_ssv_message,
vec![], // no full_data for round change
)
.expect("should create signed round change");
let wrapped_round_change = WrappedQbftMessage {
signed_message: signed_round_change,
qbft_message: round_change_msg,
};
qbft_instance.receive(wrapped_round_change);
}

Copilot uses AI. Check for mistakes.

@Zacholme7
Copy link
Member

opened issue #615

@Zacholme7 Zacholme7 closed this Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants