Boole hardening: partial-sig size cap, checked runner assertions, role-mapper lockstep test (#2978 items 3, 4, 7)

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)

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)
+	})
+}

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)
+	}
+}

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)
+	}
 }

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 {