From f977043b2243c5688859c0f2bc4bdb951d3ad203 Mon Sep 17 00:00:00 2001 From: iurii Date: Tue, 28 Apr 2026 10:22:28 +0300 Subject: [PATCH 1/4] qbft: clarify round-changes --- message/validation/consensus_validation.go | 10 +-- message/validation/errors.go | 1 - message/validation/validation_test.go | 4 +- protocol/v2/qbft/controller/decided.go | 6 +- protocol/v2/qbft/instance/instance.go | 26 ++++---- protocol/v2/qbft/instance/observability.go | 1 - protocol/v2/qbft/instance/prepare.go | 6 +- protocol/v2/qbft/instance/proposal.go | 43 +++++------- protocol/v2/qbft/instance/round_change.go | 65 ++++++++----------- .../v2/qbft/instance/test_helpers_test.go | 4 +- protocol/v2/qbft/instance/timeout.go | 35 ++++------ protocol/v2/qbft/instance/timeout_test.go | 15 +++-- protocol/v2/qbft/messages.go | 8 +++ 13 files changed, 95 insertions(+), 129 deletions(-) create mode 100644 protocol/v2/qbft/messages.go diff --git a/message/validation/consensus_validation.go b/message/validation/consensus_validation.go index 61ed77ecdf..394f3112c1 100644 --- a/message/validation/consensus_validation.go +++ b/message/validation/consensus_validation.go @@ -17,6 +17,7 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" "github.com/ssvlabs/ssv/protocol/v2/message" + "github.com/ssvlabs/ssv/protocol/v2/qbft" "github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" ) @@ -116,15 +117,8 @@ func (mv *messageValidator) validateConsensusMessageSemantics( return ErrPrepareOrCommitWithFullData } - hashedFullData, err := specqbft.HashDataRoot(signedSSVMessage.FullData) - if err != nil { - e := ErrFullDataHash - e.innerErr = err - return e - } - // Rule: Full data hash must match root - if hashedFullData != consensusMessage.Root { + if qbft.HashDataRoot(signedSSVMessage.FullData) != consensusMessage.Root { return ErrInvalidHash } } diff --git a/message/validation/errors.go b/message/validation/errors.go index 679b6b087f..72cf64a0ba 100644 --- a/message/validation/errors.go +++ b/message/validation/errors.go @@ -118,7 +118,6 @@ var ( ErrSignersNotSorted = Error{text: "signers are not sorted", reject: true} ErrInconsistentSigners = Error{text: "signer is not expected", reject: true} ErrInvalidHash = Error{text: "root doesn't match full data hash", reject: true} - ErrFullDataHash = Error{text: "couldn't hash root", reject: true} ErrUndecodableMessageData = Error{text: "message data could not be decoded", reject: true} ErrEventMessage = Error{text: "unexpected event message", reject: true} ErrUnknownSSVMessageType = Error{text: "unknown SSV message type", reject: true} diff --git a/message/validation/validation_test.go b/message/validation/validation_test.go index 90a99780fc..646923415c 100644 --- a/message/validation/validation_test.go +++ b/message/validation/validation_test.go @@ -38,6 +38,7 @@ import ( "github.com/ssvlabs/ssv/operator/duties/dutystore" "github.com/ssvlabs/ssv/operator/storage" "github.com/ssvlabs/ssv/protocol/v2/message" + "github.com/ssvlabs/ssv/protocol/v2/qbft" "github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" registrystorage "github.com/ssvlabs/ssv/registry/storage" @@ -1559,8 +1560,7 @@ func Test_ValidateSSVMessage(t *testing.T) { anotherFullData := []byte{1} signedSSVMessage = generateSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { - message.Root, err = specqbft.HashDataRoot(anotherFullData) - require.NoError(t, err) + message.Root = qbft.HashDataRoot(anotherFullData) }) signedSSVMessage.FullData = anotherFullData diff --git a/protocol/v2/qbft/controller/decided.go b/protocol/v2/qbft/controller/decided.go index 226931f119..643d611939 100644 --- a/protocol/v2/qbft/controller/decided.go +++ b/protocol/v2/qbft/controller/decided.go @@ -9,6 +9,7 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" "go.uber.org/zap" + "github.com/ssvlabs/ssv/protocol/v2/qbft" "github.com/ssvlabs/ssv/protocol/v2/qbft/instance" "github.com/ssvlabs/ssv/protocol/v2/ssv" ) @@ -97,10 +98,7 @@ func (c *Controller) ValidateDecided(msg *specqbft.ProcessingMessage) error { return fmt.Errorf("invalid decided msg: %w", err) } - r, err := specqbft.HashDataRoot(msg.SignedMessage.FullData) - if err != nil { - return fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(msg.SignedMessage.FullData) if !bytes.Equal(r[:], msg.QBFTMessage.Root[:]) { return spectypes.NewError(spectypes.RootHashInvalidErrorCode, "H(data) != root") } diff --git a/protocol/v2/qbft/instance/instance.go b/protocol/v2/qbft/instance/instance.go index 5a1da47550..80f5929daf 100644 --- a/protocol/v2/qbft/instance/instance.go +++ b/protocol/v2/qbft/instance/instance.go @@ -121,12 +121,7 @@ func (i *Instance) Start( // TODO align spec to add else to avoid broadcast errored proposal } - startValueRoot, err := specqbft.HashDataRoot(i.StartValue) - if err != nil { - logger.Warn("❗ failed to hash instance start value", zap.Error(err)) - span.SetStatus(codes.Error, err.Error()) - return - } + startValueRoot := qbft.HashDataRoot(i.StartValue) logger = logger.With(zap.String("qbft_start_value_root", hex.EncodeToString(startValueRoot[:]))) const eventMsg = "📢 leader broadcasting proposal message" @@ -172,8 +167,8 @@ func (i *Instance) MarkIrrelevant() { } func (i *Instance) Broadcast(msg *spectypes.SignedSSVMessage) error { - if !i.CanProcessMessages() { - return spectypes.NewError(spectypes.InstanceStoppedProcessingMessagesErrorCode, "instance stopped processing messages") + if !i.IsRelevant() { + return spectypes.NewError(spectypes.InstanceStoppedProcessingMessagesErrorCode, "instance is no longer considered relevant") } return i.GetConfig().GetNetwork().Broadcast(msg.SSVMessage.GetID(), msg) @@ -191,8 +186,8 @@ func allSigners(all []*specqbft.ProcessingMessage) []spectypes.OperatorID { // The returned bool/value pair reports whether this call newly decided the // instance. Callers that need the post-call state should inspect State/IsDecided. func (i *Instance) ProcessMsg(ctx context.Context, logger *zap.Logger, msg *specqbft.ProcessingMessage) (decided bool, decidedValue []byte, aggregatedCommit *spectypes.SignedSSVMessage, err error) { - if !i.CanProcessMessages() { - return false, nil, nil, spectypes.NewError(spectypes.InstanceStoppedProcessingMessagesErrorCode, "instance stopped processing messages") + if !i.IsRelevant() { + return false, nil, nil, spectypes.NewError(spectypes.InstanceStoppedProcessingMessagesErrorCode, "instance is no longer considered relevant") } if err := i.BaseMsgValidation(msg); err != nil { @@ -304,12 +299,15 @@ func (i *Instance) Decode(data []byte) error { return json.Unmarshal(data, &i) } -// bumpToRound sets round and sends current round metrics. +// bumpToRound pushes this instance to a higher round, also scheduling a timeout for it. func (i *Instance) bumpToRound(round specqbft.Round) { - i.State.Round = round + if round > i.State.Round { + i.State.Round = round + i.roundTimer.TimeoutForRound(round) + } } -// CanProcessMessages will return true if instance can process messages -func (i *Instance) CanProcessMessages() bool { +// IsRelevant will return true if instance can process messages +func (i *Instance) IsRelevant() bool { return !i.markedIrrelevant && i.State.Round < i.config.GetCutOffRound() } diff --git a/protocol/v2/qbft/instance/observability.go b/protocol/v2/qbft/instance/observability.go index 2bf2dcfcb9..a9738cb3a6 100644 --- a/protocol/v2/qbft/instance/observability.go +++ b/protocol/v2/qbft/instance/observability.go @@ -31,7 +31,6 @@ type roundChangeReason string const ( reasonTimeout roundChangeReason = "timeout" reasonPartialQuorum roundChangeReason = "partial-quorum" - reasonJustified roundChangeReason = "justified" ) var ( diff --git a/protocol/v2/qbft/instance/prepare.go b/protocol/v2/qbft/instance/prepare.go index e1f937138d..b19c156afa 100644 --- a/protocol/v2/qbft/instance/prepare.go +++ b/protocol/v2/qbft/instance/prepare.go @@ -10,6 +10,7 @@ import ( "go.uber.org/zap" "github.com/ssvlabs/ssv/observability/log/fields" + "github.com/ssvlabs/ssv/protocol/v2/qbft" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" ) @@ -70,10 +71,7 @@ func (i *Instance) getRoundChangeJustification() ([]*specqbft.ProcessingMessage, return nil, nil } - r, err := specqbft.HashDataRoot(i.State.LastPreparedValue) - if err != nil { - return nil, fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(i.State.LastPreparedValue) prepareMsgs := i.State.PrepareContainer.MessagesForRound(i.State.LastPreparedRound) ret := make([]*specqbft.ProcessingMessage, 0) diff --git a/protocol/v2/qbft/instance/proposal.go b/protocol/v2/qbft/instance/proposal.go index 7d9ecc83b6..ecf52b9683 100644 --- a/protocol/v2/qbft/instance/proposal.go +++ b/protocol/v2/qbft/instance/proposal.go @@ -10,6 +10,7 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" "go.uber.org/zap" + "github.com/ssvlabs/ssv/protocol/v2/qbft" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" ) @@ -28,24 +29,21 @@ func (i *Instance) uponProposal(ctx context.Context, logger *zap.Logger, msg *sp logger.Debug("📬 got proposal message") - i.State.ProposalAcceptedForCurrentRound = msg - + currentRound := i.State.Round msgRound := msg.QBFTMessage.Round - // A future justified proposal should bump us into future round and reset the round timer. - if msgRound > i.State.Round { - i.roundTimer.TimeoutForRound(msgRound) - } - i.bumpToRound(msgRound) - - i.metrics.EndStage(ctx, msgRound) + i.metrics.EndStage(ctx, currentRound) i.metrics.StartStage(stagePrepare) - r, err := specqbft.HashDataRoot(msg.SignedMessage.FullData) - if err != nil { - return fmt.Errorf("could not hash input data: %w", err) - } + i.State.ProposalAcceptedForCurrentRound = msg + // A future justified proposal should move us into the future round, hence we try to bump the round here. + // Always move on to the message-round. The round-change message broadcast is a best-effort thing, the QBFT + // cluster as a whole can progress further even if our round-change message cannot be created/broadcast + // for whatever reason. + i.bumpToRound(msgRound) + + r := qbft.HashDataRoot(msg.SignedMessage.FullData) prepare, err := i.CreatePrepare(msgRound, r) if err != nil { return fmt.Errorf("could not create prepare msg: %w", err) @@ -83,10 +81,7 @@ func (i *Instance) isValidProposal(msg *specqbft.ProcessingMessage) error { } // verify full data integrity - r, err := specqbft.HashDataRoot(msg.SignedMessage.FullData) - if err != nil { - return fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(msg.SignedMessage.FullData) if !bytes.Equal(msg.QBFTMessage.Root[:], r[:]) { return spectypes.NewError(spectypes.RootHashInvalidErrorCode, "H(data) != root") } @@ -189,10 +184,7 @@ func (i *Instance) isProposalJustification( } // proposed fullData must equal highest prepared fullData - r, err := specqbft.HashDataRoot(fullData) - if err != nil { - return fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(fullData) if !bytes.Equal(r[:], rcMsg.QBFTMessage.Root[:]) { return errors.New("proposed data doesn't match highest prepared") } @@ -233,16 +225,13 @@ func (i *Instance) ProposerForRound(round specqbft.Round) spectypes.OperatorID { extractSignedPrepares(prepares)); */ func (i *Instance) CreateProposal(fullData []byte, roundChanges, prepares []*specqbft.ProcessingMessage) (*spectypes.SignedSSVMessage, error) { - r, err := specqbft.HashDataRoot(fullData) - if err != nil { - return nil, fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(fullData) - roundChangeSignedMessages := make([]*spectypes.SignedSSVMessage, 0) + roundChangeSignedMessages := make([]*spectypes.SignedSSVMessage, 0, len(roundChanges)) for _, msg := range roundChanges { roundChangeSignedMessages = append(roundChangeSignedMessages, msg.SignedMessage) } - prepareSignedMessages := make([]*spectypes.SignedSSVMessage, 0) + prepareSignedMessages := make([]*spectypes.SignedSSVMessage, 0, len(prepares)) for _, msg := range prepares { prepareSignedMessages = append(prepareSignedMessages, msg.SignedMessage) } diff --git a/protocol/v2/qbft/instance/round_change.go b/protocol/v2/qbft/instance/round_change.go index d6b9e1dc87..48ee153b93 100644 --- a/protocol/v2/qbft/instance/round_change.go +++ b/protocol/v2/qbft/instance/round_change.go @@ -11,7 +11,9 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" "go.uber.org/zap" + "github.com/ssvlabs/ssv/observability" "github.com/ssvlabs/ssv/observability/log/fields" + "github.com/ssvlabs/ssv/protocol/v2/qbft" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" ) @@ -22,9 +24,8 @@ func (i *Instance) uponRoundChange( logger *zap.Logger, msg *specqbft.ProcessingMessage, ) error { - prevRound := i.State.Round logger = logger.With( - zap.Uint64("qbft_instance_round", uint64(prevRound)), + zap.Uint64("qbft_instance_round", uint64(i.State.Round)), zap.Uint64("qbft_instance_height", uint64(i.State.Height)), ) @@ -53,6 +54,9 @@ func (i *Instance) uponRoundChange( } if justifiedRoundChangeMsg != nil { + i.metrics.EndStage(ctx, i.State.Round) + i.metrics.StartStage(stageProposal) + roundChangeJustificationSignedMessages, _ := justifiedRoundChangeMsg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, check on isValidRoundChange roundChangeJustification := make([]*specqbft.ProcessingMessage, 0) @@ -66,26 +70,18 @@ func (i *Instance) uponRoundChange( proposal, err := i.CreateProposal( valueToPropose, - i.State.RoundChangeContainer.MessagesForRound(prevRound), // TODO - might be optimized to include only necessary quorum + i.State.RoundChangeContainer.MessagesForRound(i.State.Round), // TODO - might be optimized to include only necessary quorum roundChangeJustification, ) if err != nil { return fmt.Errorf("failed to create proposal: %w", err) } - valueToProposeRoot, err := specqbft.HashDataRoot(valueToPropose) - if err != nil { - return fmt.Errorf("failed to hash value-to-propose: %w", err) - } + valueToProposeRoot := qbft.HashDataRoot(valueToPropose) logger = logger.With(zap.String("qbft_value_to_propose_root", hex.EncodeToString(valueToProposeRoot[:]))) - i.metrics.EndStage(ctx, prevRound) - i.metrics.StartStage(stageProposal) - - i.metrics.RecordRoundChange(ctx, prevRound, reasonJustified) - logger.Debug("🔄 got justified round change, leader broadcasting proposal message", - zap.Any("round_change_signers", allSigners(i.State.RoundChangeContainer.MessagesForRound(prevRound))), + zap.Any("round_change_signers", allSigners(i.State.RoundChangeContainer.MessagesForRound(i.State.Round))), ) if err := i.Broadcast(proposal); err != nil { @@ -93,41 +89,42 @@ func (i *Instance) uponRoundChange( } } else if partialQuorum, rcs := i.hasReceivedPartialQuorum(); partialQuorum { newRound := minRound(rcs) - if newRound <= prevRound { + if newRound <= i.State.Round { // No need to advance round, we've already changed it. return nil } - i.metrics.EndStage(ctx, prevRound) - i.metrics.StartStage(stageRoundChange) - - i.metrics.RecordRoundChange(ctx, prevRound, reasonPartialQuorum) - - err := i.uponChangeRoundPartialQuorum(logger, newRound) - if err != nil { + if err := i.uponChangeRoundPartialQuorum(ctx, logger, newRound); err != nil { return err } } return nil } -func (i *Instance) uponChangeRoundPartialQuorum(logger *zap.Logger, newRound specqbft.Round) error { +func (i *Instance) uponChangeRoundPartialQuorum(ctx context.Context, logger *zap.Logger, newRound specqbft.Round) error { + ctx, span := tracer.Start(ctx, observability.InstrumentName(observabilityNamespace, "qbft.instance.change_round_partial_quorum")) + defer span.End() + + prevRound := i.State.Round + + i.metrics.EndStage(ctx, prevRound) + i.metrics.StartStage(stageRoundChange) + i.metrics.RecordRoundChange(ctx, prevRound, reasonPartialQuorum) + + // Always move on to the next round. The round-change message broadcast is a best-effort thing, the QBFT + // cluster as a whole can progress further even if our round-change message cannot be created/broadcast + // for whatever reason. i.bumpToRound(newRound) i.State.ProposalAcceptedForCurrentRound = nil - i.roundTimer.TimeoutForRound(newRound) + startValueRoot := qbft.HashDataRoot(i.StartValue) + logger = logger.With(zap.String("qbft_start_value_root", hex.EncodeToString(startValueRoot[:]))) roundChange, err := i.CreateRoundChange(newRound) if err != nil { return fmt.Errorf("failed to create round change message: %w", err) } - startValueRoot, err := specqbft.HashDataRoot(i.StartValue) - if err != nil { - return fmt.Errorf("failed to hash instance start value: %w", err) - } - logger = logger.With(zap.String("qbft_start_value_root", hex.EncodeToString(startValueRoot[:]))) - logger.Debug("📢 broadcasting round change message (got partial quorum)", zap.Uint64("qbft_new_round", uint64(newRound)), zap.Any("round_change_signers", roundChange.OperatorIDs), @@ -282,10 +279,7 @@ func (i *Instance) validRoundChangeForDataIgnoreSignature( // Addition to formal spec // We add this extra tests on the msg itself to filter round change msgs with invalid justifications, before they are inserted into msg containers if msg.QBFTMessage.RoundChangePrepared() { - r, err := specqbft.HashDataRoot(fullData) - if err != nil { - return fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(fullData) // validate prepare message justifications prepareSignedMsgs, _ := msg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, checked on msg.QBFTMessage.Validate() @@ -384,10 +378,7 @@ func (i *Instance) getRoundChangeData() (specqbft.Round, [32]byte, []byte, []*sp return specqbft.NoRound, [32]byte{}, nil, nil, fmt.Errorf("could not get round change justification: %w", err) } - r, err := specqbft.HashDataRoot(i.State.LastPreparedValue) - if err != nil { - return specqbft.NoRound, [32]byte{}, nil, nil, fmt.Errorf("could not hash input data: %w", err) - } + r := qbft.HashDataRoot(i.State.LastPreparedValue) return i.State.LastPreparedRound, r, i.State.LastPreparedValue, justifications, nil } diff --git a/protocol/v2/qbft/instance/test_helpers_test.go b/protocol/v2/qbft/instance/test_helpers_test.go index f4fb171a8f..9dbbcefb79 100644 --- a/protocol/v2/qbft/instance/test_helpers_test.go +++ b/protocol/v2/qbft/instance/test_helpers_test.go @@ -118,9 +118,7 @@ func (e *instanceTestEnv) setNetwork(network specqbft.Network) { func (e *instanceTestEnv) hash(fullData []byte) [32]byte { e.t.Helper() - root, err := specqbft.HashDataRoot(fullData) - require.NoError(e.t, err) - return root + return qbftconfig.HashDataRoot(fullData) } func (e *instanceTestEnv) marshalJustifications(msgs []*specqbft.ProcessingMessage) [][]byte { diff --git a/protocol/v2/qbft/instance/timeout.go b/protocol/v2/qbft/instance/timeout.go index 9b66edf15e..a32f57b9eb 100644 --- a/protocol/v2/qbft/instance/timeout.go +++ b/protocol/v2/qbft/instance/timeout.go @@ -4,53 +4,46 @@ import ( "context" "encoding/hex" - specqbft "github.com/ssvlabs/ssv-spec/qbft" "github.com/ssvlabs/ssv-spec/types" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/ssvlabs/ssv/observability" "github.com/ssvlabs/ssv/observability/traces" + "github.com/ssvlabs/ssv/protocol/v2/qbft" ) func (i *Instance) UponRoundTimeout(ctx context.Context, logger *zap.Logger) error { ctx, span := tracer.Start(ctx, observability.InstrumentName(observabilityNamespace, "qbft.instance.round_timeout")) defer span.End() - if !i.CanProcessMessages() { - return types.WrapError(types.TimeoutInstanceErrorCode, traces.Errorf(span, "instance stopped processing timeouts")) + if !i.IsRelevant() { + return types.WrapError(types.TimeoutInstanceErrorCode, traces.Errorf(span, "instance is no longer considered relevant")) } - i.metrics.EndStage(ctx, i.State.Round) + prevRound := i.State.Round + newRound := prevRound + 1 + + i.metrics.EndStage(ctx, prevRound) i.metrics.StartStage(stageRoundChange) + i.metrics.RecordRoundChange(ctx, prevRound, reasonTimeout) - startValueRoot, err := specqbft.HashDataRoot(i.StartValue) - if err != nil { - return traces.Errorf(span, "failed to hash instance start value: %w", err) - } + startValueRoot := qbft.HashDataRoot(i.StartValue) logger = logger.With(zap.String("qbft_start_value_root", hex.EncodeToString(startValueRoot[:]))) logger.Debug("⌛ round timed out") - prevRound := i.State.Round - newRound := prevRound + 1 - - // TODO: previously this was done outside of a defer, which caused the - // round to be bumped before the round change message was created & broadcasted. - // Remember to track the impact of this change and revert/modify if necessary. - defer func() { - i.bumpToRound(newRound) - i.State.ProposalAcceptedForCurrentRound = nil - i.roundTimer.TimeoutForRound(newRound) - }() + // Always move on to the next round. The round-change message broadcast is a best-effort thing, the QBFT + // cluster as a whole can progress further even if our round-change message cannot be created/broadcast + // for whatever reason. + i.bumpToRound(newRound) + i.State.ProposalAcceptedForCurrentRound = nil roundChange, err := i.CreateRoundChange(newRound) if err != nil { return traces.Errorf(span, "could not generate round change msg: %w", err) } - i.metrics.RecordRoundChange(ctx, prevRound, reasonTimeout) - const eventMsg = "📢 broadcasting round change message (this round timed out)" span.AddEvent(eventMsg, trace.WithAttributes(observability.BeaconBlockRootAttribute(startValueRoot), observability.DutyRoundAttribute(prevRound))) logger.Debug( diff --git a/protocol/v2/qbft/instance/timeout_test.go b/protocol/v2/qbft/instance/timeout_test.go index 4c840f5243..38e62926e5 100644 --- a/protocol/v2/qbft/instance/timeout_test.go +++ b/protocol/v2/qbft/instance/timeout_test.go @@ -9,7 +9,7 @@ import ( "go.uber.org/zap" ) -func TestUponRoundTimeoutBumpsRoundAfterBroadcast(t *testing.T) { +func TestUponRoundTimeoutBumpsRound(t *testing.T) { env := newInstanceTestEnv(t, 2) env.inst.State.Round = 1 env.inst.State.ProposalAcceptedForCurrentRound = env.proposal(1, 1, []byte("proposal-value"), env.hash([]byte("proposal-value")), nil, nil) @@ -26,8 +26,8 @@ func TestUponRoundTimeoutBumpsRoundAfterBroadcast(t *testing.T) { network := &recordingNetwork{ onBroadcast: func(message *spectypes.SignedSSVMessage) error { - require.Equal(t, specqbft.Round(1), env.inst.State.Round) - require.NotNil(t, env.inst.State.ProposalAcceptedForCurrentRound) + require.Equal(t, specqbft.Round(2), env.inst.State.Round) + require.Nil(t, env.inst.State.ProposalAcceptedForCurrentRound) return nil }, } @@ -56,7 +56,7 @@ func TestUponRoundTimeoutKilledInstance(t *testing.T) { env.inst.MarkIrrelevant() err := env.inst.UponRoundTimeout(t.Context(), zap.NewNop()) - require.ErrorContains(t, err, "instance stopped processing timeouts") + require.ErrorContains(t, err, "instance is no longer considered relevant") } func TestUponRoundTimeoutStopsProcessingAfterReachingCutOffRound(t *testing.T) { @@ -65,9 +65,10 @@ func TestUponRoundTimeoutStopsProcessingAfterReachingCutOffRound(t *testing.T) { env.config.CutOffRound = env.inst.State.Round + 1 err := env.inst.UponRoundTimeout(t.Context(), zap.NewNop()) - require.NoError(t, err) - require.Equal(t, specqbft.Round(2), env.inst.State.Round) + require.ErrorContains(t, err, "instance is no longer considered relevant") + require.Equal(t, env.config.CutOffRound, env.inst.State.Round) err = env.inst.UponRoundTimeout(t.Context(), zap.NewNop()) - require.ErrorContains(t, err, "instance stopped processing timeouts") + require.ErrorContains(t, err, "instance is no longer considered relevant") + require.Equal(t, env.config.CutOffRound, env.inst.State.Round) } diff --git a/protocol/v2/qbft/messages.go b/protocol/v2/qbft/messages.go new file mode 100644 index 0000000000..d61b21434d --- /dev/null +++ b/protocol/v2/qbft/messages.go @@ -0,0 +1,8 @@ +package qbft + +import "crypto/sha256" + +// HashDataRoot hashes input data to root. +func HashDataRoot(data []byte) [32]byte { + return sha256.Sum256(data) +} From bcd0593d7458c31d4a20b0c856e2d4e1df647be4 Mon Sep 17 00:00:00 2001 From: iurii Date: Tue, 28 Apr 2026 10:56:15 +0300 Subject: [PATCH 2/4] cleanup --- protocol/v2/qbft/instance/instance.go | 1 + protocol/v2/qbft/instance/proposal.go | 4 ++-- protocol/v2/qbft/instance/round_change.go | 1 - protocol/v2/qbft/instance/timeout.go | 1 - protocol/v2/qbft/messages_test.go | 21 +++++++++++++++++++++ 5 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 protocol/v2/qbft/messages_test.go diff --git a/protocol/v2/qbft/instance/instance.go b/protocol/v2/qbft/instance/instance.go index 80f5929daf..c0d32f6037 100644 --- a/protocol/v2/qbft/instance/instance.go +++ b/protocol/v2/qbft/instance/instance.go @@ -302,6 +302,7 @@ func (i *Instance) Decode(data []byte) error { // bumpToRound pushes this instance to a higher round, also scheduling a timeout for it. func (i *Instance) bumpToRound(round specqbft.Round) { if round > i.State.Round { + i.State.ProposalAcceptedForCurrentRound = nil i.State.Round = round i.roundTimer.TimeoutForRound(round) } diff --git a/protocol/v2/qbft/instance/proposal.go b/protocol/v2/qbft/instance/proposal.go index ecf52b9683..bc8b2eaa2c 100644 --- a/protocol/v2/qbft/instance/proposal.go +++ b/protocol/v2/qbft/instance/proposal.go @@ -35,14 +35,14 @@ func (i *Instance) uponProposal(ctx context.Context, logger *zap.Logger, msg *sp i.metrics.EndStage(ctx, currentRound) i.metrics.StartStage(stagePrepare) - i.State.ProposalAcceptedForCurrentRound = msg - // A future justified proposal should move us into the future round, hence we try to bump the round here. // Always move on to the message-round. The round-change message broadcast is a best-effort thing, the QBFT // cluster as a whole can progress further even if our round-change message cannot be created/broadcast // for whatever reason. i.bumpToRound(msgRound) + i.State.ProposalAcceptedForCurrentRound = msg + r := qbft.HashDataRoot(msg.SignedMessage.FullData) prepare, err := i.CreatePrepare(msgRound, r) if err != nil { diff --git a/protocol/v2/qbft/instance/round_change.go b/protocol/v2/qbft/instance/round_change.go index 48ee153b93..580e278cd3 100644 --- a/protocol/v2/qbft/instance/round_change.go +++ b/protocol/v2/qbft/instance/round_change.go @@ -115,7 +115,6 @@ func (i *Instance) uponChangeRoundPartialQuorum(ctx context.Context, logger *zap // cluster as a whole can progress further even if our round-change message cannot be created/broadcast // for whatever reason. i.bumpToRound(newRound) - i.State.ProposalAcceptedForCurrentRound = nil startValueRoot := qbft.HashDataRoot(i.StartValue) logger = logger.With(zap.String("qbft_start_value_root", hex.EncodeToString(startValueRoot[:]))) diff --git a/protocol/v2/qbft/instance/timeout.go b/protocol/v2/qbft/instance/timeout.go index a32f57b9eb..140eeb9207 100644 --- a/protocol/v2/qbft/instance/timeout.go +++ b/protocol/v2/qbft/instance/timeout.go @@ -37,7 +37,6 @@ func (i *Instance) UponRoundTimeout(ctx context.Context, logger *zap.Logger) err // cluster as a whole can progress further even if our round-change message cannot be created/broadcast // for whatever reason. i.bumpToRound(newRound) - i.State.ProposalAcceptedForCurrentRound = nil roundChange, err := i.CreateRoundChange(newRound) if err != nil { diff --git a/protocol/v2/qbft/messages_test.go b/protocol/v2/qbft/messages_test.go new file mode 100644 index 0000000000..74cc11033f --- /dev/null +++ b/protocol/v2/qbft/messages_test.go @@ -0,0 +1,21 @@ +package qbft + +import ( + "testing" + + specqbft "github.com/ssvlabs/ssv-spec/qbft" + "github.com/stretchr/testify/require" +) + +func TestHashDataRootMatchesSpec(t *testing.T) { + for _, data := range [][]byte{ + nil, + {}, + []byte("qbft-value"), + make([]byte, 256), + } { + specRoot, err := specqbft.HashDataRoot(data) + require.NoError(t, err) + require.Equal(t, specRoot, HashDataRoot(data)) + } +} From 5ff4c098a9a76c2da1aa72385e83311bd4348714 Mon Sep 17 00:00:00 2001 From: iurii Date: Tue, 12 May 2026 14:39:14 +0300 Subject: [PATCH 3/4] qbft: bump leader to justified round before proposing hasReceivedProposalJustificationForLeadingRound accepts future-round RC quorums, but CreateProposal uses i.State.Round and MessagesForRound(i.State.Round) selects the round-change justifications - without bumping first, the leader would broadcast a stale-round proposal with empty justifications. The case is structurally unreachable via natural message flow (partial-quorum f+1 fires before full quorum 2f+1 on each uponRoundChange call), so the fix closes a latent inconsistency rather than a live bug. Test bypasses natural flow by preloading the container, kept as defense-in-depth. --- protocol/v2/qbft/instance/round_change.go | 5 +++ .../v2/qbft/instance/round_change_test.go | 34 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/protocol/v2/qbft/instance/round_change.go b/protocol/v2/qbft/instance/round_change.go index 580e278cd3..51e095d6a1 100644 --- a/protocol/v2/qbft/instance/round_change.go +++ b/protocol/v2/qbft/instance/round_change.go @@ -57,6 +57,11 @@ func (i *Instance) uponRoundChange( i.metrics.EndStage(ctx, i.State.Round) i.metrics.StartStage(stageProposal) + // hasReceivedProposalJustificationForLeadingRound accepts future-round quorums, but CreateProposal + // uses i.State.Round as the proposal round and MessagesForRound(i.State.Round) selects the + // round-change justifications - so bump to the justified round here before building the proposal. + i.bumpToRound(justifiedRoundChangeMsg.QBFTMessage.Round) + roundChangeJustificationSignedMessages, _ := justifiedRoundChangeMsg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, check on isValidRoundChange roundChangeJustification := make([]*specqbft.ProcessingMessage, 0) diff --git a/protocol/v2/qbft/instance/round_change_test.go b/protocol/v2/qbft/instance/round_change_test.go index 7f207883fa..4a9770a4f4 100644 --- a/protocol/v2/qbft/instance/round_change_test.go +++ b/protocol/v2/qbft/instance/round_change_test.go @@ -227,6 +227,40 @@ func TestUponRoundChangeAsLeaderBroadcastsProposalOnJustifiedQuorum(t *testing.T require.Equal(t, env.hash(env.inst.StartValue), msg.QBFTMessage.Root) } +func TestUponRoundChangeAsLeaderBroadcastsFutureRoundProposalOnJustifiedQuorum(t *testing.T) { + env := newInstanceTestEnv(t, 1) + env.setLeader(1) + env.inst.State.Round = 1 + env.inst.StartValue = []byte("start-value") + + // Preload container to exercise the future-round leader path; this path is unreachable via + // natural message flow (partial-quorum f+1 fires before full quorum 2f+1 in uponRoundChange), + // kept as defense-in-depth against future refactors. + env.addMessages( + env.inst.State.RoundChangeContainer, + env.roundChange(2, 2, specqbft.NoRound, [32]byte{}, nil, nil), + env.roundChange(2, 3, specqbft.NoRound, [32]byte{}, nil, nil), + ) + + err := env.inst.uponRoundChange( + context.Background(), + zap.NewNop(), + env.roundChange(2, 4, specqbft.NoRound, [32]byte{}, nil, nil), + ) + require.NoError(t, err) + + require.Equal(t, specqbft.Round(2), env.inst.State.Round) + + msg := env.broadcastedProcessingMessage(0) + require.Equal(t, specqbft.ProposalMsgType, msg.QBFTMessage.MsgType) + require.Equal(t, specqbft.Round(2), msg.QBFTMessage.Round) + require.Equal(t, env.hash(env.inst.StartValue), msg.QBFTMessage.Root) + + roundChangeJustifications, err := msg.QBFTMessage.GetRoundChangeJustifications() + require.NoError(t, err) + require.Len(t, roundChangeJustifications, 3) +} + func TestValidRoundChangeForDataIgnoreSignatureValidationBranches(t *testing.T) { env := newInstanceTestEnv(t, 2) fullData := []byte("prepared-value") From 8db85bb5b74ac1f47187bd847c35a3f23207b1ba Mon Sep 17 00:00:00 2001 From: iurii Date: Tue, 12 May 2026 15:06:26 +0300 Subject: [PATCH 4/4] qbft: note that justified-round bump is a no-op in practice The partial-quorum branch (f+1 < 2f+1) always fires on an earlier uponRoundChange call and advances State.Round before the leader branch can see a full quorum for that round - so bumpToRound here is always a no-op on natural message flow. Kept as defense-in-depth. --- protocol/v2/qbft/instance/round_change.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/protocol/v2/qbft/instance/round_change.go b/protocol/v2/qbft/instance/round_change.go index 51e095d6a1..46f111e627 100644 --- a/protocol/v2/qbft/instance/round_change.go +++ b/protocol/v2/qbft/instance/round_change.go @@ -60,6 +60,11 @@ func (i *Instance) uponRoundChange( // hasReceivedProposalJustificationForLeadingRound accepts future-round quorums, but CreateProposal // uses i.State.Round as the proposal round and MessagesForRound(i.State.Round) selects the // round-change justifications - so bump to the justified round here before building the proposal. + // + // In practice this branch never observes a future-round quorum: on natural message flow, the + // partial-quorum branch (f+1 < 2f+1) fires on an earlier uponRoundChange call and advances + // State.Round before this branch can see a full quorum, so bumpToRound here is always a no-op. + // Kept as defense-in-depth in case the message-processing flow changes. i.bumpToRound(justifiedRoundChangeMsg.QBFTMessage.Round) roundChangeJustificationSignedMessages, _ := justifiedRoundChangeMsg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, check on isValidRoundChange