Merge pull request #340 from onflow/fxamacker/remove-empty-next-slab-…

…id-in-encoded-map-data Omit empty next slab ID in encoded map data slab
onflow · Sep 12, 2023 · cf56a4c · cf56a4c
2 parents 2329549 + cbc8204
commit cf56a4c
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 40 deletions.
diff --git a/map.go b/map.go
@@ -2200,20 +2200,21 @@ func newMapDataSlabFromDataV1(
 	var extraData *MapExtraData
 	var next SlabID
 
-	// Decode header
+	// Decode extra data
 	if h.isRoot() {
-		// Decode extra data
 		extraData, data, err = newMapExtraDataFromData(data, decMode, decodeTypeInfo)
 		if err != nil {
 			// Don't need to wrap error as external error because err is already categorized by newMapExtraDataFromData().
 			return nil, err
 		}
-	} else {
+	}
+
+	// Decode next slab ID
+	if h.hasNextSlabID() {
 		if len(data) < slabIDSize {
 			return nil, NewDecodingErrorf("data is too short for map data slab")
 		}
 
-		// Decode next slab ID
 		next, err = NewSlabIDFromRawBytes(data)
 		if err != nil {
 			// Don't need to wrap error as external error because err is already categorized by NewSlabIDFromRawBytes().
@@ -2291,6 +2292,10 @@ func (m *MapDataSlab) Encode(enc *Encoder) error {
 		h.setHasPointers()
 	}
 
+	if m.next != SlabIDUndefined {
+		h.setHasNextSlabID()
+	}
+
 	if m.anySize {
 		h.setNoSizeLimit()
 	}
@@ -2305,16 +2310,17 @@ func (m *MapDataSlab) Encode(enc *Encoder) error {
 		return NewEncodingError(err)
 	}
 
-	// Encode header
+	// Encode extra data
 	if m.extraData != nil {
-		// Encode extra data
 		err = m.extraData.Encode(enc)
 		if err != nil {
 			// Don't need to wrap error as external error because err is already categorized by MapExtraData.Encode().
 			return err
 		}
-	} else {
-		// Encode next slab ID to scratch
+	}
+
+	// Encode next slab ID
+	if m.next != SlabIDUndefined {
 		n, err := m.next.ToRawBytes(enc.Scratch[:])
 		if err != nil {
 			// Don't need to wrap error as external error because err is already categorized by SlabID.ToRawBytes().

diff --git a/map_debug.go b/map_debug.go
@@ -849,18 +849,16 @@ func validMapSlabSerialization(
 	}
 
 	// Extra check: encoded data size == header.size
-	encodedExtraDataSize, err := getEncodedMapExtraDataSize(slab.ExtraData(), cborEncMode)
+	encodedSlabSize, err := computeSlabSize(data)
 	if err != nil {
-		// Don't need to wrap error as external error because err is already categorized by getEncodedMapExtraDataSize().
+		// Don't need to wrap error as external error because err is already categorized by computeSlabSize().
 		return err
 	}
 
-	// Need to exclude extra data size from encoded data size.
-	encodedSlabSize := uint32(len(data) - encodedExtraDataSize)
-	if slab.Header().size != encodedSlabSize {
+	if slab.Header().size != uint32(encodedSlabSize) {
 		return NewFatalError(
-			fmt.Errorf("slab %d encoded size %d != header.size %d (encoded extra data size %d)",
-				id, encodedSlabSize, slab.Header().size, encodedExtraDataSize))
+			fmt.Errorf("slab %d encoded size %d != header.size %d",
+				id, encodedSlabSize, slab.Header().size))
 	}
 
 	// Compare encoded data of original slab with encoded data of decoded slab
@@ -1357,20 +1355,3 @@ func mapExtraDataEqual(expected, actual *MapExtraData) error {
 
 	return nil
 }
-
-func getEncodedMapExtraDataSize(extraData *MapExtraData, cborEncMode cbor.EncMode) (int, error) {
-	if extraData == nil {
-		return 0, nil
-	}
-
-	var buf bytes.Buffer
-	enc := NewEncoder(&buf, cborEncMode)
-
-	err := extraData.Encode(enc)
-	if err != nil {
-		// Don't need to wrap error as external error because err is already categorized by MapExtraData.Encode().
-		return 0, err
-	}
-
-	return len(buf.Bytes()), nil
-}
diff --git a/map_test.go b/map_test.go
@@ -2737,7 +2737,7 @@ func TestMapEncodeDecode(t *testing.T) {
 			// data slab
 			id2: {
 				// version
-				0x10,
+				0x12,
 				// flag: map data
 				0x08,
 				// next slab id
@@ -2789,8 +2789,6 @@ func TestMapEncodeDecode(t *testing.T) {
 				0x10,
 				// flag: has pointer + map data
 				0x48,
-				// next slab id
-				0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
 				// the following encoded data is valid CBOR
 
@@ -2864,7 +2862,8 @@ func TestMapEncodeDecode(t *testing.T) {
 		require.True(t, ok)
 		require.Equal(t, 2, len(meta.childrenHeaders))
 		require.Equal(t, uint32(len(stored[id2])), meta.childrenHeaders[0].size)
-		require.Equal(t, uint32(len(stored[id3])), meta.childrenHeaders[1].size)
+		// Need to add slabIDSize to encoded data slab here because empty slab ID is omitted during encoding.
+		require.Equal(t, uint32(len(stored[id3])+slabIDSize), meta.childrenHeaders[1].size)
 
 		// Decode data to new storage
 		storage2 := newTestPersistentStorageWithData(t, stored)
@@ -3392,8 +3391,6 @@ func TestMapEncodeDecode(t *testing.T) {
 				0x10,
 				// flag: any size + collision group
 				0x2b,
-				// next slab id
-				0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
 				// the following encoded data is valid CBOR
 
@@ -3457,8 +3454,6 @@ func TestMapEncodeDecode(t *testing.T) {
 				0x10,
 				// flag: any size + collision group
 				0x2b,
-				// next slab id
-				0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
 				// the following encoded data is valid CBOR