CBOR Encoding is compatible with stricter network decode mode

model/encoding/cbor/codec.go

@@ -35,6 +35,11 @@ var EncMode = func() cbor.EncMode {
 // DecMode is the default DecMode to use when creating a new cbor Decoder
 var DecMode, _ = cbor.DecOptions{}.DecMode()
 
+// NetworkDecMode is the DecMode used for decoding messages over the network.
+// It returns an error if the message contains any extra field not present in the
+// target (struct we are unmarshalling into), which prevents some classes of spamming.
+var NetworkDecMode, _ = cbor.DecOptions{ExtraReturnErrors: cbor.ExtraDecErrorUnknownField}.DecMode()
+
 func (m *Marshaler) Marshal(val interface{}) ([]byte, error) {
 	return EncMode.Marshal(val)
 }

model/flow/chunk.go

@@ -60,7 +60,7 @@ type ChunkBody struct {
 	// (2) Otherwise, ServiceEventCount must be non-nil.
 	// Within an ExecutionResult, all chunks must use either representation (1) or (2), not both.
 	// TODO(mainnet27, #6773): make this field non-pointer https://github.com/onflow/flow-go/issues/6773
-	ServiceEventCount *uint16
+	ServiceEventCount *uint16    `cbor:",omitempty"`
 	BlockID           Identifier // Block id of the execution result this chunk belongs to
 
 	// Computation consumption info

model/flow/chunk_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/onflow/flow-go/model/fingerprint"
+	cborcodec "github.com/onflow/flow-go/model/encoding/cbor"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/utils/rand"
 	"github.com/onflow/flow-go/utils/unittest"
@@ -183,11 +183,20 @@ func TestChunkEncodeDecode(t *testing.T) {
 			assert.Equal(t, chunk, unmarshaled)
 			assert.Nil(t, unmarshaled.ServiceEventCount)
 		})
-		t.Run("cbor", func(t *testing.T) {
-			bz, err := cbor.Marshal(chunk)
+		t.Run("cbor default", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunk)
 			require.NoError(t, err)
 			unmarshaled := new(flow.Chunk)
-			err = cbor.Unmarshal(bz, unmarshaled)
+			err = cborcodec.DecMode.Unmarshal(bz, unmarshaled)
+			require.NoError(t, err)
+			assert.Equal(t, chunk, unmarshaled)
+			assert.Nil(t, unmarshaled.ServiceEventCount)
+		})
+		t.Run("cbor strict", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunk)
+			require.NoError(t, err)
+			unmarshaled := new(flow.Chunk)
+			err = cborcodec.NetworkDecMode.Unmarshal(bz, unmarshaled)
 			require.NoError(t, err)
 			assert.Equal(t, chunk, unmarshaled)
 			assert.Nil(t, unmarshaled.ServiceEventCount)
@@ -204,11 +213,20 @@ func TestChunkEncodeDecode(t *testing.T) {
 			assert.Equal(t, chunk, unmarshaled)
 			assert.NotNil(t, unmarshaled.ServiceEventCount)
 		})
-		t.Run("cbor", func(t *testing.T) {
-			bz, err := cbor.Marshal(chunk)
+		t.Run("cbor default", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunk)
 			require.NoError(t, err)
 			unmarshaled := new(flow.Chunk)
-			err = cbor.Unmarshal(bz, unmarshaled)
+			err = cborcodec.DecMode.Unmarshal(bz, unmarshaled)
+			require.NoError(t, err)
+			assert.Equal(t, chunk, unmarshaled)
+			assert.NotNil(t, unmarshaled.ServiceEventCount)
+		})
+		t.Run("cbor strict", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunk)
+			require.NoError(t, err)
+			unmarshaled := new(flow.Chunk)
+			err = cborcodec.NetworkDecMode.Unmarshal(bz, unmarshaled)
 			require.NoError(t, err)
 			assert.Equal(t, chunk, unmarshaled)
 			assert.NotNil(t, unmarshaled.ServiceEventCount)
@@ -238,41 +256,105 @@ func TestChunk_ModelVersions_EncodeDecode(t *testing.T) {
 			assert.Nil(t, unmarshaled.ServiceEventCount)
 		})
 
-		t.Run("cbor", func(t *testing.T) {
-			bz, err := cbor.Marshal(chunkv0)
+		t.Run("cbor default", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunkv0)
 			require.NoError(t, err)
 
 			var unmarshaled flow.ChunkBody
-			err = cbor.Unmarshal(bz, &unmarshaled)
+			err = cborcodec.DecMode.Unmarshal(bz, &unmarshaled)
 			require.NoError(t, err)
 			assert.Equal(t, chunkv0.EventCollection, unmarshaled.EventCollection)
 			assert.Equal(t, chunkv0.BlockID, unmarshaled.BlockID)
 			assert.Nil(t, unmarshaled.ServiceEventCount)
 		})
-	})
-	t.Run("encoding v1 and decoding it into v0 should not error", func(t *testing.T) {
-		chunkv1 := chunkFixture.ChunkBody
-		chunkv1.ServiceEventCount = unittest.PtrTo[uint16](0) // ensure non-nil ServiceEventCount field
 
-		t.Run("json", func(t *testing.T) {
-			bz, err := json.Marshal(chunkv1)
+		t.Run("cbor strict", func(t *testing.T) {
+			bz, err := cborcodec.EncMode.Marshal(chunkv0)
 			require.NoError(t, err)
 
-			var unmarshaled flow.ChunkBodyV0
-			err = json.Unmarshal(bz, &unmarshaled)
+			var unmarshaled flow.ChunkBody
+			err = cborcodec.NetworkDecMode.Unmarshal(bz, &unmarshaled)
 			require.NoError(t, err)
-			assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
-			assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			assert.Equal(t, chunkv0.EventCollection, unmarshaled.EventCollection)
+			assert.Equal(t, chunkv0.BlockID, unmarshaled.BlockID)
+			assert.Nil(t, unmarshaled.ServiceEventCount)
 		})
-		t.Run("cbor", func(t *testing.T) {
-			bz, err := cbor.Marshal(chunkv1)
-			require.NoError(t, err)
-
-			var unmarshaled flow.ChunkBodyV0
-			err = cbor.Unmarshal(bz, &unmarshaled)
-			require.NoError(t, err)
-			assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
-			assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+	})
+	t.Run("encoding v1 and decoding it into v0", func(t *testing.T) {
+		t.Run("non-nil ServiceEventCount field", func(t *testing.T) {
+			chunkv1 := chunkFixture.ChunkBody
+			chunkv1.ServiceEventCount = unittest.PtrTo[uint16](0) // ensure non-nil ServiceEventCount field
+
+			t.Run("json - should not error", func(t *testing.T) {
+				bz, err := json.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = json.Unmarshal(bz, &unmarshaled)
+				require.NoError(t, err)
+				assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
+				assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			})
+			t.Run("cbor default - should not error", func(t *testing.T) {
+				bz, err := cborcodec.EncMode.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = cborcodec.DecMode.Unmarshal(bz, &unmarshaled)
+				require.NoError(t, err)
+				assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
+				assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			})
+			// In the stricter mode we use for network decoding, an error is expected
+			// because the message includes a field not present in the v0 target,
+			// when the new ServiceEventCount field is non-nil
+			t.Run("cbor strict - error expected", func(t *testing.T) {
+				bz, err := cborcodec.EncMode.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = cborcodec.NetworkDecMode.Unmarshal(bz, &unmarshaled)
+				assert.Error(t, err)
+				target := &cbor.UnknownFieldError{}
+				assert.ErrorAs(t, err, &target)
+			})
+		})
+		t.Run("nil ServiceEventCount field should not error", func(t *testing.T) {
+			chunkv1 := chunkFixture.ChunkBody
+			chunkv1.ServiceEventCount = nil // ensure non-nil ServiceEventCount field
+
+			t.Run("json", func(t *testing.T) {
+				bz, err := json.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = json.Unmarshal(bz, &unmarshaled)
+				require.NoError(t, err)
+				assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
+				assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			})
+			t.Run("cbor default", func(t *testing.T) {
+				bz, err := cborcodec.EncMode.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = cborcodec.DecMode.Unmarshal(bz, &unmarshaled)
+				require.NoError(t, err)
+				assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
+				assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			})
+			// When the new ServiceEventCount field is nil, we expect even strict
+			// cbor decoding to pass, because the new field will be omitted.
+			t.Run("cbor strict", func(t *testing.T) {
+				bz, err := cborcodec.EncMode.Marshal(chunkv1)
+				require.NoError(t, err)
+
+				var unmarshaled flow.ChunkBodyV0
+				err = cborcodec.NetworkDecMode.Unmarshal(bz, &unmarshaled)
+				require.NoError(t, err)
+				assert.Equal(t, chunkv1.EventCollection, unmarshaled.EventCollection)
+				assert.Equal(t, chunkv1.BlockID, unmarshaled.BlockID)
+			})
 		})
 	})
 }
@@ -294,15 +376,16 @@ func TestChunk_FingerprintBackwardCompatibility(t *testing.T) {
 	// A nil ServiceEventCount fields indicates a prior model version.
 	// The ID calculation for the old and new model version should be the same.
 	t.Run("nil ServiceEventCount fields", func(t *testing.T) {
-		chunkBody.ServiceEventCount = nil
-		assert.Equal(t, flow.MakeID(chunkBodyV0), flow.MakeID(chunkBody))
-		assert.Equal(t, fingerprint.Fingerprint(chunkBodyV0), fingerprint.Fingerprint(chunkBody))
+		chunk.ServiceEventCount = nil
+		assert.Equal(t, flow.MakeID(chunkBodyV0), chunk.ID())
+		assert.Equal(t, flow.MakeID(chunkBodyV0), flow.MakeID(chunk.ChunkBody))
 	})
 	// A non-nil ServiceEventCount fields indicates an up-to-date model version.
 	// The ID calculation for the old and new model version should be different,
 	// because the new model should include the ServiceEventCount field value.
 	t.Run("non-nil ServiceEventCount fields", func(t *testing.T) {
-		chunkBody.ServiceEventCount = unittest.PtrTo[uint16](0)
-		assert.NotEqual(t, flow.MakeID(chunkBodyV0), flow.MakeID(chunkBody))
+		chunk.ServiceEventCount = unittest.PtrTo[uint16](0)
+		assert.NotEqual(t, flow.MakeID(chunkBodyV0), chunk.ID())
+		assert.NotEqual(t, flow.MakeID(chunkBodyV0), flow.MakeID(chunk.ChunkBody))
 	})
 }

network/codec/cbor/codec.go

@@ -5,19 +5,14 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/fxamacker/cbor/v2"
-
 	cborcodec "github.com/onflow/flow-go/model/encoding/cbor"
 	"github.com/onflow/flow-go/network"
 	"github.com/onflow/flow-go/network/codec"
 	_ "github.com/onflow/flow-go/utils/binstat"
 )
 
-var defaultDecMode, _ = cbor.DecOptions{ExtraReturnErrors: cbor.ExtraDecErrorUnknownField}.DecMode()
-
 // Codec represents a CBOR codec for our network.
-type Codec struct {
-}
+type Codec struct{}
 
 // NewCodec creates a new CBOR codec.
 func NewCodec() *Codec {
@@ -33,7 +28,7 @@ func (c *Codec) NewEncoder(w io.Writer) network.Encoder {
 
 // NewDecoder creates a new CBOR decoder with the given underlying reader.
 func (c *Codec) NewDecoder(r io.Reader) network.Decoder {
-	dec := defaultDecMode.NewDecoder(r)
+	dec := cborcodec.NetworkDecMode.NewDecoder(r)
 	return &Decoder{dec: dec}
 }
 
@@ -104,7 +99,7 @@ func (c *Codec) Decode(data []byte) (interface{}, error) {
 
 	// unmarshal the payload
 	//bs2 := binstat.EnterTimeVal(fmt.Sprintf("%s%s%s:%d", binstat.BinNet, ":wire>4(cbor)", what, code), int64(len(data))) // e.g. ~3net:wire>4(cbor)CodeEntityRequest:23
-	err = defaultDecMode.Unmarshal(data[1:], msgInterface) // all but first byte
+	err = cborcodec.NetworkDecMode.Unmarshal(data[1:], msgInterface) // all but first byte
 	//binstat.Leave(bs2)
 	if err != nil {
 		return nil, codec.NewMsgUnmarshalErr(data[0], what, err)

network/codec/cbor/decoder.go

@@ -3,6 +3,7 @@ package cbor
 import (
 	"github.com/fxamacker/cbor/v2"
 
+	cborcodec "github.com/onflow/flow-go/model/encoding/cbor"
 	"github.com/onflow/flow-go/network/codec"
 	_ "github.com/onflow/flow-go/utils/binstat"
 )
@@ -40,7 +41,7 @@ func (d *Decoder) Decode() (interface{}, error) {
 
 	// unmarshal the payload
 	//bs2 := binstat.EnterTimeVal(fmt.Sprintf("%s%s%s:%d", binstat.BinNet, ":strm>2(cbor)", what, code), int64(len(data))) // e.g. ~3net:strm>2(cbor)CodeEntityRequest:23
-	err = defaultDecMode.Unmarshal(data[1:], msgInterface) // all but first byte
+	err = cborcodec.NetworkDecMode.Unmarshal(data[1:], msgInterface) // all but first byte
 	//binstat.Leave(bs2)
 	if err != nil {
 		return nil, codec.NewMsgUnmarshalErr(data[0], what, err)