fix(qbft): commit message handling in catch-up scenarios

[diegomrsantos]

Issue Addressed

This PR addresses a bug in QBFT commit message handling where commit messages are dropped when received before a proposal, preventing consensus achievement in out-of-order message scenarios.

Proposed Changes

• Add comprehensive test test_consensus_with_commits_before_proposal that verifies QBFT can achieve consensus when commit messages arrive before proposals
• The test demonstrates the expected behavior: nodes should achieve consensus when they have both a valid proposal and commit quorum, regardless of message arrival order
• This ensures QBFT provides proper message ordering independence as required by distributed consensus protocols

Additional Info

The current implementation drops commit messages when proposal_accepted_for_current_round is false. The fix involves implementing proper message buffering and re-evaluation when proposals arrive.

Test Scenario:

1. Node receives commit messages from other nodes (commit quorum available)
2. Node later receives the corresponding proposal
3. Node achieves consensus with the committed value

This ensures network resilience by allowing nodes to properly participate in consensus even when experiencing temporary network issues or joining late.

[Zacholme7]

So we should just store the commit in the container before checking if the proposal was accepted?

[diegomrsantos]

So we should just store the commit in the container before checking if the proposal was accepted?

From what I understand, we can receive the messages out of order, so we need to buffer the commit messages in case we receive the proposal later. Do you think it makes sense?

[diegomrsantos]

The QBFT Specification Requirement

According to the QBFT specification, if:

1. We have a valid proposal for a value
2. We receive a quorum of commit messages for that same value

Then we should accept the value as committed, regardless of the order these messages arrived.

Why Buffering is Necessary

In distributed systems, messages can arrive out of order:

Scenario 1 (Normal Order):

1. Receive proposal for value X
2. Accept proposal
3. Receive commit messages for value X
4. Achieve commit quorum → Success ✅

Scenario 2 (Out of Order - Catch-up):

1. Receive commit messages for value X first (from other nodes)
2. Receive proposal for value X later
3. Accept proposal
4. Re-evaluate buffered commits → Achieve commit quorum → Success ✅

Current Anchor Bug

The current code at lines 748-750:

if !self.proposal_accepted_for_current_round {
    warn!(from=?operator_id, ?self.state, "Have not accepted Proposal for current round yet");
    return; // ❌ DROPS commit messages instead of buffering
}

This violates the QBFT specification because it prevents Scenario 2 from working correctly.

Correct Implementation Should:

1. Always buffer commit messages (regardless of proposal status)
2. When a proposal is accepted, re-evaluate all buffered commit messages
3. Check for commit quorum using both new and previously buffered commits

This ensures that message ordering doesn't affect consensus correctness, which is a fundamental requirement for robust distributed consensus protocols like QBFT.

[Zacholme7]

Okay good job, I did some more looking into this and it does appear to be a valid issue. This is spec compliance, but anchor and go-ssv both do not implement this. I think our first step would be to bring it up to them in the discord and coordinate a change. I dont think this change would be huge, but it does require refactoring our some pieces of our logic and thinking through every case where this is needed.

We are just doing the same thing as go-ssv atm

[diegomrsantos]

Okay good job, I did some more looking into this and it does appear to be a valid issue. This is spec compliance, but anchor and go-ssv both do not implement this. I think our first step would be to bring it up to them in the discord and coordinate a change. I dont think this change would be huge, but it does require refactoring our some pieces of our logic and thinking through every case where this is needed.

We are just doing the same thing as go-ssv atm

I agree, let's do it

[Zacholme7]

Think we should close these for now and make an issue for each linking these prs?

[diegomrsantos]

Think we should close these for now and make an issue for each linking these prs?

But thinking more about it, what would be the issue if Anchor fixes it while Go doesn't?

[Zacholme7]

I dont think there is an issue, but atm I think our focus should be making sure we are 100% confident in our implementations correctness with respect to go-ssv and then coordinate these extra changes.

[diegomrsantos]

I dont think there is an issue, but atm I think our focus should be making sure we are 100% confident in our implementations correctness with respect to go-ssv and then coordinate these extra changes.

Shouldn't we be correct wrt to the spec? If this is really an issue that happens in production, although probably not often, an Anchor only cluster could have worse performance. But I agree it might not be high in the priority. @jking-aus @AgeManning what are your thoughts?

[Zacholme7]

Are we good to close these and just put them into an issue?

[diegomrsantos]

Are we good to close these and just put them into an issue?

Should we raise this on discord?

[Zacholme7]

opened issue #614