fix(orm)!: properly encode/decode nanoseconds in Duration type (cosmo…

…s#19909) Co-authored-by: cool-developer <[email protected]> Co-authored-by: Aaron Craelius <[email protected]>
berachain · Jun 4, 2024 · 4fea3fb · 4fea3fb
1 parent 6ea50ef
commit 4fea3fb
Show file tree

Hide file tree

Showing 4 changed files with 349 additions and 47 deletions.
diff --git a/orm/CHANGELOG.md b/orm/CHANGELOG.md
@@ -53,7 +53,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * [#12273](https://github.com/cosmos/cosmos-sdk/pull/12273) The timestamp key encoding was reworked to properly handle nil values. Existing users will need to manually migrate their data to the new encoding before upgrading.
 * [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) The duration key encoding was reworked to properly handle nil values. Existing users will need to manually migrate their data to the new encoding before upgrading.
+* [#19909](https://github.com/cosmos/cosmos-sdk/pull/19909) (Breaking) Adjusts the encoding of zero and positive nanoseconds to ensure consistent comparison of duration objects. In the previous implementation, when nanoseconds were greater than or equal to zero, the encoding format was simple: we just represented the number in bytes (for example, 0 with [0x0]). For negative nanoseconds, we added 999,999,999 to ensure they were non-negative. In the new implementation, we always add 999,999,999 to all nanoseconds to ensure consistency in encoding and to maintain lexicographical order when compared with other durations.
 
 ### Bug Fixes
 
 * [#16023](https://github.com/cosmos/cosmos-sdk/pull/16023) Fix bugs introduced by lack of CI tests in [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) and [#15813](https://github.com/cosmos/cosmos-sdk/pull/15813). This changes the duration encoding in [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) to correctly order values with negative nanos.
+
diff --git a/orm/encoding/ormfield/duration.go b/orm/encoding/ormfield/duration.go
@@ -18,10 +18,13 @@ const (
 // encoding:
 // - nil is encoded as []byte{0xFF}
 // - seconds (which can range from -315,576,000,000 to +315,576,000,000) is encoded as 5 fixed bytes
-// - nanos (which can range from 0 to 999,999,999 or -999,999,999 to 0 if seconds is negative) is encoded as:
-//   - []byte{0x0} for zero nanos
-//   - 4 fixed bytes with the bit mask 0xC0 applied to the first byte, with negative nanos scaled so that -999,999,999
-//     is encoded as 1 and -1 is encoded as 999,999,999
+// - nanos (which can range from 0 to 999,999,999 or -999,999,999 to 0 if seconds is negative) are encoded such
+//   that 999,999,999 is always added to nanos. This ensures that the encoded nanos are always >= 0. Additionally,
+//   by adding 999,999,999 to both positive and negative nanos, we guarantee that the lexicographical order is
+//   preserved when comparing the encoded values of two Durations:
+//   - []byte{0xBB, 0x9A, 0xC9, 0xFF} for zero nanos
+//   - 4 fixed bytes with the bit mask 0x80 applied to the first byte, with negative nanos scaled so that -999,999,999
+//     is encoded as 0 and -1 is encoded as 999,999,998
 //
 // When iterating over timestamp indexes, nil values will always be ordered last.
 //
@@ -37,32 +40,21 @@ func (d DurationCodec) Encode(value protoreflect.Value, w io.Writer) error {
 
 	seconds, nanos := getDurationSecondsAndNanos(value)
 	secondsInt := seconds.Int()
-	if secondsInt < DurationSecondsMin || secondsInt > DurationSecondsMax {
-		return fmt.Errorf("duration seconds is out of range %d, must be between %d and %d", secondsInt, DurationSecondsMin, DurationSecondsMax)
+	nanosInt := nanos.Int()
+
+	if err := validateDurationRanges(secondsInt, nanosInt); err != nil {
+		return err
 	}
-	negative := secondsInt < 0
+
 	// we subtract the min duration value to make sure secondsInt is always non-negative and starts at 0.
 	secondsInt -= DurationSecondsMin
 	err := encodeSeconds(secondsInt, w)
 	if err != nil {
 		return err
 	}
 
-	nanosInt := nanos.Int()
-	if nanosInt == 0 {
-		_, err = w.Write(timestampZeroNanosBz)
-		return err
-	}
-
-	if negative {
-		if nanosInt < DurationNanosMin || nanosInt > 0 {
-			return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanosInt, DurationNanosMin, 0)
-		}
-		nanosInt = DurationNanosMax + nanosInt + 1
-	} else if nanosInt < 0 || nanosInt > DurationNanosMax {
-		return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanosInt, 0, DurationNanosMax)
-	}
-
+	// we subtract the min duration value to make sure nanosInt is always non-negative and starts at 0.
+	nanosInt -= DurationNanosMin
 	return encodeNanos(nanosInt, w)
 }
 
@@ -72,26 +64,18 @@ func (d DurationCodec) Decode(r Reader) (protoreflect.Value, error) {
 		return protoreflect.Value{}, err
 	}
 
-	// we add the min duration value to get back the original value
+	// we add the min seconds duration value to get back the original value
 	seconds += DurationSecondsMin
 
-	negative := seconds < 0
-
 	msg := durationMsgType.New()
 	msg.Set(durationSecondsField, protoreflect.ValueOfInt64(seconds))
 
 	nanos, err := decodeNanos(r)
 	if err != nil {
 		return protoreflect.Value{}, err
 	}
-
-	if nanos == 0 {
-		return protoreflect.ValueOfMessage(msg), nil
-	}
-
-	if negative {
-		nanos = nanos - DurationNanosMax - 1
-	}
+	// we add the min nanos duration value to get back the original value
+	nanos += DurationNanosMin
 
 	msg.Set(durationNanosField, protoreflect.ValueOfInt32(nanos))
 	return protoreflect.ValueOfMessage(msg), nil
@@ -141,6 +125,36 @@ func getDurationSecondsAndNanos(value protoreflect.Value) (protoreflect.Value, p
 	return msg.Get(durationSecondsField), msg.Get(durationNanosField)
 }
 
+// validateDurationRanges checks whether seconds and nanoseconds are in valid ranges
+// for a protobuf Duration type. It ensures that seconds are within the allowed range
+// and, if seconds are zero or negative, verifies that nanoseconds are also within
+// the valid range. For negative seconds, nanoseconds must be non-positive.
+// Parameters:
+//   - seconds: The number of seconds component of the duration.
+//   - nanos: The number of nanoseconds component of the duration.
+//
+// Returns:
+//   - error: An error indicating if the duration components are out of range.
+func validateDurationRanges(seconds, nanos int64) error {
+	if seconds < DurationSecondsMin || seconds > DurationSecondsMax {
+		return fmt.Errorf("duration seconds is out of range %d, must be between %d and %d", seconds, DurationSecondsMin, DurationSecondsMax)
+	}
+
+	if seconds == 0 {
+		if nanos < DurationNanosMin || nanos > DurationNanosMax {
+			return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanos, DurationNanosMin, DurationNanosMax)
+		}
+	} else if seconds < 0 {
+		if nanos < DurationNanosMin || nanos > 0 {
+			return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanos, DurationNanosMin, 0)
+		}
+	} else if nanos < 0 || nanos > DurationNanosMax {
+		return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanos, 0, DurationNanosMax)
+	}
+
+	return nil
+}
+
 // DurationV0Codec encodes a google.protobuf.Duration value as 12 bytes using
 // Int64Codec for seconds followed by Int32Codec for nanos. This allows for
 // sorted iteration.
@@ -191,3 +205,120 @@ func (d DurationV0Codec) FixedBufferSize() int {
 func (d DurationV0Codec) ComputeBufferSize(protoreflect.Value) (int, error) {
 	return d.FixedBufferSize(), nil
 }
+
+// DurationV1Codec encodes google.protobuf.Duration values with the following
+// encoding:
+// - nil is encoded as []byte{0xFF}
+// - seconds (which can range from -315,576,000,000 to +315,576,000,000) is encoded as 5 fixed bytes
+// - nanos (which can range from 0 to 999,999,999 or -999,999,999 to 0 if seconds is negative) is encoded as:
+//   - []byte{0x0} for zero nanos
+//   - 4 fixed bytes with the bit mask 0xC0 applied to the first byte, with negative nanos scaled so that -999,999,999
+//     is encoded as 1 and -1 is encoded as 999,999,999
+//
+// When iterating over timestamp indexes, nil values will always be ordered last.
+//
+// Values for seconds and nanos outside the ranges specified by google.protobuf.Duration will be rejected.
+type DurationV1Codec struct{}
+
+func (d DurationV1Codec) Encode(value protoreflect.Value, w io.Writer) error {
+	// nil case
+	if !value.IsValid() {
+		_, err := w.Write(timestampDurationNilBz)
+		return err
+	}
+
+	seconds, nanos := getDurationSecondsAndNanos(value)
+	secondsInt := seconds.Int()
+	if secondsInt < DurationSecondsMin || secondsInt > DurationSecondsMax {
+		return fmt.Errorf("duration seconds is out of range %d, must be between %d and %d", secondsInt, DurationSecondsMin, DurationSecondsMax)
+	}
+	negative := secondsInt < 0
+	// we subtract the min duration value to make sure secondsInt is always non-negative and starts at 0.
+	secondsInt -= DurationSecondsMin
+	err := encodeSeconds(secondsInt, w)
+	if err != nil {
+		return err
+	}
+
+	nanosInt := nanos.Int()
+	if nanosInt == 0 {
+		_, err = w.Write(timestampZeroNanosBz)
+		return err
+	}
+
+	if negative {
+		if nanosInt < DurationNanosMin || nanosInt > 0 {
+			return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanosInt, DurationNanosMin, 0)
+		}
+		nanosInt = DurationNanosMax + nanosInt + 1
+	} else if nanosInt < 0 || nanosInt > DurationNanosMax {
+		return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanosInt, 0, DurationNanosMax)
+	}
+
+	return encodeNanosV1(nanosInt, w)
+}
+
+func (d DurationV1Codec) Decode(r Reader) (protoreflect.Value, error) {
+	isNil, seconds, err := decodeSeconds(r)
+	if isNil || err != nil {
+		return protoreflect.Value{}, err
+	}
+
+	// we add the min duration value to get back the original value
+	seconds += DurationSecondsMin
+
+	negative := seconds < 0
+
+	msg := durationMsgType.New()
+	msg.Set(durationSecondsField, protoreflect.ValueOfInt64(seconds))
+
+	nanos, err := decodeNanosV1(r)
+	if err != nil {
+		return protoreflect.Value{}, err
+	}
+
+	if nanos == 0 {
+		return protoreflect.ValueOfMessage(msg), nil
+	}
+
+	if negative {
+		nanos = nanos - DurationNanosMax - 1
+	}
+
+	msg.Set(durationNanosField, protoreflect.ValueOfInt32(nanos))
+	return protoreflect.ValueOfMessage(msg), nil
+}
+
+func (d DurationV1Codec) Compare(v1, v2 protoreflect.Value) int {
+	if !v1.IsValid() {
+		if !v2.IsValid() {
+			return 0
+		}
+		return 1
+	}
+
+	if !v2.IsValid() {
+		return -1
+	}
+
+	s1, n1 := getDurationSecondsAndNanos(v1)
+	s2, n2 := getDurationSecondsAndNanos(v2)
+	c := compareInt(s1, s2)
+	if c != 0 {
+		return c
+	}
+
+	return compareInt(n1, n2)
+}
+
+func (d DurationV1Codec) IsOrdered() bool {
+	return true
+}
+
+func (d DurationV1Codec) FixedBufferSize() int {
+	return timestampDurationBufferSize
+}
+
+func (d DurationV1Codec) ComputeBufferSize(protoreflect.Value) (int, error) {
+	return timestampDurationBufferSize, nil
+}
diff --git a/orm/encoding/ormfield/duration_test.go b/orm/encoding/ormfield/duration_test.go
@@ -38,7 +38,7 @@ func TestDuration(t *testing.T) {
 			"no nanos",
 			100,
 			0,
-			6,
+			9,
 		},
 		{
 			"with nanos",
@@ -114,14 +114,6 @@ func TestDurationOutOfRange(t *testing.T) {
 			},
 			expectErr: "seconds is out of range",
 		},
-		{
-			name: "positive seconds negative nanos",
-			dur: &durationpb.Duration{
-				Seconds: 0,
-				Nanos:   -1,
-			},
-			expectErr: "nanos is out of range",
-		},
 		{
 			name: "positive seconds nanos too big",
 			dur: &durationpb.Duration{
@@ -241,6 +233,42 @@ func TestDurationCompare(t *testing.T) {
 			},
 			want: -1,
 		},
+		{
+			name: "negative seconds equal, dur1 nanos zero",
+			dur1: &durationpb.Duration{
+				Seconds: -1,
+				Nanos:   0,
+			},
+			dur2: &durationpb.Duration{
+				Seconds: -1,
+				Nanos:   -1,
+			},
+			want: 1,
+		},
+		{
+			name: "negative seconds equal, dur2 nanos zero",
+			dur1: &durationpb.Duration{
+				Seconds: -1,
+				Nanos:   -1,
+			},
+			dur2: &durationpb.Duration{
+				Seconds: -1,
+				Nanos:   0,
+			},
+			want: -1,
+		},
+		{
+			name: "seconds equal and dur1 nanos min values",
+			dur1: &durationpb.Duration{
+				Seconds: ormfield.DurationSecondsMin,
+				Nanos:   ormfield.DurationNanosMin,
+			},
+			dur2: &durationpb.Duration{
+				Seconds: ormfield.DurationSecondsMin,
+				Nanos:   -1,
+			},
+			want: -1,
+		},
 	}
 
 	for _, tc := range tt {