From a2b9f959c5effec3176779bccee65f0c960cbaf0 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 10 Aug 2022 18:45:42 +0000 Subject: [PATCH] Core_Version codec changes - Always encode using all latest fields - Decode required fields then optional fields - Remove `legacyData` struct - Update `Encode` and `DecodeVersion` comments - Update unit tests --- lib/runtime/version.go | 106 +++++++++++------------- lib/runtime/version_test.go | 127 +++++++++++++---------------- lib/runtime/wasmer/exports_test.go | 2 +- 3 files changed, 104 insertions(+), 131 deletions(-) diff --git a/lib/runtime/version.go b/lib/runtime/version.go index fba9720303b..a2ab51de0d2 100644 --- a/lib/runtime/version.go +++ b/lib/runtime/version.go @@ -4,9 +4,9 @@ package runtime import ( + "bytes" "errors" "fmt" - "strings" "github.com/ChainSafe/gossamer/pkg/scale" ) @@ -26,79 +26,65 @@ type Version struct { ImplVersion uint32 APIItems []APIItem TransactionVersion uint32 - legacy bool -} - -type legacyData struct { - SpecName []byte - ImplName []byte - AuthoringVersion uint32 - SpecVersion uint32 - ImplVersion uint32 - APIItems []APIItem } // Encode returns the scale encoding of the version. +// Note the encoding contains all the latest Core_version fields as defined in +// https://spec.polkadot.network/#defn-rt-core-version +// In other words, decoding older version data with missing fields +// and then encoding it will result in a longer encoding due to the +// extra version fields. This however remains compatible since the +// version fields are still encoded in the same order and an older +// decoder would succeed with the longer encoding. func (v *Version) Encode() (encoded []byte, err error) { - if !v.legacy { - return scale.Marshal(*v) - } - - toEncode := legacyData{ - SpecName: v.SpecName, - ImplName: v.ImplName, - AuthoringVersion: v.AuthoringVersion, - SpecVersion: v.SpecVersion, - ImplVersion: v.ImplVersion, - APIItems: v.APIItems, - } - return scale.Marshal(toEncode) + return scale.Marshal(*v) } var ( - ErrDecodingVersion = errors.New("decoding version") - ErrDecodingLegacyVersion = errors.New("decoding legacy version") + ErrDecodingVersionField = errors.New("decoding version field") ) -// DecodeVersion scale decodes the encoded version data and returns an error. -// It first tries to decode the data using the current version format. -// If that fails with an EOF error, it then tries to decode the data -// using the legacy version format (for Kusama). +// DecodeVersion scale decodes the encoded version data. +// For older version data with missing fields (such as `transaction_version`) +// the missing field is set to its zero value (such as `0`). func DecodeVersion(encoded []byte) (version Version, err error) { - err = scale.Unmarshal(encoded, &version) - if err == nil { - return version, nil - } + reader := bytes.NewReader(encoded) + decoder := scale.NewDecoder(reader) - if !strings.Contains(err.Error(), "EOF") { - // TODO io.EOF should be wrapped in scale and - // ErrDecodingVersion should be removed once this is done. - return version, fmt.Errorf("%w: %s", ErrDecodingVersion, err) + type namedValue struct { + // name is the field name used to wrap eventual codec errors + name string + // value is the field value to handle + value interface{} } - // TODO: kusama seems to use the legacy version format - var legacy legacyData - err = scale.Unmarshal(encoded, &legacy) - if err != nil { - // TODO sentinel error should be wrapped in scale and - // ErrDecodingLegacyVersion should be removed once this is done. - return version, fmt.Errorf("%w: %s", ErrDecodingLegacyVersion, err) + requiredFields := [...]namedValue{ + {name: "spec name", value: &version.SpecName}, + {name: "impl name", value: &version.ImplName}, + {name: "authoring version", value: &version.AuthoringVersion}, + {name: "spec version", value: &version.SpecVersion}, + {name: "impl version", value: &version.ImplVersion}, + {name: "API items", value: &version.APIItems}, + } + for _, requiredField := range requiredFields { + err = decoder.Decode(requiredField.value) + if err != nil { + return Version{}, fmt.Errorf("%w %s: %s", ErrDecodingVersionField, requiredField.name, err) + } } - return Version{ - SpecName: legacy.SpecName, - ImplName: legacy.ImplName, - AuthoringVersion: legacy.AuthoringVersion, - SpecVersion: legacy.SpecVersion, - ImplVersion: legacy.ImplVersion, - APIItems: legacy.APIItems, - legacy: true, - }, nil -} + optionalFields := [...]namedValue{ + {name: "transaction version", value: &version.TransactionVersion}, + } + for _, optionalField := range optionalFields { + err = decoder.Decode(optionalField.value) + if err != nil { + if err.Error() == "EOF" { + return version, nil + } + return Version{}, fmt.Errorf("%w %s: %s", ErrDecodingVersionField, optionalField.name, err) + } + } -// WithLegacy sets the legacy boolean (for Kusama) -// and is only used for tests. -func (v Version) WithLegacy() Version { - v.legacy = true //skipcq: RVV-B0006 - return v + return version, nil } diff --git a/lib/runtime/version_test.go b/lib/runtime/version_test.go index a88761c758a..df28d15b8fa 100644 --- a/lib/runtime/version_test.go +++ b/lib/runtime/version_test.go @@ -6,25 +6,26 @@ package runtime import ( "testing" + "github.com/ChainSafe/gossamer/pkg/scale" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func Test_Version_Encode(t *testing.T) { - t.Parallel() +func scaleEncode(t *testing.T, x any) []byte { + encoded, err := scale.Marshal(x) + require.NoError(t, err) + return encoded +} - someVersion := Version{ - SpecName: []byte{1}, - ImplName: []byte{2}, - AuthoringVersion: 3, - SpecVersion: 4, - ImplVersion: 5, - APIItems: []APIItem{{ - Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, - Ver: 6, - }}, - TransactionVersion: 7, +func concatBytes(slices [][]byte) (concatenated []byte) { + for _, slice := range slices { + concatenated = append(concatenated, slice...) } + return concatenated +} + +func Test_Version_Encode(t *testing.T) { + t.Parallel() testCases := map[string]struct { version Version @@ -32,20 +33,24 @@ func Test_Version_Encode(t *testing.T) { errWrapped error errMessage string }{ - "not legacy": { - version: someVersion, + "all optional fields set": { + version: Version{ + SpecName: []byte{1}, + ImplName: []byte{2}, + AuthoringVersion: 3, + SpecVersion: 4, + ImplVersion: 5, + APIItems: []APIItem{{ + Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, + Ver: 6, + }}, + TransactionVersion: 7, + }, encoding: []byte{ 0x4, 0x1, 0x4, 0x2, 0x3, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x5, 0x0, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x6, 0x0, 0x0, 0x0, 0x7, 0x0, 0x0, 0x0}, }, - "legacy": { - version: someVersion.WithLegacy(), - encoding: []byte{ - 0x4, 0x1, 0x4, 0x2, 0x3, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, - 0x5, 0x0, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, - 0x8, 0x6, 0x0, 0x0, 0x0}, - }, } for name, testCase := range testCases { @@ -73,11 +78,35 @@ func Test_DecodeVersion(t *testing.T) { errWrapped error errMessage string }{ - "unmarshal success": { + "required field decode error": { + encoded: concatBytes([][]byte{ + scaleEncode(t, []byte{1, 2}), + {255, 255}, // error + }), + errWrapped: ErrDecodingVersionField, + errMessage: "decoding version field impl name: could not decode invalid integer", + }, + // TODO add transaction version decode error once + // https://github.com/ChainSafe/gossamer/pull/2683 + // is merged. + // "transaction version decode error": { + // encoded: concatBytes([][]byte{ + // scaleEncode(t, []byte("a")), // spec name + // scaleEncode(t, []byte("b")), // impl name + // scaleEncode(t, uint32(1)), // authoring version + // scaleEncode(t, uint32(2)), // spec version + // scaleEncode(t, uint32(3)), // impl version + // scaleEncode(t, []APIItem{{}}), // api items + // {1, 2, 3}, // transaction version + // }), + // errWrapped: ErrDecoding, + // errMessage: "decoding transaction version: could not decode invalid integer", + // }, + "no optional field set": { encoded: []byte{ 0x4, 0x1, 0x4, 0x2, 0x3, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x5, 0x0, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, - 0x8, 0x6, 0x0, 0x0, 0x0, 0x7, 0x0, 0x0, 0x0}, + 0x8, 0x6, 0x0, 0x0, 0x0}, version: Version{ SpecName: []byte{1}, ImplName: []byte{2}, @@ -88,24 +117,13 @@ func Test_DecodeVersion(t *testing.T) { Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, Ver: 6, }}, - TransactionVersion: 7, }, }, - "unmarshal error": { - encoded: []byte{255, 255}, - errWrapped: ErrDecodingVersion, - errMessage: "decoding version: could not decode invalid integer, field: []", - }, - "legacy unmarshal error": { - encoded: []byte{0}, - errWrapped: ErrDecodingLegacyVersion, - errMessage: "decoding legacy version: EOF, field: []", - }, - "legacy unmarshal success": { + "transaction version set": { encoded: []byte{ 0x4, 0x1, 0x4, 0x2, 0x3, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x5, 0x0, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, - 0x8, 0x6, 0x0, 0x0, 0x0}, + 0x8, 0x6, 0x0, 0x0, 0x0, 0x7, 0x0, 0x0, 0x0}, version: Version{ SpecName: []byte{1}, ImplName: []byte{2}, @@ -116,7 +134,7 @@ func Test_DecodeVersion(t *testing.T) { Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, Ver: 6, }}, - legacy: true, + TransactionVersion: 7, }, }, } @@ -145,7 +163,7 @@ func Test_Version_Scale(t *testing.T) { encoding []byte decoded Version }{ - "current version": { + "all fields set": { version: Version{ SpecName: []byte{1}, ImplName: []byte{2}, @@ -175,37 +193,6 @@ func Test_Version_Scale(t *testing.T) { TransactionVersion: 7, }, }, - "legacy version": { - version: Version{ - SpecName: []byte{1}, - ImplName: []byte{2}, - AuthoringVersion: 3, - SpecVersion: 4, - ImplVersion: 5, - APIItems: []APIItem{{ - Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, - Ver: 6, - }}, - TransactionVersion: 7, - legacy: true, - }, - encoding: []byte{ - 0x4, 0x1, 0x4, 0x2, 0x3, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, - 0x5, 0x0, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, - 0x8, 0x6, 0x0, 0x0, 0x0}, - decoded: Version{ - SpecName: []byte{1}, - ImplName: []byte{2}, - AuthoringVersion: 3, - SpecVersion: 4, - ImplVersion: 5, - APIItems: []APIItem{{ - Name: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}, - Ver: 6, - }}, - legacy: true, - }, - }, } for name, testCase := range testCases { diff --git a/lib/runtime/wasmer/exports_test.go b/lib/runtime/wasmer/exports_test.go index eecacae4d40..90a9277b9f1 100644 --- a/lib/runtime/wasmer/exports_test.go +++ b/lib/runtime/wasmer/exports_test.go @@ -157,7 +157,7 @@ func Test_Instance_Version(t *testing.T) { {Name: [8]uint8{0xbc, 0x9d, 0x89, 0x90, 0x4f, 0x5b, 0x92, 0x3f}, Ver: 0x1}, {Name: [8]uint8{0x37, 0xc8, 0xbb, 0x13, 0x50, 0xa9, 0xa2, 0xa8}, Ver: 0x1}, }, - }.WithLegacy(), + }, }, "polkadot v0825": { instanceBuilder: func(t *testing.T) InstanceVersion {