Add nil-element validation for TimeoutCertificate in TimeoutObject and HeaderBody

consensus/hotstuff/model/timeout.go

@@ -94,13 +94,19 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error)
 	// If a TC is included, the TC must be for the past round, no matter whether a QC
 	// for the last round is also included. In some edge cases, a node might observe
 	// _both_ QC and TC for the previous round, in which case it can include both.
+	var lastViewTC *flow.TimeoutCertificate
 	if untrusted.LastViewTC != nil {
 		if untrusted.View != untrusted.LastViewTC.View+1 {
 			return nil, fmt.Errorf("invalid TC for non-previous view, expected view %d, got view %d", untrusted.View-1, untrusted.LastViewTC.View)
 		}
-		if untrusted.NewestQC.View < untrusted.LastViewTC.NewestQC.View {
-			return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, untrusted.LastViewTC.NewestQC.View)
+		tc, err := flow.NewTimeoutCertificate(flow.UntrustedTimeoutCertificate(*untrusted.LastViewTC))
+		if err != nil {
+			return nil, fmt.Errorf("invalid LastViewTC: %w", err)
 		}
+		if untrusted.NewestQC.View < tc.NewestQC.View {
+			return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, tc.NewestQC.View)
+		}
+		lastViewTC = tc
 	}
 	// The TO must contain a proof that sender legitimately entered View. Transitioning
 	// to round timeout.View is possible either by observing a QC or a TC for the previous round.
@@ -118,7 +124,7 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error)
 	return &TimeoutObject{
 		View:        untrusted.View,
 		NewestQC:    untrusted.NewestQC,
-		LastViewTC:  untrusted.LastViewTC,
+		LastViewTC:  lastViewTC,
 		SignerID:    untrusted.SignerID,
 		SigData:     untrusted.SigData,
 		TimeoutTick: untrusted.TimeoutTick,

consensus/hotstuff/model/timeout_test.go

@@ -195,13 +195,15 @@ func TestNewTimeoutObject(t *testing.T) {
 	})
 
 	t.Run("invalid input when TimeoutObject's QC is older than TC's QC", func(t *testing.T) {
+		// TC must be valid: TC.View (199) >= TC.NewestQC.View (150).
+		// TO.NewestQC.View (80) < TC.NewestQC.View (150) triggers the error.
 		tcQC := helper.MakeQC(helper.WithQCView(150))
-		tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(99))
+		tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(199))
 
 		res, err := model.NewTimeoutObject(
 			model.UntrustedTimeoutObject(
 				*helper.TimeoutObjectFixture(
-					helper.WithTimeoutObjectView(100),
+					helper.WithTimeoutObjectView(200), // must be TC.View+1
 					helper.WithTimeoutLastViewTC(tc),
 					helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(80))), // older than TC.NewestQC
 				),
@@ -212,6 +214,24 @@ func TestNewTimeoutObject(t *testing.T) {
 		assert.Contains(t, err.Error(), "timeout.NewestQC is older")
 	})
 
+	t.Run("invalid input when LastViewTC has nil NewestQC", func(t *testing.T) {
+		tc := helper.MakeTC(helper.WithTCNewestQC(helper.MakeQC(helper.WithQCView(150))), helper.WithTCView(199))
+		tc.NewestQC = nil
+
+		res, err := model.NewTimeoutObject(
+			model.UntrustedTimeoutObject(
+				*helper.TimeoutObjectFixture(
+					helper.WithTimeoutObjectView(200), // TO.View == TC.View+1
+					helper.WithTimeoutLastViewTC(tc),
+					helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(150))), // TC.NewestQC.View(150) <= TO.NewestQC.View < TO.View(200)
+				),
+			),
+		)
+		require.Error(t, err)
+		require.Nil(t, res)
+		assert.Contains(t, err.Error(), "invalid LastViewTC")
+	})
+
 	t.Run("invalid input when no QC for previous round and TC is missing", func(t *testing.T) {
 		qc := helper.MakeQC(helper.WithQCView(90))
 

model/cluster/block_test.go

@@ -13,18 +13,45 @@ import (
 	"github.com/onflow/flow-go/utils/unittest"
 )
 
+// clusterBlockWithLastViewTC returns a ClusterBlockFixture that is guaranteed to have a non-nil
+// LastViewTC. HeaderBodyWithParentFixture omits LastViewTC when view == parent.View+1 (1-in-10
+// chance). The malleability checker allocates a zero-value struct when a checked field is a nil
+// pointer, and NewHeaderBody rejects that zero-value TC (nil `NewestQC`), so hashModel() would panic.
+func clusterBlockWithLastViewTC() *cluster.Block {
+	const maxAttempts = 1000
+	for range maxAttempts {
+		if b := unittest.ClusterBlockFixture(); b.LastViewTC != nil {
+			return b
+		}
+	}
+	panic("failed to generate ClusterBlockFixture with non-nil LastViewTC")
+}
+
 // TestClusterBlockMalleability checks that cluster.Block is not malleable: any change in its data
 // should result in a different ID.
-// Because our NewHeaderBody constructor enforces ParentView < View we use
-// WithFieldGenerator to safely pass it.
+// Because our NewHeaderBody constructor enforces ParentView < View and validates LastViewTC via
+// NewTimeoutCertificate we use WithFieldGenerator to safely pass both.
 func TestClusterBlockMalleability(t *testing.T) {
-	clusterBlock := unittest.ClusterBlockFixture()
+	clusterBlock := clusterBlockWithLastViewTC()
 	unittest.RequireEntityNonMalleable(
 		t,
 		clusterBlock,
 		unittest.WithFieldGenerator("HeaderBody.ParentView", func() uint64 {
 			return clusterBlock.View - 1 // ParentView must stay below View, so set it to View-1
 		}),
+		// The field generator for LastViewTC must return the struct value (not a pointer):
+		// isModelMalleable dereferences *TimeoutCertificate before invoking the generator,
+		// so modelOrField is flow.TimeoutCertificate at that point.
+		unittest.WithFieldGenerator("HeaderBody.LastViewTC", func() flow.TimeoutCertificate {
+			qc := unittest.QuorumCertificateFixture()
+			return flow.TimeoutCertificate{
+				View:          qc.View + 1,
+				NewestQCViews: []uint64{qc.View},
+				NewestQC:      qc,
+				SignerIndices: unittest.SignerIndicesFixture(4),
+				SigData:       unittest.SignatureFixture(),
+			}
+		}),
 		unittest.WithFieldGenerator("Payload.Collection", func() flow.Collection {
 			return unittest.CollectionFixture(3)
 		}),

model/flow/block_test.go

@@ -118,18 +118,45 @@ func TestBlock_Status(t *testing.T) {
 	}
 }
 
+// blockWithLastViewTC returns a FullBlockFixture that is guaranteed to have a non-nil LastViewTC.
+// HeaderBodyWithParentFixture omits LastViewTC when view == parent.View+1 (1-in-10 chance).
+// NewHeaderBody now validates LastViewTC via NewTimeoutCertificate, so mutating a nil TC to a
+// zero-value struct would cause hashModel() to panic in the malleability checker.
+func blockWithLastViewTC() *flow.Block {
+	const maxAttempts = 1000
+	for range maxAttempts {
+		if b := unittest.FullBlockFixture(); b.LastViewTC != nil {
+			return b
+		}
+	}
+	panic("failed to generate FullBlockFixture with non-nil LastViewTC")
+}
+
 // TestBlockMalleability checks that flow.Block is not malleable: any change in its data
 // should result in a different ID.
-// Because our NewHeaderBody constructor enforces ParentView < View we use
-// WithFieldGenerator to safely pass it.
+// NewHeaderBody enforces ParentView < View and validates LastViewTC via NewTimeoutCertificate,
+// so WithFieldGenerator is used for both to keep those constraints intact.
 func TestBlockMalleability(t *testing.T) {
-	block := unittest.FullBlockFixture()
+	block := blockWithLastViewTC()
 	unittest.RequireEntityNonMalleable(
 		t,
-		unittest.FullBlockFixture(),
+		block,
 		unittest.WithFieldGenerator("HeaderBody.ParentView", func() uint64 {
 			return block.View - 1 // ParentView must stay below View, so set it to View-1
 		}),
+		// The field generator for LastViewTC must return the struct value (not a pointer):
+		// isModelMalleable dereferences *TimeoutCertificate before invoking the generator,
+		// so modelOrField is flow.TimeoutCertificate at that point.
+		unittest.WithFieldGenerator("HeaderBody.LastViewTC", func() flow.TimeoutCertificate {
+			qc := unittest.QuorumCertificateFixture()
+			return flow.TimeoutCertificate{
+				View:          qc.View + 1,
+				NewestQCViews: []uint64{qc.View},
+				NewestQC:      qc,
+				SignerIndices: unittest.SignerIndicesFixture(4),
+				SigData:       unittest.SignatureFixture(),
+			}
+		}),
 		unittest.WithFieldGenerator("Payload.Results", func() flow.ExecutionResultList {
 			return flow.ExecutionResultList{unittest.ExecutionResultFixture()}
 		}),

model/flow/header.go

@@ -111,8 +111,27 @@ func NewHeaderBody(untrusted UntrustedHeaderBody) (*HeaderBody, error) {
 		return nil, fmt.Errorf("Timestamp must not be zero-value")
 	}
 
-	hb := HeaderBody(untrusted)
-	return &hb, nil
+	var lastViewTC *TimeoutCertificate
+	if untrusted.LastViewTC != nil {
+		tc, err := NewTimeoutCertificate(UntrustedTimeoutCertificate(*untrusted.LastViewTC))
+		if err != nil {
+			return nil, fmt.Errorf("invalid LastViewTC: %w", err)
+		}
+		lastViewTC = tc
+	}
+
+	return &HeaderBody{
+		ChainID:            untrusted.ChainID,
+		ParentID:           untrusted.ParentID,
+		Height:             untrusted.Height,
+		Timestamp:          untrusted.Timestamp,
+		View:               untrusted.View,
+		ParentView:         untrusted.ParentView,
+		ParentVoterIndices: untrusted.ParentVoterIndices,
+		ParentVoterSigData: untrusted.ParentVoterSigData,
+		ProposerID:         untrusted.ProposerID,
+		LastViewTC:         lastViewTC,
+	}, nil
 }
 
 // NewRootHeaderBody creates a new instance of root HeaderBody.

model/flow/header_test.go

@@ -314,6 +314,22 @@ func TestNewHeaderBody(t *testing.T) {
 		assert.Nil(t, hb)
 		assert.Contains(t, err.Error(), "Timestamp must not be zero-value")
 	})
+
+	t.Run("non-nil LastViewTC with nil NewestQC rejected", func(t *testing.T) {
+		u := UntrustedHeaderBodyFixture(func(u *flow.UntrustedHeaderBody) {
+			u.LastViewTC = &flow.TimeoutCertificate{
+				View:          u.View - 1,
+				NewestQCViews: []uint64{u.View - 2},
+				NewestQC:      nil,
+				SignerIndices: unittest.SignerIndicesFixture(4),
+				SigData:       unittest.SignatureFixture(),
+			}
+		})
+		hb, err := flow.NewHeaderBody(u)
+		assert.Error(t, err)
+		assert.Nil(t, hb)
+		assert.Contains(t, err.Error(), "invalid LastViewTC")
+	})
 }
 
 // TestHeaderBodyBuilder_PresenceChecks verifies that HeaderBodyBuilder.Build

utils/unittest/fixtures/generators_test.go

@@ -25,6 +25,35 @@ func TestGeneratorSuiteRandomSeed(t *testing.T) {
 	assert.NotEqual(t, header, header2)
 }
 
+// TestBlockFixtureLastViewTC verifies that a block which skipped views carries a LastViewTC that is
+// consistent with the block's own `View` and `ParentView`. `ToHeader` runs the trusted header body
+// constructor, which rejects a TC whose newest QC is newer than the TC itself.
+func TestBlockFixtureLastViewTC(t *testing.T) {
+	assertHeader := func(t *testing.T, header *flow.Header) {
+		require.Greater(t, header.View, header.ParentView)
+		if header.LastViewTC == nil {
+			require.Equal(t, header.ParentView+1, header.View)
+			return
+		}
+		// the TC certifies the last view that failed to produce a QC
+		require.Equal(t, header.View-1, header.LastViewTC.View)
+		require.LessOrEqual(t, header.LastViewTC.NewestQC.View, header.LastViewTC.View)
+	}
+
+	// Blocks().List returns a chain whose blocks have a 50% chance of skipping views, so it covers
+	// both blocks with and without a TC. A fixed seed keeps the coverage reproducible.
+	suite := NewGeneratorSuite(WithSeed(42))
+	blocks := suite.Blocks().List(10)
+	for _, block := range blocks {
+		assertHeader(t, block.ToHeader()) // panics if the generated header body is invalid
+	}
+
+	// a header whose view skips multiple views must carry a TC for view-1
+	header := suite.Headers().Fixture(Header.WithView(100), Header.WithParentView(10))
+	require.NotNil(t, header.LastViewTC)
+	assertHeader(t, header)
+}
+
 func TestGeneratorsDeterminism(t *testing.T) {
 	// Test all generators
 	tests := []struct {

utils/unittest/fixtures/header.go

@@ -182,17 +182,6 @@ func (g *HeaderGenerator) Fixture(opts ...HeaderOption) *flow.Header {
 		opt(g, header)
 	}
 
-	if header.View != header.ParentView+1 && header.LastViewTC == nil {
-		newestQC := g.quorumCerts.Fixture(QuorumCertificate.WithView(header.ParentView))
-		header.LastViewTC = &flow.TimeoutCertificate{
-			View:          view - 1,
-			NewestQCViews: []uint64{newestQC.View},
-			NewestQC:      newestQC,
-			SignerIndices: g.signerIndices.Fixture(),
-			SigData:       g.signatures.Fixture(),
-		}
-	}
-
 	// View must be strictly greater than ParentView. Since we are generating default values for each
 	// and allowing the caller to independently update them, we need to do some extra bookkeeping to
 	// ensure that the values remain consistent after applying the options. Since the values start
@@ -212,6 +201,21 @@ func (g *HeaderGenerator) Fixture(opts ...HeaderOption) *flow.Header {
 		}
 	}
 
+	// A block that skipped views must carry the timeout certificate for the last view that failed to
+	// produce a QC. It is generated from the final `View` and `ParentView` (after the bookkeeping
+	// above), so that `LastViewTC.View` is the highest view below `View` and is never smaller than
+	// `ParentView`, which is the view of the TC's newest QC.
+	if header.View > header.ParentView+1 && header.LastViewTC == nil {
+		newestQC := g.quorumCerts.Fixture(QuorumCertificate.WithView(header.ParentView))
+		header.LastViewTC = &flow.TimeoutCertificate{
+			View:          header.View - 1,
+			NewestQCViews: []uint64{newestQC.View},
+			NewestQC:      newestQC,
+			SignerIndices: g.signerIndices.Fixture(),
+			SigData:       g.signatures.Fixture(),
+		}
+	}
+
 	// sanity checks
 	Assertf(header.View > header.ParentView,
 		"view must be greater than or equal to parent view: %d > %d", header.View, header.ParentView)