Skip to content

Commit

Permalink
fix(inclusion): fix integer overflow+inefficient Round*PowerOfTwo
Browse files Browse the repository at this point in the history
This code fixes an integer flow using finer bit twiddling
and even makes it much more efficient to always run
deterministically in O(1) time essentially and not O(k) where
k=log2(input) due to the prior k iterations that then caused
the overflow when result became > maxInt.

While here also added some benchmarks and more test cases.

Fixes #117
  • Loading branch information
odeke-em committed Oct 17, 2024
1 parent 8e9b275 commit 4965d68
Show file tree
Hide file tree
Showing 2 changed files with 185 additions and 97 deletions.
29 changes: 21 additions & 8 deletions inclusion/blob_share_commitment_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package inclusion
import (
"fmt"
"math"
"math/bits"

"golang.org/x/exp/constraints"
)
Expand Down Expand Up @@ -50,23 +51,32 @@ func RoundUpByMultipleOf(cursor, v int) int {

// RoundUpPowerOfTwo returns the next power of two greater than or equal to input.
func RoundUpPowerOfTwo[I constraints.Integer](input I) I {
var result I = 1
for result < input {
result <<= 1
if input <= 1 {
return 1
}
return result
if input&(input-1) == 0 { // It is already a power of 2
return input
}
var powUp I = 1 << bits.Len64(uint64(input))
if powUp < input {
// An overflow occured due to a very large size

Check failure on line 62 in inclusion/blob_share_commitment_rules.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint

`occured` is a misspelling of `occurred` (misspell)
// of input and we should return a positive power of 2.
powUp = 1
}
return powUp
}

// RoundDownPowerOfTwo returns the next power of two less than or equal to input.
func RoundDownPowerOfTwo[I constraints.Integer](input I) (I, error) {
if input <= 0 {
return 0, fmt.Errorf("input %v must be positive", input)
}
roundedUp := RoundUpPowerOfTwo(input)
if roundedUp == input {
return roundedUp, nil
if input&(input-1) == 0 { // It is already a power of 2
return input, nil
}
return roundedUp / 2, nil

// Return 1 << (numberOfBits-1)
return 1 << (bits.Len64(uint64(input)) - 1), nil
}

// BlobMinSquareSize returns the minimum square size that can contain shareCount
Expand All @@ -79,6 +89,9 @@ func BlobMinSquareSize(shareCount int) int {
// commitment over a given blob. The input should be the total number of shares
// used by that blob. See ADR-013.
func SubTreeWidth(shareCount, subtreeRootThreshold int) int {
if subtreeRootThreshold <= 0 {
return 1
}
// Per ADR-013, we use a predetermined threshold to determine width of sub
// trees used to create share commitments
s := (shareCount / subtreeRootThreshold)
Expand Down
253 changes: 164 additions & 89 deletions inclusion/blob_share_commitment_rules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package inclusion_test

import (
"fmt"
"math"
"testing"

"github.com/celestiaorg/go-square/v2/inclusion"
Expand Down Expand Up @@ -257,25 +258,35 @@ func TestRoundUpByMultipleOf(t *testing.T) {
}
}

type roundUpTestCase struct {
input int
want int
}

var roundUpTestCases = []roundUpTestCase{
{input: -1, want: 1},
{input: 0, want: 1},
{input: 1, want: 1},
{input: 2, want: 2},
{input: 4, want: 4},
{input: 5, want: 8},
{input: 8, want: 8},
{input: 11, want: 16},
{input: 511, want: 512},
{input: math.MaxInt32 - 1, want: 1<<31},
{input: math.MaxInt32 + 1, want: 1<<31},
{input: math.MaxInt32, want: 1<<31},
{input: math.MaxInt>>1, want: 1<<62},
{input: math.MaxInt, want: 1},
}

func TestRoundUpPowerOfTwo(t *testing.T) {
type testCase struct {
input int
want int
}
testCases := []testCase{
{input: -1, want: 1},
{input: 0, want: 1},
{input: 1, want: 1},
{input: 2, want: 2},
{input: 4, want: 4},
{input: 5, want: 8},
{input: 8, want: 8},
{input: 11, want: 16},
{input: 511, want: 512},
}
for _, tc := range testCases {
got := inclusion.RoundUpPowerOfTwo(tc.input)
assert.Equal(t, tc.want, got)
for _, tc := range roundUpTestCases {
testName := fmt.Sprintf("%d=%x", tc.input, tc.input)
t.Run(testName, func(t *testing.T) {
got := inclusion.RoundUpPowerOfTwo(tc.input)
assert.Equal(t, tc.want, got)
})
}
}

Expand Down Expand Up @@ -326,86 +337,150 @@ func TestBlobMinSquareSize(t *testing.T) {
}
}

type subtreeWidthTestCase struct {
shareCount int
want int
}

var subtreeWidthTestCases = []subtreeWidthTestCase{
{
shareCount: 0,
want: 1,
},
{
shareCount: 1,
want: 1,
},
{
shareCount: 2,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold + 1,
want: 2,
},
{
shareCount: defaultSubtreeRootThreshold - 1,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold * 2,
want: 2,
},
{
shareCount: (defaultSubtreeRootThreshold * 2) + 1,
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 3) - 1,
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 4),
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 5),
want: 8,
},
{
shareCount: (defaultSubtreeRootThreshold * defaultMaxSquareSize) - 1,
want: 128,
},
}

func TestSubTreeWidth(t *testing.T) {
type testCase struct {
shareCount int
want int
}
testCases := []testCase{
{
shareCount: 0,
want: 1,
},
{
shareCount: 1,
want: 1,
},
{
shareCount: 2,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold + 1,
want: 2,
},
{
shareCount: defaultSubtreeRootThreshold - 1,
want: 1,
},
{
shareCount: defaultSubtreeRootThreshold * 2,
want: 2,
},
{
shareCount: (defaultSubtreeRootThreshold * 2) + 1,
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 3) - 1,
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 4),
want: 4,
},
{
shareCount: (defaultSubtreeRootThreshold * 5),
want: 8,
},
{
shareCount: (defaultSubtreeRootThreshold * defaultMaxSquareSize) - 1,
want: 128,
},
}
for i, tc := range testCases {
for i, tc := range subtreeWidthTestCases {
t.Run(fmt.Sprintf("shareCount %d", tc.shareCount), func(t *testing.T) {
got := inclusion.SubTreeWidth(tc.shareCount, defaultSubtreeRootThreshold)
assert.Equal(t, tc.want, got, i)
})
}
}

func TestRoundDownPowerOfTwo(t *testing.T) {
type testCase struct {
input int
want int
var sink any

func BenchmarkSubTreeWidth(b *testing.B) {
b.ReportAllocs()

for i := 0; i < b.N; i++ {
for _, tc := range subtreeWidthTestCases {
got := inclusion.SubTreeWidth(tc.shareCount, defaultSubtreeRootThreshold)
assert.Equal(b, tc.want, got)
sink = got
}
}
testCases := []testCase{
{input: 1, want: 1},
{input: 2, want: 2},
{input: 4, want: 4},
{input: 5, want: 4},
{input: 8, want: 8},
{input: 11, want: 8},
{input: 511, want: 256},

if sink == nil {
b.Fatal("Benchmark did not run!")
}
for _, tc := range testCases {
got, err := inclusion.RoundDownPowerOfTwo(tc.input)
require.NoError(t, err)
assert.Equal(t, tc.want, got)
sink = nil
}

func BenchmarkRoundDownPowerOfTwo(b *testing.B) {
b.ReportAllocs()

for i := 0; i < b.N; i++ {
for _, tc := range roundDownTestCases {
got, _ := inclusion.RoundDownPowerOfTwo(tc.input)
assert.Equal(b, tc.want, got)
sink = got
}
}

if sink == nil {
b.Fatal("Benchmark did not run!")
}
sink = nil
}

func BenchmarkRoundUpPowerOfTwo(b *testing.B) {
b.ReportAllocs()

for i := 0; i < b.N; i++ {
for _, tc := range roundUpTestCases {
got := inclusion.RoundUpPowerOfTwo(tc.input)
assert.Equal(b, tc.want, got)
sink = got
}
}

if sink == nil {
b.Fatal("Benchmark did not run!")
}
sink = nil
}

type roundDownTestCase struct {
input int
want int
}

var roundDownTestCases = []roundDownTestCase{
{input: 1, want: 1},
{input: 2, want: 2},
{input: 4, want: 4},
{input: 5, want: 4},
{input: 8, want: 8},
{input: 11, want: 8},
{input: 511, want: 256},
{input: math.MaxInt32 - 1, want: 1 << 30},
{input: math.MaxInt32, want: 1 << 30},
{input: math.MaxInt32 + 1, want: 1 << 31},
{input: math.MaxInt, want: 1 << 62},
}

func TestRoundDownPowerOfTwo(t *testing.T) {
for _, tc := range roundDownTestCases {
testName := fmt.Sprintf("%d=%x", tc.input, tc.input)
t.Run(testName, func(t *testing.T) {
got, err := inclusion.RoundDownPowerOfTwo(tc.input)
require.NoError(t, err)
assert.Equal(t, tc.want, got)
})
}
}

0 comments on commit 4965d68

Please sign in to comment.