Skip to content

[BFT] Remove VoteCollectorStatusInvalid from state machine #8219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
durkmurder wants to merge 5 commits into
base: master
Choose a base branch
from
+125 −86
Open

[BFT] Remove VoteCollectorStatusInvalid from state machine #8219

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef4b114
Removed invalid state for VoteCollector
durkmurder Dec 3, 2025
1bd4efd
Merge branch 'master' into yurii/6127-remove-termination-of-vote-proc…
durkmurder Dec 5, 2025
25a198a
Merge branch 'master' into yurii/6127-remove-termination-of-vote-proc…
durkmurder Dec 8, 2025
41d1169
BFT documentation: component-level breakdown own safety and liveness,…
AlexHentschel Dec 23, 2025
ab14a1d
typos
AlexHentschel Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions consensus/hotstuff/vote_collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ const (
// VoteCollectorStatusVerifying is for the status when the block has been received,
// and is able to process all votes for it.
VoteCollectorStatusVerifying

// VoteCollectorStatusInvalid is for the status when the block has been verified and
// is invalid. All votes to this block will be collected to slash the voter.
VoteCollectorStatusInvalid
)

// VoteCollector collects votes for the same block, produces QC when enough votes are collected
Expand Down
48 changes: 1 addition & 47 deletions consensus/hotstuff/votecollector/statemachine.go
View file
Open in desktop
Copy link
Member

@AlexHentschel AlexHentschel Dec 23, 2025

Choose a reason for hiding this comment

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

I assume the following no longer holds:

flow-go/consensus/hotstuff/votecollector/statemachine.go

Lines 50 to 55 in ef4b114

// * In the current implementation, the `votesCache` does not catch leader attacks 1. and 2., which exploit the fact that stand-alone
// votes and votes embedded in proposals are processed concurrently through different code paths. We emphasize that attack vectors
// 1. and 2. are only available to a byzantine leader as long as the leader has not (yet) equivocated for the current view. Once
// the VoteCollector notices that the _leader_ equivocates, it immediately stops accepting proposals for the view, thereby closing
// attack vectors 1. and 2. Hence, it is sufficient for the [VerifyingVoteProcessor] to catch _all_ equivocation attacks,
// including attacks 1. and 2.

Specifically:

it immediately stops accepting proposals for the view

Copy link
Member

@AlexHentschel AlexHentschel Dec 23, 2025

Choose a reason for hiding this comment

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

I tried to address this with my commit ... turned into bit of a broader breakdown. Please take a look when you have time 🎄

Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,7 @@ func (m *VoteCollector) processVote(vote *model.Vote) error {
// Informally, the state transition will happen _after_ we cached the vote.
// * Case (c): `currentState` = [VoteCollectorStatusCaching] while `m.Status()` = [VoteCollectorStatusVerifying].
// In this scenario, the vote is being fed into the [NoopProcessor] first, before we realize that the state has changed. However,
// since the status has changed, the check below will trigger a repeat of the processing, which will then enter case (a)
// (or leave the happy path by transitioning to [VoteCollectorStatusInvalid], implying we are dealing with a byzantine proposer, in which
// case we may drop all votes anyway).
// since the status has changed, the check below will trigger a repeat of the processing, which will then enter case (a).
// We have shown that all votes will reach the [VerifyingVoteProcessor] on the happy path.
//
// CAUTION: In the proof, we utilized that reading the `votesCache` happens before writing to it (case b). It is important to emphasize that
Expand Down Expand Up @@ -283,8 +281,6 @@ func (m *VoteCollector) View() uint64 {
// equal to `expectedValue`. The implementation only allows the transitions
//
// CachingVotes → VerifyingVotes
// CachingVotes → Invalid
// VerifyingVotes → Invalid
//
// No errors are expected during normal operation (Byzantine edge cases handled via notifications internally).
func (m *VoteCollector) ProcessBlock(proposal *model.SignedProposal) error {
Expand Down Expand Up @@ -339,13 +335,6 @@ func (m *VoteCollector) ProcessBlock(proposal *model.SignedProposal) error {
return fmt.Errorf("while processing block %v, found that VoteProcessor reports status %s but has an incompatible implementation type %T",
Copy link
Member

@AlexHentschel AlexHentschel Dec 22, 2025

Choose a reason for hiding this comment

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

Are the following lines still needed?

flow-go/consensus/hotstuff/votecollector/statemachine.go

Lines 333 to 337 in 25a198a

verifyingProc, ok := proc.(hotstuff.VerifyingVoteProcessor)
if !ok {
return fmt.Errorf("while processing block %v, found that VoteProcessor reports status %s but has an incompatible implementation type %T",
proposal.Block.BlockID, proc.Status(), verifyingProc)
}

Previously, we did the cast, because we wanted to call terminateVoteProcessing. Since that's no longer happening, the cast here is essentially only a sanity check.

For simplicity, I would be inclined to remove the code, because this sanity check just covers a very narrow range of bugs, for which we typically don't include similar sanity checks. Furthermore, in the specific case here, there is nothing that would go catastrophically wrong immediately if the didn't detect the wrong type.

proposal.Block.BlockID, proc.Status(), verifyingProc)
}
if verifyingProc.Block().BlockID != proposal.Block.BlockID {
m.terminateVoteProcessing()
}

// Vote processing for this view has already been terminated. Note: proposal equivocation
// is handled by hotstuff.Forks, so we don't have anything to do here.
case hotstuff.VoteCollectorStatusInvalid: /* no op */

default:
return fmt.Errorf("while processing block %v, found that VoteProcessor reported unknown status %s", proposal.Block.BlockID, proc.Status())
Expand Down Expand Up @@ -404,41 +393,6 @@ func (m *VoteCollector) caching2Verifying(proposal *model.SignedProposal) error
return nil
}

// terminateVoteProcessing atomically transitions the VoteCollector into state
// `VoteCollectorStatusInvalid`. if it wasn't already in this state.
func (m *VoteCollector) terminateVoteProcessing() {
currentProcWrapper := m.votesProcessor.Load().(*atomicValueWrapper)
if currentProcWrapper.processor.Status() == hotstuff.VoteCollectorStatusInvalid {
return
}

newProcWrapper := &atomicValueWrapper{
processor: NewNoopCollector(hotstuff.VoteCollectorStatusInvalid),
}

// We now have an optimistically-constructed `newProcWrapper` that represents the desired new state (happy path). We must ensure
// that writing the `newProcWrapper` to `m.votesProcessor` happens ATOMICALLY if and only if the current state is not
// `VoteCollectorStatusInvalid`. The "Compare-And-Swap Loop" (CAS LOOP) is an efficient pattern to implement this:
// (i) We first retrieved the current state (above) and checked whether it is different from `VoteCollectorStatusInvalid`.
// (ii) If so, we attempt to compare-and-swap the current with the new state.
// Note that (i) and (ii) are separate operations. However, the CAS in (ii) ensures that the write only happens if the current state
// is still the same as what we observed in (i). If another thread changed the state in between (i) and (ii), we have worked with
// an outdated view of the current state, and should repeat the attempt to update the state (hence the "loop" in CAS LOOP).
// Since we are storing a pointer in the atomic.Value the value compared will be the reference to the object.
for {
stateUpdateSuccessful := m.votesProcessor.CompareAndSwap(currentProcWrapper, newProcWrapper)
if stateUpdateSuccessful {
return
}
// the `currentProcWrapper` we worked with was stale:
// reload, check if invalid target state has already been reached, and repeat if not
currentProcWrapper = m.votesProcessor.Load().(*atomicValueWrapper)
if currentProcWrapper.processor.Status() == hotstuff.VoteCollectorStatusInvalid {
return
}
}
}

// processCachedVotes feeds all cached votes into the VoteProcessor
func (m *VoteCollector) processCachedVotes(block *model.Block) {
cachedVotes := m.votesCache.All()
Expand Down
5 changes: 0 additions & 5 deletions consensus/hotstuff/votecollector/statemachine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,6 @@ func (s *StateMachineTestSuite) TestStatus_StateTransitions() {
err := s.collector.ProcessBlock(proposal)
require.NoError(s.T(), err)
require.Equal(s.T(), hotstuff.VoteCollectorStatusVerifying, s.collector.Status())

// after submitting double proposal we should transfer into invalid state
err = s.collector.ProcessBlock(makeSignedProposalWithView(s.view))
require.NoError(s.T(), err)
require.Equal(s.T(), hotstuff.VoteCollectorStatusInvalid, s.collector.Status())
}

// TestStatus_FactoryErrorPropagation verifies that errors from the injected
Expand Down
Loading