From c6b55c8355826d37f51859706bc62b7e36e957ba Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Wed, 4 Oct 2023 16:08:45 -0400 Subject: [PATCH 1/7] feat: introduces `SaturatingAdd` and `SaturatingSub` --- lib/primitives/math.go | 91 +++++++++++++++++++++++++++++++++++++ lib/primitives/math_test.go | 35 ++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 lib/primitives/math.go create mode 100644 lib/primitives/math_test.go diff --git a/lib/primitives/math.go b/lib/primitives/math.go new file mode 100644 index 0000000000..513547d611 --- /dev/null +++ b/lib/primitives/math.go @@ -0,0 +1,91 @@ +// Copyright 2023 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package primitives + +import ( + "fmt" + "unsafe" + + "golang.org/x/exp/constraints" +) + +// SaturatingAdd computes a + b saturating at the numeric bounds instead of overflowing +func SaturatingAdd[T constraints.Integer](a, b T) T { + switch any(a).(type) { + case int, int8, int16, int32, int64: + sizeOf := (unsafe.Sizeof(a) * 8) - 1 + + var ( + maxValueOfSignedType T = 1< 0 && a > max-b { + return max + } + + if b < 0 && a < min-b { + return min + } + + return a + b +} + +func saturatingAddUnsigned[T constraints.Integer](a, b, max T) T { + if a > max-b { + return max + } + return a + b +} + +// SaturatingSub computes a - b saturating at the numeric bounds instead of overflowing +func SaturatingSub[T constraints.Integer](a, b T) T { + switch any(a).(type) { + case int, int8, int16, int32, int64: + sizeOf := (unsafe.Sizeof(a) * 8) - 1 + + var ( + maxValueOfSignedType T = 1< max+b { + return max + } + + if b > 0 && a < min+b { + return min + } + + return a - b +} + +func saturatingSubUnsigned[T constraints.Integer](a, b T) T { + if a > b { + return a - b + } + return 0 +} diff --git a/lib/primitives/math_test.go b/lib/primitives/math_test.go new file mode 100644 index 0000000000..3551802d6f --- /dev/null +++ b/lib/primitives/math_test.go @@ -0,0 +1,35 @@ +// Copyright 2023 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package primitives + +import ( + "testing" + + "github.com/ethereum/go-ethereum/common/math" + "github.com/stretchr/testify/require" +) + +func TestSaturatingAdd(t *testing.T) { + require.Equal(t, uint8(2), SaturatingAdd(uint8(1), uint8(1))) + require.Equal(t, ^uint8(0), SaturatingAdd(^uint8(0), 100)) + require.Equal(t, ^uint32(0), SaturatingAdd(^uint32(0), 100)) + + // should not be able to overflow in the oposite direction as well + require.Equal(t, int64(math.MinInt64), SaturatingAdd(int64(math.MinInt64), -100)) + require.Equal(t, int8(127), SaturatingAdd(int8(120), 7)) + require.Equal(t, int8(127), SaturatingAdd(int8(120), 8)) +} + +func TestSaturatingSub(t *testing.T) { + // -128 - 100 overflows, so it should return just -128 + require.Equal(t, int8(math.MinInt8), SaturatingSub(int8(math.MinInt8), 100)) + require.Equal(t, int8(0), SaturatingSub(int8(100), 100)) + + // max - (-1) = max + 1 = overflows, so it should return just max + require.Equal(t, int64(math.MaxInt64), SaturatingSub(int64(math.MaxInt64), -1)) + + // 2 - 10 = -8 which overflows, then should return just 0 + require.Equal(t, uint32(0), SaturatingSub(uint32(2), uint32(10))) + require.Equal(t, uint64(math.MaxUint64), SaturatingSub(uint64(math.MaxUint64), uint64(0))) +} From 8e2c9e976dc5a07f625246affd936a0f08be1b79 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Wed, 4 Oct 2023 16:15:35 -0400 Subject: [PATCH 2/7] chore: add one more test --- lib/primitives/math_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/primitives/math_test.go b/lib/primitives/math_test.go index 3551802d6f..2711d00b5d 100644 --- a/lib/primitives/math_test.go +++ b/lib/primitives/math_test.go @@ -12,8 +12,10 @@ import ( func TestSaturatingAdd(t *testing.T) { require.Equal(t, uint8(2), SaturatingAdd(uint8(1), uint8(1))) - require.Equal(t, ^uint8(0), SaturatingAdd(^uint8(0), 100)) - require.Equal(t, ^uint32(0), SaturatingAdd(^uint32(0), 100)) + require.Equal(t, uint8(math.MaxUint8), SaturatingAdd(uint8(math.MaxUint8), 100)) + + require.Equal(t, uint32(math.MaxUint32), SaturatingAdd(uint32(math.MaxUint32), 100)) + require.Equal(t, uint32(100), SaturatingAdd(uint32(0), 100)) // should not be able to overflow in the oposite direction as well require.Equal(t, int64(math.MinInt64), SaturatingAdd(int64(math.MinInt64), -100)) From 044afbe7f354faeb3fa5ca9b73f5fe8cb2d29fc9 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Wed, 4 Oct 2023 16:27:58 -0400 Subject: [PATCH 3/7] chore: avoid code repetition --- lib/primitives/math.go | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/lib/primitives/math.go b/lib/primitives/math.go index 513547d611..65dffafc50 100644 --- a/lib/primitives/math.go +++ b/lib/primitives/math.go @@ -10,8 +10,12 @@ import ( "golang.org/x/exp/constraints" ) -// SaturatingAdd computes a + b saturating at the numeric bounds instead of overflowing -func SaturatingAdd[T constraints.Integer](a, b T) T { +// saturatingOperations applies the correct operation +// given the input types +func saturatingOperations[T constraints.Integer](a, b T, + signedSaturatingOperation func(T, T, T, T) T, + unsignedSaturatingOperation func(T, T) T, +) T { switch any(a).(type) { case int, int8, int16, int32, int64: sizeOf := (unsafe.Sizeof(a) * 8) - 1 @@ -21,16 +25,21 @@ func SaturatingAdd[T constraints.Integer](a, b T) T { minValueOfSignedType T = ^maxValueOfSignedType ) - return saturatingAddSigned(a, b, maxValueOfSignedType, minValueOfSignedType) + return signedSaturatingOperation(a, b, maxValueOfSignedType, minValueOfSignedType) case uint, uint8, uint16, uint32, uint64, uintptr: // the operation ^T(0) gives us the max value of type T // eg. if T is uint8 then it gives us 255 - return saturatingAddUnsigned(a, b, ^T(0)) + return unsignedSaturatingOperation(a, b) } panic(fmt.Sprintf("type %T not supported while performing SaturatingAdd", a)) } +// SaturatingAdd computes a + b saturating at the numeric bounds instead of overflowing +func SaturatingAdd[T constraints.Integer](a, b T) T { + return saturatingOperations(a, b, saturatingAddSigned, saturatingAddUnsigned) +} + func saturatingAddSigned[T constraints.Integer](a, b, max, min T) T { if b > 0 && a > max-b { return max @@ -43,7 +52,11 @@ func saturatingAddSigned[T constraints.Integer](a, b, max, min T) T { return a + b } -func saturatingAddUnsigned[T constraints.Integer](a, b, max T) T { +func saturatingAddUnsigned[T constraints.Integer](a, b T) T { + // the operation ^T(0) gives us the max value of type T + // eg. if T is uint8 then it gives us 255 + max := ^T(0) + if a > max-b { return max } @@ -52,23 +65,7 @@ func saturatingAddUnsigned[T constraints.Integer](a, b, max T) T { // SaturatingSub computes a - b saturating at the numeric bounds instead of overflowing func SaturatingSub[T constraints.Integer](a, b T) T { - switch any(a).(type) { - case int, int8, int16, int32, int64: - sizeOf := (unsafe.Sizeof(a) * 8) - 1 - - var ( - maxValueOfSignedType T = 1< Date: Wed, 4 Oct 2023 16:32:09 -0400 Subject: [PATCH 4/7] chore: ignore deepsource `G103` --- lib/primitives/math.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/primitives/math.go b/lib/primitives/math.go index 65dffafc50..d44a0c1293 100644 --- a/lib/primitives/math.go +++ b/lib/primitives/math.go @@ -18,6 +18,7 @@ func saturatingOperations[T constraints.Integer](a, b T, ) T { switch any(a).(type) { case int, int8, int16, int32, int64: + // #nosec G103 sizeOf := (unsafe.Sizeof(a) * 8) - 1 var ( From 7d9d328a351a4f6abde03b166ada0a2162334142 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Wed, 4 Oct 2023 16:58:34 -0400 Subject: [PATCH 5/7] chore: fix typo --- lib/primitives/math_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/primitives/math_test.go b/lib/primitives/math_test.go index 2711d00b5d..52cfb78db2 100644 --- a/lib/primitives/math_test.go +++ b/lib/primitives/math_test.go @@ -17,7 +17,7 @@ func TestSaturatingAdd(t *testing.T) { require.Equal(t, uint32(math.MaxUint32), SaturatingAdd(uint32(math.MaxUint32), 100)) require.Equal(t, uint32(100), SaturatingAdd(uint32(0), 100)) - // should not be able to overflow in the oposite direction as well + // should not be able to overflow in the opposite direction as well require.Equal(t, int64(math.MinInt64), SaturatingAdd(int64(math.MinInt64), -100)) require.Equal(t, int8(127), SaturatingAdd(int8(120), 7)) require.Equal(t, int8(127), SaturatingAdd(int8(120), 8)) From 31c3832eba691cec9fc343d7cd0ea596c7ea3fa9 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 5 Oct 2023 07:22:02 -0400 Subject: [PATCH 6/7] chore: introduce saturating sub usage at slot.go --- dot/state/slot.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/dot/state/slot.go b/dot/state/slot.go index f926edbf03..665e2ac76f 100644 --- a/dot/state/slot.go +++ b/dot/state/slot.go @@ -11,6 +11,7 @@ import ( "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/internal/database" + "github.com/ChainSafe/gossamer/lib/primitives" "github.com/ChainSafe/gossamer/pkg/scale" ) @@ -48,7 +49,7 @@ func (s *SlotState) CheckEquivocation(slotNow, slot uint64, header *types.Header signer types.AuthorityID) (*types.BabeEquivocationProof, error) { // We don't check equivocations for old headers out of our capacity. // checking slotNow is greater than slot to avoid overflow, same as saturating_sub - if saturatingSub(slotNow, slot) > maxSlotCapacity { + if primitives.SaturatingSub(slotNow, slot) > maxSlotCapacity { return nil, nil } @@ -127,7 +128,7 @@ func (s *SlotState) CheckEquivocation(slotNow, slot uint64, header *types.Header newFirstSavedSlot := firstSavedSlot if slotNow-firstSavedSlot >= pruningBound { - newFirstSavedSlot = saturatingSub(slotNow, maxSlotCapacity) + newFirstSavedSlot = primitives.SaturatingSub(slotNow, maxSlotCapacity) for s := firstSavedSlot; s < newFirstSavedSlot; s++ { slotEncoded := make([]byte, 8) @@ -184,10 +185,3 @@ func (s *SlotState) CheckEquivocation(slotNow, slot uint64, header *types.Header return nil, nil } - -func saturatingSub(a, b uint64) uint64 { - if a > b { - return a - b - } - return 0 -} From b1c8bb012bd35da28ac36a0c8c76a280a3fc7ee7 Mon Sep 17 00:00:00 2001 From: EclesioMeloJunior Date: Thu, 5 Oct 2023 14:23:47 -0400 Subject: [PATCH 7/7] chore: `skipcq: GO-R1005` --- dot/state/slot.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/state/slot.go b/dot/state/slot.go index 665e2ac76f..267a8ada56 100644 --- a/dot/state/slot.go +++ b/dot/state/slot.go @@ -46,7 +46,7 @@ type headerAndSigner struct { } func (s *SlotState) CheckEquivocation(slotNow, slot uint64, header *types.Header, - signer types.AuthorityID) (*types.BabeEquivocationProof, error) { + signer types.AuthorityID) (*types.BabeEquivocationProof, error) { //skipcq: GO-R1005 // We don't check equivocations for old headers out of our capacity. // checking slotNow is greater than slot to avoid overflow, same as saturating_sub if primitives.SaturatingSub(slotNow, slot) > maxSlotCapacity {