diff --git a/message/validation/consensus_validation.go b/message/validation/consensus_validation.go index f3e9db31f2..17f8ea901e 100644 --- a/message/validation/consensus_validation.go +++ b/message/validation/consensus_validation.go @@ -17,7 +17,6 @@ import ( "github.com/ssvlabs/ssv/protocol/v2/message" "github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer" - ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" "github.com/ssvlabs/ssv/utils/casts" ) @@ -82,48 +81,92 @@ func (mv *messageValidator) validateConsensusMessageSemantics( committee []spectypes.OperatorID, ) error { signers := signedSSVMessage.OperatorIDs - quorumSize, _ := ssvtypes.ComputeQuorumAndPartialQuorum(uint64(len(committee))) msgType := consensusMessage.MsgType - if len(signers) > 1 { - // Rule: Decided msg with different type than Commit - if msgType != specqbft.CommitMsgType { - e := ErrNonDecidedWithMultipleSigners - e.got = len(signers) - return e - } - - // Rule: Number of signers must be >= quorum size - if uint64(len(signers)) < quorumSize { - e := ErrDecidedNotEnoughSigners - e.want = quorumSize - e.got = len(signers) - return e - } + // Rule: Consensus message type must be valid + if !mv.validConsensusMsgType(msgType) { + return ErrUnknownQBFTMessageType } - if len(signedSSVMessage.FullData) != 0 { - // Rule: Prepare or commit messages must not have full data - if msgType == specqbft.PrepareMsgType || (msgType == specqbft.CommitMsgType && len(signers) == 1) { - return ErrPrepareOrCommitWithFullData + if msgType == specqbft.ProposalMsgType { + // Rule: Proposal message must have exactly 1 signer + if err := ensureSingleSignerAtMost(signers); err != nil { + return fmt.Errorf("proposal message with > 1 signer: %w", err) } - - hashedFullData, err := specqbft.HashDataRoot(signedSSVMessage.FullData) - if err != nil { - e := ErrFullDataHash - e.innerErr = err - return e + // Rule: Proposal message must have full data + if len(signedSSVMessage.FullData) == 0 { + return ErrProposalWithoutFullData } - // Rule: Full data hash must match root - if hashedFullData != consensusMessage.Root { - return ErrInvalidHash + if err := verifyRootAgainstFullData(consensusMessage.Root, signedSSVMessage.FullData); err != nil { + return err } } - - // Rule: Consensus message type must be valid - if !mv.validConsensusMsgType(msgType) { - return ErrUnknownQBFTMessageType + if msgType == specqbft.PrepareMsgType { + // Rule: Prepare message must have exactly 1 signer + if err := ensureSingleSignerAtMost(signers); err != nil { + return fmt.Errorf("prepare message with > 1 signer: %w", err) + } + // Rule: Prepare message must not have full data + if len(signedSSVMessage.FullData) != 0 { + return ErrPrepareWithFullData + } + // Rule: Prepare message must have non-zero root (operator validates it against locally + // stored proposal) + if consensusMessage.Root == [32]byte{} { + return ErrZeroRootHash + } + } + if msgType == specqbft.CommitMsgType && len(signers) == 1 { + // Rule: Commit message (unless it's decided commit message, which is handled below) + // must not have full data + if len(signedSSVMessage.FullData) != 0 { + return ErrCommitWithFullData + } + // Rule: Commit message (unless it's decided commit message, which is handled below) + // must have non-zero root (operator validates it against locally stored proposal) + if consensusMessage.Root == [32]byte{} { + return ErrZeroRootHash + } + } + if msgType == specqbft.CommitMsgType && len(signers) >= 2 { + // Rule: Commit message must have exactly 1 signer + if err := ensureSingleSignerAtMost(signers); err != nil { + return fmt.Errorf("commit message with > 1 signer: %w", err) + } + // TODO in case we want to spread the data QBFT decided upon via QBFT commit phase we need + // to introduce the concept of "decided commit" as well as adjust the rule-set above to the + // change it to the rule-set commented out below. + // Note also, QBFT itself doesn't support this ^ at the moment and hence will not permit + // commit messages with >= 2 signers. + // + //// Rule: Commit message with multiple signers (aka decided commit) must have full data so + //// it can share it with those operator who didn't receive it during proposal stage of QBFT + //if len(signedSSVMessage.FullData) == 0 { + // return ErrDecidedCommitWithoutFullData + //} + //// Rule: Number of signers for decided commit must be >= quorum size + //quorumSize, _ := ssvtypes.ComputeQuorumAndPartialQuorum(uint64(len(committee))) + //if uint64(len(signers)) < quorumSize { + // e := ErrCommitSignersNotEnoughForQuorum + // e.want = quorumSize + // e.got = len(signers) + // return e + //} + //// Rule: Full data hash must match root + //if err := verifyRootAgainstFullData(consensusMessage.Root, signedSSVMessage.FullData); err != nil { + // return err + //} + } + if msgType == specqbft.RoundChangeMsgType { + // Rule: Round change message must have exactly 1 signer + if err := ensureSingleSignerAtMost(signers); err != nil { + return fmt.Errorf("round change message with > 1 signer: %w", err) + } + // Rule: Full data hash must match root + if err := verifyRootAgainstFullData(consensusMessage.Root, signedSSVMessage.FullData); err != nil { + return err + } } // Rule: Round must not be zero @@ -291,6 +334,32 @@ func (mv *messageValidator) validateQBFTMessageByDutyLogic( return nil } +func ensureSingleSignerAtMost(signers []spectypes.OperatorID) error { + if len(signers) > 1 { + e := ErrMultipleSigners + e.got = len(signers) + e.want = 1 + return e + } + return nil +} + +func verifyRootAgainstFullData(root [32]byte, fullData []byte) error { + hashedFullData, err := specqbft.HashDataRoot(fullData) + if err != nil { + e := ErrComputeFullDataHash + e.innerErr = err + return e + } + if hashedFullData != root { + e := ErrInvalidRootHash + e.want = hashedFullData + e.got = root + return e + } + return nil +} + func (mv *messageValidator) updateConsensusState(signedSSVMessage *spectypes.SignedSSVMessage, consensusMessage *specqbft.Message, consensusState *consensusState) error { msgSlot := phase0.Slot(consensusMessage.Height) msgEpoch := mv.netCfg.Beacon.EstimatedEpochAtSlot(msgSlot) diff --git a/message/validation/errors.go b/message/validation/errors.go index c1a90997f3..307b6ec225 100644 --- a/message/validation/errors.go +++ b/message/validation/errors.go @@ -14,12 +14,16 @@ import ( ) type Error struct { - text string + // text is the main/core factor defining this error + text string + // reject signals if we want to reject incoming message based on this error + reject bool + got any want any innerErr error - reject bool - silent bool + + silent bool } func (e Error) Error() string { @@ -39,6 +43,17 @@ func (e Error) Error() string { return sb.String() } +// Is defines the equivalency of Error (compared to other errors), 2 Error(s) are same if +// they have matching Error.text and Error.reject values. +func (e Error) Is(target error) bool { + var targetErr Error + ok := errors.As(target, &targetErr) + if !ok { + return false + } + return targetErr.text == e.text && targetErr.reject == e.reject +} + func (e Error) Reject() bool { return e.reject } @@ -88,16 +103,17 @@ var ( ErrSignerNotLeader = Error{text: "signer is not leader", reject: true} 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} + ErrZeroRootHash = Error{text: "root hash is zero", reject: true} + ErrInvalidRootHash = Error{text: "root doesn't match full data hash", reject: true} + ErrComputeFullDataHash = 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} ErrUnknownQBFTMessageType = Error{text: "unknown QBFT message type", reject: true} ErrInvalidPartialSignatureType = Error{text: "unknown partial signature message type", reject: true} ErrPartialSignatureTypeRoleMismatch = Error{text: "partial signature type and role don't match", reject: true} - ErrNonDecidedWithMultipleSigners = Error{text: "non-decided with multiple signers", reject: true} - ErrDecidedNotEnoughSigners = Error{text: "not enough signers in decided message", reject: true} + ErrMultipleSigners = Error{text: "message with multiple signers", reject: true} + ErrCommitSignersNotEnoughForQuorum = Error{text: "commit message has > 1 signers, but not enough for quorum", reject: true} ErrDifferentProposalData = Error{text: "different proposal data", reject: true} ErrMalformedPrepareJustifications = Error{text: "malformed prepare justifications", reject: true} ErrUnexpectedPrepareJustifications = Error{text: "prepare justifications unexpected for this message type", reject: true} @@ -108,8 +124,11 @@ var ( ErrNoSignatures = Error{text: "no signatures", reject: true} ErrSignersAndSignaturesWithDifferentLength = Error{text: "signature and operator ID length mismatch", reject: true} ErrPartialSigOneSigner = Error{text: "partial signature message must have only one signer", reject: true} - ErrPrepareOrCommitWithFullData = Error{text: "prepare or commit with full data", reject: true} - ErrFullDataNotInConsensusMessage = Error{text: "full data not in consensus message", reject: true} + ErrProposalWithoutFullData = Error{text: "proposal without full data", reject: true} + ErrPrepareWithFullData = Error{text: "prepare with full data", reject: true} + ErrCommitWithFullData = Error{text: "commit with full data", reject: true} + ErrDecidedCommitWithoutFullData = Error{text: "decided commit without full data", reject: true} + ErrFullDataNotInPartialSignatureMessage = Error{text: "full data not in partial signature message", reject: true} ErrTripleValidatorIndexInPartialSignatures = Error{text: "triple validator index in partial signatures", reject: true} ErrZeroRound = Error{text: "zero round", reject: true} ErrDuplicatedMessage = Error{text: "message is duplicated", reject: true} diff --git a/message/validation/partial_validation.go b/message/validation/partial_validation.go index 5a6006696a..0ea4d00929 100644 --- a/message/validation/partial_validation.go +++ b/message/validation/partial_validation.go @@ -79,7 +79,7 @@ func (mv *messageValidator) validatePartialSignatureMessageSemantics( // Rule: Partial signature message must not have full data if len(signedSSVMessage.FullData) > 0 { - return ErrFullDataNotInConsensusMessage + return ErrFullDataNotInPartialSignatureMessage } // Rule: Valid signature type diff --git a/message/validation/validation_test.go b/message/validation/validation_test.go index 8376cd8b25..ec4225f632 100644 --- a/message/validation/validation_test.go +++ b/message/validation/validation_test.go @@ -212,12 +212,13 @@ func Test_ValidateSSVMessage(t *testing.T) { _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt.Add(netCfg.Beacon.SlotDurationSec())) require.ErrorContains(t, err, ErrDuplicatedMessage.Error()) - signedSSVMessage = generateMultiSignedMessage(ks, msgID, slot+1) - _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt.Add(netCfg.Beacon.SlotDurationSec())) - require.NoError(t, err) - require.NotNil(t, stateBySlot) - require.EqualValues(t, 1, storedState.Round) - require.EqualValues(t, MessageCounts{Commit: 1}, storedState.MessageCounts) + // TODO - adjust + //signedSSVMessage = generateMultiSignedMessage(ks, msgID, slot+1) + //_, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt.Add(netCfg.Beacon.SlotDurationSec())) + //require.NoError(t, err) + //require.NotNil(t, stateBySlot) + //require.EqualValues(t, 1, storedState.Round) + //require.EqualValues(t, MessageCounts{Commit: 1}, storedState.MessageCounts) }) // Send a pubsub message with no data should cause an error @@ -993,21 +994,22 @@ func Test_ValidateSSVMessage(t *testing.T) { require.ErrorContains(t, err, ErrSignersAndSignaturesWithDifferentLength.Error()) }) - // Get error when receiving message from less than quorum size amount of signers - t.Run("decided too few signers", func(t *testing.T) { - validator := New(netCfg, validatorStore, dutyStore, signatureVerifier).(*messageValidator) - - slot := netCfg.Beacon.FirstSlotAtEpoch(1) - signedSSVMessage := generateMultiSignedMessage(ks, committeeIdentifier, slot) - signedSSVMessage.OperatorIDs = []spectypes.OperatorID{1, 2} - signedSSVMessage.Signatures = signedSSVMessage.Signatures[:2] - - receivedAt := netCfg.Beacon.GetSlotStartTime(slot) - topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] - _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - - require.ErrorContains(t, err, ErrDecidedNotEnoughSigners.Error()) - }) + // TODO - adjust + //// Get error when receiving message from less than quorum size amount of signers + //t.Run("decided too few signers", func(t *testing.T) { + // validator := New(netCfg, validatorStore, dutyStore, signatureVerifier).(*messageValidator) + // + // slot := netCfg.Beacon.FirstSlotAtEpoch(1) + // signedSSVMessage := generateMultiSignedMessage(ks, committeeIdentifier, slot) + // signedSSVMessage.OperatorIDs = []spectypes.OperatorID{1, 2} + // signedSSVMessage.Signatures = signedSSVMessage.Signatures[:2] + // + // receivedAt := netCfg.Beacon.GetSlotStartTime(slot) + // topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] + // _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) + // + // require.ErrorContains(t, err, ErrCommitSignersNotEnoughForQuorum.Error()) + //}) // Get error when receiving a non decided message with multiple signers t.Run("non decided with multiple signers", func(t *testing.T) { @@ -1022,8 +1024,9 @@ func Test_ValidateSSVMessage(t *testing.T) { topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - expectedErr := ErrNonDecidedWithMultipleSigners + expectedErr := ErrMultipleSigners expectedErr.got = 3 + expectedErr.want = 1 require.ErrorIs(t, err, expectedErr) }) @@ -1167,7 +1170,6 @@ func Test_ValidateSSVMessage(t *testing.T) { signedSSVMessage := generateSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { message.RoundChangeJustification = [][]byte{{1}} }) - signedSSVMessage.FullData = nil receivedAt := netCfg.Beacon.GetSlotStartTime(slot) topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] @@ -1189,8 +1191,7 @@ func Test_ValidateSSVMessage(t *testing.T) { topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - expectedErr := ErrInvalidHash - require.ErrorIs(t, err, expectedErr) + require.ErrorIs(t, err, ErrInvalidRootHash) }) // Receive proposal from same operator twice with different messages (same round) should receive an error @@ -1272,40 +1273,48 @@ func Test_ValidateSSVMessage(t *testing.T) { signedSSVMessage := generateSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { message.MsgType = specqbft.RoundChangeMsgType }) - signedSSVMessage.FullData = nil receivedAt := netCfg.Beacon.GetSlotStartTime(slot) topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) require.NoError(t, err) + // duplicate round change msg with same data _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) expectedErr := ErrDuplicatedMessage - expectedErr.got = "round change, having pre-consensus: 0, proposal: 0, prepare: 0, commit: 0, round change: 1, post-consensus: 0" require.ErrorIs(t, err, expectedErr) - }) - // Decided with same signers should receive an error - t.Run("decided with same signers", func(t *testing.T) { - validator := New(netCfg, validatorStore, dutyStore, signatureVerifier).(*messageValidator) - - slot := netCfg.Beacon.FirstSlotAtEpoch(1) - - signedSSVMessage := generateMultiSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { - message.MsgType = specqbft.CommitMsgType + // duplicate round change msg with different data + signedSSVMessage = generateSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { + message.MsgType = specqbft.RoundChangeMsgType + message.DataRound += 1 }) - signedSSVMessage.FullData = nil - - receivedAt := netCfg.Beacon.GetSlotStartTime(slot) - topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] - _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - require.NoError(t, err) - - _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - require.ErrorIs(t, err, ErrDecidedWithSameSigners) + expectedErr = ErrDuplicatedMessage + require.ErrorIs(t, err, expectedErr) }) + // TODO - adjust + //// Decided with same signers should receive an error + //t.Run("decided with same signers", func(t *testing.T) { + // validator := New(netCfg, validatorStore, dutyStore, signatureVerifier).(*messageValidator) + // + // slot := netCfg.Beacon.FirstSlotAtEpoch(1) + // + // signedSSVMessage := generateMultiSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { + // message.MsgType = specqbft.CommitMsgType + // }) + // + // receivedAt := netCfg.Beacon.GetSlotStartTime(slot) + // topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] + // + // _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) + // require.NoError(t, err) + // + // _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) + // require.ErrorIs(t, err, ErrDecidedWithSameSigners) + //}) + // Send message with a slot lower than in the previous message t.Run("slot already advanced", func(t *testing.T) { validator := New(netCfg, validatorStore, dutyStore, signatureVerifier).(*messageValidator) @@ -1564,13 +1573,13 @@ func Test_ValidateSSVMessage(t *testing.T) { receivedAt := netCfg.Beacon.GetSlotStartTime(slot) topicID := commons.CommitteeTopicID(spectypes.CommitteeID(signedSSVMessage.SSVMessage.GetID().GetDutyExecutorID()[16:]))[0] _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - require.ErrorContains(t, err, ErrPrepareOrCommitWithFullData.Error()) + require.ErrorContains(t, err, ErrPrepareWithFullData.Error()) signedSSVMessage = generateSignedMessage(ks, committeeIdentifier, slot, func(message *specqbft.Message) { message.MsgType = specqbft.CommitMsgType }) _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - require.ErrorContains(t, err, ErrPrepareOrCommitWithFullData.Error()) + require.ErrorContains(t, err, ErrCommitWithFullData.Error()) }) // Receive a non-consensus message with FullData @@ -1585,7 +1594,7 @@ func Test_ValidateSSVMessage(t *testing.T) { receivedAt := netCfg.Beacon.GetSlotStartTime(slot) topicID := commons.CommitteeTopicID(committeeID)[0] _, err = validator.handleSignedSSVMessage(signedSSVMessage, topicID, receivedAt) - require.ErrorIs(t, err, ErrFullDataNotInConsensusMessage) + require.ErrorIs(t, err, ErrFullDataNotInPartialSignatureMessage) }) // Receive a partial signature message with multiple signers @@ -1826,6 +1835,7 @@ func generateSignedMessage( opts ...func(message *specqbft.Message), ) *spectypes.SignedSSVMessage { fullData := spectestingutils.TestingQBFTFullData + root := spectestingutils.TestingQBFTRootData height := specqbft.Height(slot) qbftMessage := &specqbft.Message{ @@ -1833,7 +1843,7 @@ func generateSignedMessage( Height: height, Round: specqbft.FirstRound, Identifier: identifier[:], - Root: sha256.Sum256(fullData), + Root: root, RoundChangeJustification: [][]byte{}, PrepareJustification: [][]byte{}, diff --git a/protocol/genesis/qbft/config.go b/protocol/genesis/qbft/config.go index 942c5853ad..f3c519b966 100644 --- a/protocol/genesis/qbft/config.go +++ b/protocol/genesis/qbft/config.go @@ -19,7 +19,7 @@ type IConfig interface { signing // GetValueCheckF returns value check function GetValueCheckF() genesisspecqbft.ProposedValueCheckF - // GetProposerF returns func used to calculate proposer + // GetProposerF returns func used to calculate proposer(leader) GetProposerF() genesisspecqbft.ProposerF // GetNetwork returns a p2p Network instance GetNetwork() genesisspecqbft.Network @@ -60,7 +60,7 @@ func (c *Config) GetValueCheckF() genesisspecqbft.ProposedValueCheckF { return c.ValueCheckF } -// GetProposerF returns func used to calculate proposer +// GetProposerF returns func used to calculate proposer(leader) func (c *Config) GetProposerF() genesisspecqbft.ProposerF { return c.ProposerF } diff --git a/protocol/genesis/qbft/instance/prepare.go b/protocol/genesis/qbft/instance/prepare.go index 54329a63f9..97e025ac02 100644 --- a/protocol/genesis/qbft/instance/prepare.go +++ b/protocol/genesis/qbft/instance/prepare.go @@ -103,29 +103,6 @@ func getRoundChangeJustification(state *genesisspecqbft.State, config qbft.IConf return ret, nil } -// validPreparesForHeightRoundAndValue returns an aggregated prepare msg for a specific Height and round -// func validPreparesForHeightRoundAndValue( -// config IConfig, -// prepareMessages []*SignedMessage, -// height Height, -// round Round, -// value []byte, -// operators []*types.Operator) *SignedMessage { -// var aggregatedPrepareMsg *SignedMessage -// for _, signedMsg := range prepareMessages { -// if err := validSignedPrepareForHeightRoundAndValue(config, signedMsg, height, round, value, operators); err == nil { -// if aggregatedPrepareMsg == nil { -// aggregatedPrepareMsg = signedMsg -// } else { -// // TODO: check error -// // nolint -// aggregatedPrepareMsg.Aggregate(signedMsg) -// } -// } -// } -// return aggregatedPrepareMsg -// } - // validSignedPrepareForHeightRoundAndValue known in dafny spec as validSignedPrepareForHeightRoundAndDigest // https://entethalliance.github.io/client-spec/qbft_spec.html#dfn-qbftspecification func validSignedPrepareForHeightRoundAndRoot( @@ -134,7 +111,8 @@ func validSignedPrepareForHeightRoundAndRoot( height genesisspecqbft.Height, round genesisspecqbft.Round, root [32]byte, - operators []*genesisspectypes.Operator) error { + operators []*genesisspectypes.Operator, +) error { if signedPrepare.Message.MsgType != genesisspecqbft.PrepareMsgType { return errors.New("prepare msg type is wrong") } diff --git a/protocol/genesis/qbft/instance/proposal.go b/protocol/genesis/qbft/instance/proposal.go index 29ed1b5401..496a789811 100644 --- a/protocol/genesis/qbft/instance/proposal.go +++ b/protocol/genesis/qbft/instance/proposal.go @@ -46,7 +46,7 @@ func (i *Instance) uponProposal(logger *zap.Logger, signedProposal *genesisspecq return errors.Wrap(err, "could not hash input data") } - prepare, err := CreatePrepare(i.State, i.config, newRound, r) + prepareMsg, err := CreatePrepare(i.State, i.config, newRound, r) if err != nil { return errors.Wrap(err, "could not create prepare msg") } @@ -54,9 +54,10 @@ func (i *Instance) uponProposal(logger *zap.Logger, signedProposal *genesisspecq logger.Debug("📢 got proposal, broadcasting prepare message", fields.Round(specqbft.Round(i.State.Round)), zap.Any("proposal_signers", signedProposal.Signers), - zap.Any("prepare_signers", prepare.Signers)) + zap.Any("prepare_signers", prepareMsg.Signers), + fields.Root(prepareMsg.Message.Root)) - if err := i.Broadcast(logger, prepare); err != nil { + if err := i.Broadcast(logger, prepareMsg); err != nil { return errors.Wrap(err, "failed to broadcast prepare message") } return nil diff --git a/protocol/genesis/qbft/instance/round_change.go b/protocol/genesis/qbft/instance/round_change.go index e65f4bb434..6a8959a964 100644 --- a/protocol/genesis/qbft/instance/round_change.go +++ b/protocol/genesis/qbft/instance/round_change.go @@ -99,7 +99,7 @@ func (i *Instance) uponChangeRoundPartialQuorum(logger *zap.Logger, newRound gen i.config.GetTimer().TimeoutForRound(i.State.Height, i.State.Round) - roundChange, err := CreateRoundChange(i.State, i.config, newRound, instanceStartValue) + roundChange, err := CreateRoundChange(i.State, i.config, newRound) if err != nil { return errors.Wrap(err, "failed to create round change message") } @@ -152,7 +152,6 @@ func hasReceivedProposalJustificationForLeadingRound( // Important! // We iterate on all round chance msgs for liveliness in case the last round change msg is malicious. for _, msg := range roundChanges { - // Chose proposal value. // If justifiedRoundChangeMsg has no prepare justification chose state value // If justifiedRoundChangeMsg has prepare justification chose prepared value @@ -337,7 +336,7 @@ func minRound(roundChangeMsgs []*genesisspecqbft.SignedMessage) genesisspecqbft. return ret } -func getRoundChangeData(state *genesisspecqbft.State, config qbft.IConfig, instanceStartValue []byte) (genesisspecqbft.Round, [32]byte, []byte, []*genesisspecqbft.SignedMessage, error) { +func getRoundChangeData(state *genesisspecqbft.State, config qbft.IConfig) (genesisspecqbft.Round, [32]byte, []byte, []*genesisspecqbft.SignedMessage, error) { if state.LastPreparedRound != genesisspecqbft.NoRound && state.LastPreparedValue != nil { justifications, err := getRoundChangeJustification(state, config, state.PrepareContainer) if err != nil { @@ -368,12 +367,17 @@ RoundChange( getRoundChangeJustification(current) ) */ -func CreateRoundChange(state *genesisspecqbft.State, config qbft.IConfig, newRound genesisspecqbft.Round, instanceStartValue []byte) (*genesisspecqbft.SignedMessage, error) { - round, root, fullData, justifications, err := getRoundChangeData(state, config, instanceStartValue) +func CreateRoundChange(state *genesisspecqbft.State, config qbft.IConfig, newRound genesisspecqbft.Round) (*genesisspecqbft.SignedMessage, error) { + round, root, fullData, justifications, err := getRoundChangeData(state, config) if err != nil { return nil, errors.Wrap(err, "could not generate round change data") } + // TODO + // so we can have fullData==nil and root==0 here + // - if current state.LastPreparedValue == nil + // - or state.LastPreparedRound != 0 + justificationsData, err := genesisspecqbft.MarshalJustifications(justifications) if err != nil { return nil, errors.Wrap(err, "could not marshal justifications") diff --git a/protocol/genesis/qbft/instance/timeout.go b/protocol/genesis/qbft/instance/timeout.go index 7232c627b4..348a4c99a5 100644 --- a/protocol/genesis/qbft/instance/timeout.go +++ b/protocol/genesis/qbft/instance/timeout.go @@ -28,19 +28,19 @@ func (i *Instance) UponRoundTimeout(logger *zap.Logger) error { i.config.GetTimer().TimeoutForRound(i.State.Height, i.State.Round) }() - roundChange, err := CreateRoundChange(i.State, i.config, newRound, i.StartValue) + roundChangeMsg, err := CreateRoundChange(i.State, i.config, newRound) if err != nil { return errors.Wrap(err, "could not generate round change msg") } logger.Debug("📢 broadcasting round change message", fields.Round(specqbft.Round(i.State.Round)), - fields.Root(roundChange.Message.Root), - zap.Any("round_change_signers", roundChange.Signers), + fields.Root(roundChangeMsg.Message.Root), + zap.Any("round_change_signers", roundChangeMsg.Signers), fields.Height(specqbft.Height(i.State.Height)), zap.String("reason", "timeout")) - if err := i.Broadcast(logger, roundChange); err != nil { + if err := i.Broadcast(logger, roundChangeMsg); err != nil { return errors.Wrap(err, "failed to broadcast round change message") } diff --git a/protocol/v2/qbft/config.go b/protocol/v2/qbft/config.go index 1d356fd582..bf0780a111 100644 --- a/protocol/v2/qbft/config.go +++ b/protocol/v2/qbft/config.go @@ -21,7 +21,7 @@ type IConfig interface { signing // GetValueCheckF returns value check function GetValueCheckF() specqbft.ProposedValueCheckF - // GetProposerF returns func used to calculate proposer + // GetProposerF returns func used to calculate proposer(leader) GetProposerF() specqbft.ProposerF // GetNetwork returns a p2p Network instance GetNetwork() specqbft.Network @@ -59,7 +59,7 @@ func (c *Config) GetValueCheckF() specqbft.ProposedValueCheckF { return c.ValueCheckF } -// GetProposerF returns func used to calculate proposer +// GetProposerF returns func used to calculate proposer(leader) func (c *Config) GetProposerF() specqbft.ProposerF { return c.ProposerF } diff --git a/protocol/v2/qbft/instance/commit.go b/protocol/v2/qbft/instance/commit.go index 2b905ea249..8d2eeca4b1 100644 --- a/protocol/v2/qbft/instance/commit.go +++ b/protocol/v2/qbft/instance/commit.go @@ -2,6 +2,7 @@ package instance import ( "bytes" + "fmt" "sort" "github.com/pkg/errors" @@ -183,20 +184,28 @@ func validateCommit( proposedMsg *specqbft.ProcessingMessage, operators []*spectypes.Operator, ) error { + if proposedMsg == nil { + return fmt.Errorf("did not receive proposal for round: %d", round) + } + if err := baseCommitValidationIgnoreSignature(msg, height, operators); err != nil { return err } - if len(msg.SignedMessage.OperatorIDs) != 1 { - return errors.New("msg allows 1 signer") + signerCnt := len(msg.SignedMessage.OperatorIDs) + if signerCnt != 1 { + return fmt.Errorf("msg must have exactly 1 signer, got: %d", signerCnt) } - if msg.QBFTMessage.Round != round { - return errors.New("wrong msg round") + msgRound := msg.QBFTMessage.Round + if msgRound != round { + return fmt.Errorf("wrong msg round: %d, want: %d", msg.QBFTMessage.Round, round) } - if !bytes.Equal(proposedMsg.QBFTMessage.Root[:], msg.QBFTMessage.Root[:]) { - return errors.New("proposed data mismatch") + wantRoot := proposedMsg.QBFTMessage.Root[:] + gotRoot := msg.QBFTMessage.Root[:] + if !bytes.Equal(wantRoot, gotRoot) { + return fmt.Errorf("proposed data root mismatch, want: %s, got: %s", wantRoot, gotRoot) } return nil diff --git a/protocol/v2/qbft/instance/error.go b/protocol/v2/qbft/instance/error.go new file mode 100644 index 0000000000..de151e1489 --- /dev/null +++ b/protocol/v2/qbft/instance/error.go @@ -0,0 +1,9 @@ +package instance + +import "fmt" + +var ( + convertJustificationToProcessingMessageErr = fmt.Errorf("could not create ProcessingMessage from justification") + noJustifiedProposalErr = fmt.Errorf("no justified proposal for round change quorum") + noQuorumErr = fmt.Errorf("no quorum") +) diff --git a/protocol/v2/qbft/instance/instance.go b/protocol/v2/qbft/instance/instance.go index 67d688d32c..b38cc3e8a9 100644 --- a/protocol/v2/qbft/instance/instance.go +++ b/protocol/v2/qbft/instance/instance.go @@ -192,10 +192,6 @@ func (i *Instance) BaseMsgValidation(msg *specqbft.ProcessingMessage) error { i.State.CommitteeMember.Committee, ) case specqbft.CommitMsgType: - proposedMsg := i.State.ProposalAcceptedForCurrentRound - if proposedMsg == nil { - return errors.New("did not receive proposal for this round") - } return validateCommit( msg, i.State.Height, diff --git a/protocol/v2/qbft/instance/proposal.go b/protocol/v2/qbft/instance/proposal.go index af31dc1560..31b952a734 100644 --- a/protocol/v2/qbft/instance/proposal.go +++ b/protocol/v2/qbft/instance/proposal.go @@ -2,6 +2,7 @@ package instance import ( "bytes" + "fmt" "github.com/pkg/errors" specqbft "github.com/ssvlabs/ssv-spec/qbft" @@ -106,7 +107,7 @@ func isValidProposal( for _, rcSignedMessage := range roundChangeJustificationSignedMessages { rc, err := specqbft.NewProcessingMessage(rcSignedMessage) if err != nil { - return errors.Wrap(err, "could not create ProcessingMessage from round change justification") + return fmt.Errorf("%w: %s", convertJustificationToProcessingMessageErr, err) } roundChangeJustification = append(roundChangeJustification, rc) } @@ -114,7 +115,7 @@ func isValidProposal( for _, prepareSignedMessage := range prepareJustificationSignedMessages { procMsg, err := specqbft.NewProcessingMessage(prepareSignedMessage) if err != nil { - return errors.Wrap(err, "could not create ProcessingMessage from prepare justification") + return fmt.Errorf("%w: %s", convertJustificationToProcessingMessageErr, err) } prepareJustification = append(prepareJustification, procMsg) } @@ -163,7 +164,8 @@ func IsProposalJustification( ) } -// isProposalJustification returns nil if the proposal and round change messages are valid and justify a proposal message for the provided round, value and leader +// isProposalJustification returns nil if the proposal and round change messages are valid and +// justify a proposal message for the provided round, value and leader func isProposalJustification( state *specqbft.State, config qbft.IConfig, @@ -179,11 +181,13 @@ func isProposalJustification( } if round == specqbft.FirstRound { + // when validation the value to propose for the 1st round there is no previously prepared + // value (let alone justified prepared) to compare against, hence we are done here return nil } else { - // check all round changes are valid for height and round - // no quorum, duplicate signers, invalid still has quorum, invalid no quorum - // prepared + // check all round changes are valid for height and round, common error conditions are: + // no quorum, duplicate signers, invalid still has quorum, invalid no quorum for prepared + for _, rc := range roundChangeMsgs { if err := validRoundChangeForDataVerifySignature(state, config, rc, height, round, fullData); err != nil { return errors.Wrap(err, "change round msg not valid") @@ -211,7 +215,6 @@ func isProposalJustification( if !previouslyPrepared { return nil } else { - // check prepare quorum if !specqbft.HasQuorum(state.CommitteeMember, prepareMsgs) { return errors.New("prepares has no quorum") diff --git a/protocol/v2/qbft/instance/round_change.go b/protocol/v2/qbft/instance/round_change.go index cfcc4044c5..70d1f544a3 100644 --- a/protocol/v2/qbft/instance/round_change.go +++ b/protocol/v2/qbft/instance/round_change.go @@ -2,6 +2,8 @@ package instance import ( "bytes" + errs "errors" + "fmt" "github.com/pkg/errors" specqbft "github.com/ssvlabs/ssv-spec/qbft" @@ -46,60 +48,82 @@ func (i *Instance) uponRoundChange( fields.Root(msg.QBFTMessage.Root), zap.Any("round_change_signers", msg.SignedMessage.OperatorIDs)) + tryProcessPartialQuorum := func() error { + partialQuorum, rcs := hasReceivedPartialQuorum(i.State, roundChangeMsgContainer) + if !partialQuorum { + return nil + } + + newRound := minRound(rcs) + if newRound <= i.State.Round { + return nil // no need to advance round + } + return i.uponChangeRoundPartialQuorum(logger, newRound, instanceStartValue) + } + justifiedRoundChangeMsg, valueToPropose, err := hasReceivedProposalJustificationForLeadingRound( i.State, i.config, instanceStartValue, msg, roundChangeMsgContainer, - valCheck) - if err != nil { - return errors.Wrap(err, "could not get proposal justification for leading round") + valCheck, + ) + if errs.Is(err, noQuorumErr) { + return tryProcessPartialQuorum() + } else if errs.Is(err, noJustifiedProposalErr) { + logger.Debug( + "checked if we can lead round change", + fields.Root(msg.QBFTMessage.Root), + zap.Any("round_change_signers", msg.SignedMessage.OperatorIDs), + zap.Error(err), + ) + // we still want to process partial quorum flow below (even though we've got full quorum now) + return tryProcessPartialQuorum() + } else if err != nil { + return fmt.Errorf("could not get proposal justification for leading round: %w", err) } - if justifiedRoundChangeMsg != nil { - roundChangeJustificationSignedMessages, _ := justifiedRoundChangeMsg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, check on isValidRoundChange + // we've got justifiedRoundChangeMsg, lets process it (note, there won't be a need to process partial + // quorum if we made it here, we might not have done it before but that's ok because we'll move + // straight to proposal stage of qbft now) - roundChangeJustification := make([]*specqbft.ProcessingMessage, 0) - for _, rcSignedMessage := range roundChangeJustificationSignedMessages { - rc, err := specqbft.NewProcessingMessage(rcSignedMessage) - if err != nil { - return errors.Wrap(err, "could not create ProcessingMessage from round change justification") - } - roundChangeJustification = append(roundChangeJustification, rc) - } + roundChangeJustificationSignedMessages, _ := justifiedRoundChangeMsg.QBFTMessage.GetRoundChangeJustifications() // no need to check error, check on isValidRoundChange - proposal, err := CreateProposal( - i.State, - i.signer, - valueToPropose, - roundChangeMsgContainer.MessagesForRound(i.State.Round), // TODO - might be optimized to include only necessary quorum - roundChangeJustification, - ) + roundChangeJustification := make([]*specqbft.ProcessingMessage, 0) + for _, rcSignedMessage := range roundChangeJustificationSignedMessages { + rc, err := specqbft.NewProcessingMessage(rcSignedMessage) if err != nil { - return errors.Wrap(err, "failed to create proposal") + return fmt.Errorf("%w: %s", convertJustificationToProcessingMessageErr, err) } + roundChangeJustification = append(roundChangeJustification, rc) + } - r, _ := specqbft.HashDataRoot(valueToPropose) // TODO: err check although already happenes in createproposal + proposal, err := CreateProposal( + i.State, + i.signer, + valueToPropose, + roundChangeMsgContainer.MessagesForRound(i.State.Round), // TODO - might be optimized to include only necessary quorum + roundChangeJustification, + ) + if err != nil { + return errors.Wrap(err, "failed to create proposal") + } - logger.Debug("🔄 got justified round change, broadcasting proposal message", - fields.Round(i.State.Round), - zap.Any("round_change_signers", allSigners(roundChangeMsgContainer.MessagesForRound(i.State.Round))), - fields.Root(r)) + r, err := specqbft.HashDataRoot(valueToPropose) + if err != nil { + return fmt.Errorf("couldn't calculate data root hash: %w", err) + } - if err := i.Broadcast(logger, proposal); err != nil { - return errors.Wrap(err, "failed to broadcast proposal message") - } - } else if partialQuorum, rcs := hasReceivedPartialQuorum(i.State, roundChangeMsgContainer); partialQuorum { - newRound := minRound(rcs) - if newRound <= i.State.Round { - return nil // no need to advance round - } - err := i.uponChangeRoundPartialQuorum(logger, newRound, instanceStartValue) - if err != nil { - return err - } + logger.Debug("🔄 got justified round change, broadcasting proposal message", + fields.Round(i.State.Round), + zap.Any("round_change_signers", allSigners(roundChangeMsgContainer.MessagesForRound(i.State.Round))), + fields.Root(r)) + + if err := i.Broadcast(logger, proposal); err != nil { + return errors.Wrap(err, "failed to broadcast proposal message") } + return nil } @@ -160,9 +184,10 @@ func hasReceivedProposalJustificationForLeadingRound( roundChanges := roundChangeMsgContainer.MessagesForRound(roundChangeMessage.QBFTMessage.Round) // optimization, if no round change quorum can return false if !specqbft.HasQuorum(state.CommitteeMember, roundChanges) { - return nil, nil, nil + return nil, nil, noQuorumErr } + var combinedErr error // Important! // We iterate on all round chance msgs for liveliness in case the last round change msg is malicious. for _, containerRoundChangeMessage := range roundChanges { @@ -180,12 +205,12 @@ func hasReceivedProposalJustificationForLeadingRound( for _, signedMessage := range roundChangeSignedMessagesJustification { msg, err := specqbft.NewProcessingMessage(signedMessage) if err != nil { - return nil, nil, errors.Wrap(err, "could not create ProcessingMessage from round change justification") + return nil, nil, fmt.Errorf("%w: %s", convertJustificationToProcessingMessageErr, err) } roundChangeJustification = append(roundChangeJustification, msg) } - if isProposalJustificationForLeadingRound( + err := isProposalJustificationForLeadingRound( state, config, containerRoundChangeMessage, @@ -194,15 +219,23 @@ func hasReceivedProposalJustificationForLeadingRound( valueToPropose, valCheck, roundChangeMessage.QBFTMessage.Round, - ) == nil { - // not returning error, no need to - return containerRoundChangeMessage, valueToPropose, nil + ) + if err != nil { + combinedErr = errs.Join(combinedErr, fmt.Errorf( + "check proposal justificaiton for msg: %s, err: %w", + containerRoundChangeMessage.QBFTMessage.Identifier, + err, + )) + continue } + return containerRoundChangeMessage, valueToPropose, nil } - return nil, nil, nil + + return nil, nil, fmt.Errorf("%w: %s", noJustifiedProposalErr, combinedErr) } -// isProposalJustificationForLeadingRound - returns nil if we have a quorum of round change msgs and highest justified value for leading round +// isProposalJustificationForLeadingRound - returns nil if we have a quorum of round change msgs +// and highest justified value for leading round func isProposalJustificationForLeadingRound( state *specqbft.State, config qbft.IConfig, @@ -213,14 +246,16 @@ func isProposalJustificationForLeadingRound( valCheck specqbft.ProposedValueCheckF, newRound specqbft.Round, ) error { - if err := isReceivedProposalJustification( + if err := isProposalJustification( state, config, roundChanges, roundChangeJustifications, + roundChangeMsg.QBFTMessage.Height, roundChangeMsg.QBFTMessage.Round, value, - valCheck); err != nil { + valCheck, + ); err != nil { return err } @@ -238,30 +273,6 @@ func isProposalJustificationForLeadingRound( return nil } -// isReceivedProposalJustification - returns nil if we have a quorum of round change msgs and highest justified value -func isReceivedProposalJustification( - state *specqbft.State, - config qbft.IConfig, - roundChanges, prepares []*specqbft.ProcessingMessage, - newRound specqbft.Round, - value []byte, - valCheck specqbft.ProposedValueCheckF, -) error { - if err := isProposalJustification( - state, - config, - roundChanges, - prepares, - state.Height, - newRound, - value, - valCheck, - ); err != nil { - return errors.Wrap(err, "proposal not justified") - } - return nil -} - func validRoundChangeForDataIgnoreSignature( state *specqbft.State, config qbft.IConfig, @@ -306,7 +317,7 @@ func validRoundChangeForDataIgnoreSignature( for _, signedMessage := range prepareSignedMsgs { procMsg, err := specqbft.NewProcessingMessage(signedMessage) if err != nil { - return errors.Wrap(err, "could not create ProcessingMessage from prepare message in round change justification") + return fmt.Errorf("%w: %s", convertJustificationToProcessingMessageErr, err) } prepareMsgs = append(prepareMsgs, procMsg) }