diff --git a/message/validation/const_test.go b/message/validation/const_test.go index cac2f07dfa..7b254073bb 100644 --- a/message/validation/const_test.go +++ b/message/validation/const_test.go @@ -11,6 +11,9 @@ import ( // drifting below the pinned ssv-spec's worst-case message sizes. If this // fails after a spec bump, re-derive the corresponding const.go values. func TestSizeCapsCoverSpecWorstCase(t *testing.T) { + // The partial-signature cap is compared against the spec's full-SSVMessage-envelope + // constant, which is over-conservative: the cap applies to SSVMessage.Data, the inner + // encoding. require.GreaterOrEqual(t, maxEncodedPartialSignatureSize, maxmsgsize.MaxSizeSSVMessageFromPartialSignatureMessages) require.GreaterOrEqual(t, maxEncodedConsensusMsgSize, maxmsgsize.MaxSizeSSVMessageFromQBFTMessage) require.GreaterOrEqual(t, MaxEncodedMsgSize, maxmsgsize.MaxSizeSignedSSVMessageFromQBFTWith2Justification) diff --git a/message/validation/partial_validation_test.go b/message/validation/partial_validation_test.go new file mode 100644 index 0000000000..1160bacc0a --- /dev/null +++ b/message/validation/partial_validation_test.go @@ -0,0 +1,50 @@ +package validation + +import ( + "bytes" + "context" + "testing" + "time" + + spectypes "github.com/ssvlabs/ssv-spec/types" + "github.com/stretchr/testify/require" + + "github.com/ssvlabs/ssv/networkconfig" +) + +// TestPartialSignatureSizeCapEnforcedInValidation drives validatePartialSignatureMessage +// itself (not just the constant) with payloads around the cap, guarding that the size gate +// stays wired into the validation path: an oversized payload is rejected as too big, while +// one at the cap passes the size gate and only fails later, at decoding. +func TestPartialSignatureSizeCapEnforcedInValidation(t *testing.T) { + t.Parallel() + + mv := &messageValidator{netCfg: networkconfig.TestNetwork} + + t.Run("payload above the cap is rejected", func(t *testing.T) { + t.Parallel() + + signedSSVMessage := &spectypes.SignedSSVMessage{ + SSVMessage: &spectypes.SSVMessage{Data: bytes.Repeat([]byte{1}, maxEncodedPartialSignatureSize+1)}, + } + _, err := mv.validatePartialSignatureMessage(context.Background(), signedSSVMessage, CommitteeInfo{}, "", "", time.Time{}) + require.ErrorIs(t, err, ErrSSVDataTooBig) + + var valErr Error + require.ErrorAs(t, err, &valErr) + require.Equal(t, maxEncodedPartialSignatureSize, valErr.want) + }) + + t.Run("payload at the cap passes the size gate", func(t *testing.T) { + t.Parallel() + + signedSSVMessage := &spectypes.SignedSSVMessage{ + SSVMessage: &spectypes.SSVMessage{Data: bytes.Repeat([]byte{1}, maxEncodedPartialSignatureSize)}, + } + _, err := mv.validatePartialSignatureMessage(context.Background(), signedSSVMessage, CommitteeInfo{}, "", "", time.Time{}) + require.NotErrorIs(t, err, ErrSSVDataTooBig) + // The garbage payload fails at the next step, decoding — proof the size gate + // (not the content) made the difference between the two cases. + require.ErrorIs(t, err, ErrUndecodableMessageData) + }) +} diff --git a/observability/utils/format_test.go b/observability/utils/format_test.go index 827570c013..f729bf2f0e 100644 --- a/observability/utils/format_test.go +++ b/observability/utils/format_test.go @@ -6,6 +6,7 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" "github.com/stretchr/testify/require" + "github.com/ssvlabs/ssv/protocol/v2/message" ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" ) @@ -78,3 +79,47 @@ func TestFormatRunnerRole(t *testing.T) { require.NotEqual(t, FormatRunnerRole(ssvtypes.RoleAggregator), FormatRunnerRole(ssvtypes.RoleSyncCommitteeContribution)) }) } + +// TestRunnerRoleStringMappersLockstep guards the contract documented on +// ssvtypes.RunnerRoleToString and message.RunnerRoleToString: the two mappers are +// independent (one reaches the strings via the spec's String() plus a deprecated-role +// shim, the other via its own switch) and must produce the same string for every runner +// role that is valid in any fork. A role added or deprecated in one must be reflected in +// the other — this test is what fails when they drift. +func TestRunnerRoleStringMappersLockstep(t *testing.T) { + t.Parallel() + + // The full role union across forks, mirroring messageValidator.validRoleUnion. + roles := []spectypes.RunnerRole{ + spectypes.RoleCommittee, + spectypes.RoleAggregatorCommittee, + spectypes.RoleProposer, + spectypes.RoleValidatorRegistration, + spectypes.RoleVoluntaryExit, + ssvtypes.RoleAggregator, + ssvtypes.RoleSyncCommitteeContribution, + } + + for _, role := range roles { + require.Equal(t, message.RunnerRoleToString(role), FormatRunnerRole(role), + "role %d: message.RunnerRoleToString and utils.FormatRunnerRole disagree", role) + } + + // Sweep beyond the explicit list so a role added to the spec — which FormatRunnerRole + // picks up automatically via (RunnerRole).String() but message.RunnerRoleToString's + // hand-written switch would miss — fails here instead of drifting silently. The bound + // 15 is headroom over the spec's current max role value (6): roles are appended + // sequentially, so sweeping a few values past the end catches additions without the + // spec exporting a count. Roles the spec does not know return "UNDEFINED" and are + // skipped: divergence on genuinely unknown values is intentional (the deprecated Alan + // roles also stringify to "UNDEFINED" in the spec, but they are covered by the + // explicit list above). + for i := 0; i <= 15; i++ { + role := spectypes.RunnerRole(i) + if role.String() == "UNDEFINED" { + continue + } + require.Equal(t, message.RunnerRoleToString(role), FormatRunnerRole(role), + "role %d is known to the spec but the two mappers disagree", role) + } +} diff --git a/protocol/v2/message/msg_test.go b/protocol/v2/message/msg_test.go index 6c80cf2201..83946c8964 100644 --- a/protocol/v2/message/msg_test.go +++ b/protocol/v2/message/msg_test.go @@ -115,4 +115,23 @@ func TestRunnerRoleFromString_ToString_RoundTrip(t *testing.T) { require.NoError(t, err, "round-trip failed for role %v (string %q)", role, s) assert.Equal(t, role, got) } + + // Sweep spec-known role values beyond the hardcoded list, so a role added to the spec + // must gain a RunnerRoleFromString case as well: the lockstep sweep in + // observability/utils/format_test.go already forces a RunnerRoleToString case for it, + // and without this sweep FromString could silently stay behind — leaving + // CommitteeRunnerRoleFromString to reject the exporter's own emitted string. The + // bound and the skip mirror that sweep: 15 is headroom over the spec's current max + // role value (6), and values the spec stringifies as "UNDEFINED" (unused or + // deprecated) are covered by the explicit list above instead. + for i := 0; i <= 15; i++ { + role := spectypes.RunnerRole(i) + if role.String() == "UNDEFINED" { + continue + } + s := RunnerRoleToString(role) + got, err := RunnerRoleFromString(s) + require.NoError(t, err, "spec-known role %d (%q) does not round-trip", role, s) + assert.Equal(t, role, got) + } } diff --git a/protocol/v2/ssv/validator/committee.go b/protocol/v2/ssv/validator/committee.go index e735cdaa70..8c83c0b9f1 100644 --- a/protocol/v2/ssv/validator/committee.go +++ b/protocol/v2/ssv/validator/committee.go @@ -550,6 +550,9 @@ func (c *Committee) createRunner( if err != nil { return nil, fmt.Errorf("create committee runner: %w", err) } + if r == nil { + return nil, fmt.Errorf("BUG: CreateRunnerFn returned nil runner without error") + } // Wire the QBFT round-timer factory, bound to a msg ID carrying this duty's role so timeout // events are routed to the matching (committee vs aggregator-committee) slot queue. @@ -559,19 +562,41 @@ func (c *Committee) createRunner( // timer events under the right domain. Only GetRoleType() is read from this ID downstream, so // this is a consistency fix, not a behavior change today. runnerIdentifier := spectypes.NewMsgID(c.networkConfig.DomainTypeAtSlot(duty.DutySlot()), c.CommitteeMember.CommitteeID[:], role) - r.SetQBFTRoundTimerF(c.newQBFTRoundTimerF(runnerIdentifier)) + // The typed-nil checks below complement the interface-nil guard above: a CreateRunnerFn + // returning a nil *runner.CommitteeRunner behind a non-nil interface passes that guard but + // would panic on the first (promoted) method call — so no method is called on r until its + // concrete type and non-nilness are established. switch duty := duty.(type) { case *spectypes.CommitteeDuty: - c.Runners[duty.DutySlot()] = r.(*runner.CommitteeRunner) + cr, ok := r.(*runner.CommitteeRunner) + if !ok { + return nil, fmt.Errorf("BUG: runner created for committee duty has type %T, expected *runner.CommitteeRunner", r) + } + if cr == nil { + return nil, fmt.Errorf("BUG: CreateRunnerFn returned a typed-nil *runner.CommitteeRunner without error") + } + cr.SetQBFTRoundTimerF(c.newQBFTRoundTimerF(runnerIdentifier)) + c.Runners[duty.DutySlot()] = cr case *spectypes.AggregatorCommitteeDuty: - c.AggregatorRunners[duty.DutySlot()] = r.(*runner.AggregatorCommitteeRunner) + ar, ok := r.(*runner.AggregatorCommitteeRunner) + if !ok { + return nil, fmt.Errorf("BUG: runner created for aggregator committee duty has type %T, expected *runner.AggregatorCommitteeRunner", r) + } + if ar == nil { + return nil, fmt.Errorf("BUG: CreateRunnerFn returned a typed-nil *runner.AggregatorCommitteeRunner without error") + } + ar.SetQBFTRoundTimerF(c.newQBFTRoundTimerF(runnerIdentifier)) + c.AggregatorRunners[duty.DutySlot()] = ar default: + // Unlike the runner-type mismatches above, which guard the injected CreateRunnerFn, + // the duty type is produced by this package's own callers — a mismatch here is a + // local code bug, so it stays a loud panic rather than a returned error. c.logger.Panic("BUG: attempt to create committee runner with non-committee duty type", zap.String("type", fmt.Sprintf("%T", duty))) } - return r, err + return r, nil } func (c *Committee) extractValidatorDuties(duty spectypes.Duty) []*spectypes.ValidatorDuty {