feat: extends error handling to detect byzantine data and unordered shares in tree construction

datasquare.go

@@ -26,6 +26,9 @@ type dataSquare struct {
 	createTreeFn TreeConstructorFn
 }
 
+// newDataSquare populates the data square from the supplied data and treeCreator.
+// No root calculation is performed.
+// data may have nil values.
 func newDataSquare(data [][]byte, treeCreator TreeConstructorFn, chunkSize uint) (*dataSquare, error) {
 	width := int(math.Ceil(math.Sqrt(float64(len(data)))))
 	if width*width != len(data) {

extendeddatacrossword.go

@@ -53,7 +53,8 @@ type ErrByzantineData struct {
 }
 
 func (e *ErrByzantineData) Error() string {
-	return fmt.Sprintf("byzantine %s: %d", e.Axis, e.Index)
+	return fmt.Sprintf(
+		"byzantine %s: %d", e.Axis, e.Index)
 }
 
 // Repair attempts to repair an incomplete extended data
@@ -143,7 +144,7 @@ func (eds *ExtendedDataSquare) solveCrosswordRow(
 		shares[c] = vectorData[c]
 	}
 
-	// Attempt rebuild
+	// Attempt rebuild the row
 	rebuiltShares, isDecoded, err := eds.rebuildShares(shares)
 	if err != nil {
 		return false, false, err
@@ -168,7 +169,7 @@ func (eds *ExtendedDataSquare) solveCrosswordRow(
 		if col[r] != nil {
 			continue // not newly completed
 		}
-		if noMissingData(col, r) { // not completed
+		if noMissingData(col, r) { // completed
 			err := eds.verifyAgainstColRoots(colRoots, uint(c), col, r, rebuiltShares[c])
 			if err != nil {
 				var byzErr *ErrByzantineData
@@ -241,7 +242,7 @@ func (eds *ExtendedDataSquare) solveCrosswordCol(
 		if row[c] != nil {
 			continue // not newly completed
 		}
-		if noMissingData(row, c) { // not completed
+		if noMissingData(row, c) { // completed
 			err := eds.verifyAgainstRowRoots(rowRoots, uint(r), row, c, rebuiltShares[r])
 			if err != nil {
 				var byzErr *ErrByzantineData
@@ -300,35 +301,46 @@ func (eds *ExtendedDataSquare) verifyAgainstRowRoots(
 		root, err = eds.computeSharesRootWithRebuiltShare(oldShares, Row, r, rebuiltIndex, rebuiltShare)
 	}
 	if err != nil {
-		return err
+		// any error during the computation of the root is considered byzantine
+		// the shares are set to nil, as the caller will populate them
+		return &ErrByzantineData{Row, r, nil}
 	}
 
 	if !bytes.Equal(root, rowRoots[r]) {
+		// the shares are set to nil, as the caller will populate them
 		return &ErrByzantineData{Row, r, nil}
 	}
 
 	return nil
 }
 
+// verifyAgainstColRoots checks that the shares of column index `c` match their expected column root available in `colRoots`.
+// `colRoots` is a slice of the expected roots of the columns of the `eds`.
+// `shares` is a slice of the shares of the column index `c` of the `eds`.
+// `rebuiltIndex` is the index of the share that was rebuilt, if any.
+// `rebuiltShare` is the rebuilt share, if any.
+// Returns a ErrByzantineData error if the computed root does not match the expected root or if the root computation fails.
 func (eds *ExtendedDataSquare) verifyAgainstColRoots(
 	colRoots [][]byte,
 	c uint,
-	oldShares [][]byte,
+	shares [][]byte,
 	rebuiltIndex int,
 	rebuiltShare []byte,
 ) error {
 	var root []byte
 	var err error
 	if rebuiltIndex < 0 || rebuiltShare == nil {
-		root, err = eds.computeSharesRoot(oldShares, Col, c)
+		root, err = eds.computeSharesRoot(shares, Col, c)
 	} else {
-		root, err = eds.computeSharesRootWithRebuiltShare(oldShares, Col, c, rebuiltIndex, rebuiltShare)
+		root, err = eds.computeSharesRootWithRebuiltShare(shares, Col, c, rebuiltIndex, rebuiltShare)
 	}
 	if err != nil {
-		return err
+		// the shares are set to nil, as the caller will populate them
+		return &ErrByzantineData{Col, c, nil}
 	}
 
 	if !bytes.Equal(root, colRoots[c]) {
+		// the shares are set to nil, as the caller will populate them
 		return &ErrByzantineData{Col, c, nil}
 	}
 
@@ -354,10 +366,13 @@ func (eds *ExtendedDataSquare) preRepairSanityCheck(
 				// ensure that the roots are equal
 				rowRoot, err := eds.getRowRoot(i)
 				if err != nil {
-					return err
+					// any error regarding the root calculation signifies an issue in the shares e.g., out of order shares
+					// therefore, it should be treated as byzantine data
+					return &ErrByzantineData{Row, i, eds.row(i)}
 				}
 				if !bytes.Equal(rowRoots[i], rowRoot) {
-					return fmt.Errorf("bad root input: row %d expected %v got %v", i, rowRoots[i], rowRoot)
+					// if the roots are not equal, then the data is byzantine
+					return &ErrByzantineData{Row, i, eds.row(i)}
 				}
 				return nil
 			})
@@ -380,14 +395,18 @@ func (eds *ExtendedDataSquare) preRepairSanityCheck(
 				// ensure that the roots are equal
 				colRoot, err := eds.getColRoot(i)
 				if err != nil {
-					return err
+					// any error regarding the root calculation signifies an issue in the shares e.g., out of order shares
+					// therefore, it should be treated as byzantine data
+					return &ErrByzantineData{Col, i, eds.col(i)}
 				}
 				if !bytes.Equal(colRoots[i], colRoot) {
-					return fmt.Errorf("bad root input: col %d expected %v got %v", i, colRoots[i], colRoot)
+					// if the roots are not equal, then the data is byzantine
+					return &ErrByzantineData{Col, i, eds.col(i)}
 				}
 				return nil
 			})
 			errs.Go(func() error {
+				// check if we take the first half of the col and encode it, we get the second half
 				parityShares, err := eds.codec.Encode(eds.colSlice(0, i, eds.originalDataWidth))
 				if err != nil {
 					return err
@@ -415,6 +434,7 @@ func noMissingData(input [][]byte, rebuiltIndex int) bool {
 	return true
 }
 
+// computeSharesRoot calculates the root of the shares for the specified axis (`i`th column or row).
 func (eds *ExtendedDataSquare) computeSharesRoot(shares [][]byte, axis Axis, i uint) ([]byte, error) {
 	tree := eds.createTreeFn(axis, i)
 	for _, d := range shares {
@@ -426,6 +446,7 @@ func (eds *ExtendedDataSquare) computeSharesRoot(shares [][]byte, axis Axis, i u
 	return tree.Root()
 }
 
+// computeSharesRootWithRebuiltShare computes the root of the shares with the rebuilt share `rebuiltShare` at the specified index `rebuiltIndex`.
 func (eds *ExtendedDataSquare) computeSharesRootWithRebuiltShare(shares [][]byte, axis Axis, i uint, rebuiltIndex int, rebuiltShare []byte) ([]byte, error) {
 	tree := eds.createTreeFn(axis, i)
 	for _, d := range shares[:rebuiltIndex] {

extendeddatacrossword_test.go

@@ -7,6 +7,7 @@ import (
 	"math/rand"
 	"testing"
 
+	"github.com/celestiaorg/nmt"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -344,3 +345,143 @@ func createTestEds(codec Codec, shareSize int) *ExtendedDataSquare {
 
 	return eds
 }
+
+func TestCorruptedEdsReturnsErrByzantineData_UorderedShares(t *testing.T) {
+	shareSize := 64
+	namespaceSize := 1
+	one := bytes.Repeat([]byte{1}, shareSize)
+	two := bytes.Repeat([]byte{2}, shareSize)
+	three := bytes.Repeat([]byte{3}, shareSize)
+	sharesValue := []int{1, 2, 3, 4}
+	tests := []struct {
+		name          string
+		coords        [][]uint
+		values        [][]byte
+		wantErr       bool
+		corruptedAxis Axis
+		corruptedInd  uint
+	}{
+		{
+			name:    "no corruption",
+			wantErr: false,
+		},
+		{
+			// disturbs the order of shares in the first row, erases the rest of the eds
+			name:          "rows with unordered shares",
+			wantErr:       true, // repair should error out during root construction
+			corruptedAxis: Row,
+			coords: [][]uint{
+				{0, 0},
+				{0, 1},
+				{1, 0},
+				{1, 1},
+				{1, 2},
+				{1, 3},
+				{2, 0},
+				{2, 1},
+				{2, 2},
+				{2, 3},
+				{3, 0},
+				{3, 1},
+				{3, 2},
+				{3, 3},
+			},
+			values: [][]byte{
+				two, one,
+				nil, nil, nil, nil,
+				nil, nil, nil, nil,
+				nil, nil, nil, nil,
+			},
+			corruptedInd: 0,
+		},
+		{
+			// disturbs the order of shares in the first column, erases the rest of the eds
+			name:          "columns with unordered shares",
+			wantErr:       true, // repair should error out during root construction
+			corruptedAxis: Col,
+			coords: [][]uint{
+				{0, 0},
+				{0, 1},
+				{0, 2},
+				{0, 3},
+				{1, 0},
+				{1, 1},
+				{1, 2},
+				{1, 3},
+				{2, 1},
+				{2, 2},
+				{2, 3},
+				{3, 1},
+				{3, 2},
+				{3, 3},
+			},
+			values: [][]byte{
+				three, nil, nil, nil,
+				one, nil, nil, nil,
+				nil, nil, nil,
+				nil, nil, nil,
+			},
+			corruptedInd: 0,
+		},
+	}
+
+	for codecName, codec := range codecs {
+		t.Run(codecName, func(t *testing.T) {
+			// create a DA header
+			eds := createTestEdsWithNMT(t, codec, shareSize, namespaceSize, 1, 2, 3, 4)
+			assert.NotNil(t, eds)
+			dAHeaderRoots, err := eds.getRowRoots()
+			assert.NoError(t, err)
+
+			dAHeaderCols, err := eds.getColRoots()
+			assert.NoError(t, err)
+			for _, test := range tests {
+				t.Run(test.name, func(t *testing.T) {
+					// create an eds with the given shares
+					corruptEds := createTestEdsWithNMT(t, codec, shareSize, namespaceSize, sharesValue...)
+					assert.NotNil(t, eds)
+					// corrupt it by setting the values at the given coordinates
+					for i, coords := range test.coords {
+						x := coords[0]
+						y := coords[1]
+						corruptEds.setCell(x, y, test.values[i])
+					}
+
+					err = corruptEds.Repair(dAHeaderRoots, dAHeaderCols)
+					assert.Equal(t, err != nil, test.wantErr)
+					if test.wantErr {
+						var byzErr *ErrByzantineData
+						assert.ErrorAs(t, err, &byzErr)
+						errors.As(err, &byzErr)
+						assert.Equal(t, byzErr.Axis, test.corruptedAxis)
+						assert.Equal(t, byzErr.Index, test.corruptedInd)
+					}
+				})
+			}
+		})
+	}
+}
+
+// createTestEdsWithNMT creates an extended data square with the given shares and namespace size.
+// Shares are placed in row-major order.
+// The first namespaceSize bytes of each share are treated as its namespace.
+// Roots of the extended data square are computed using namespace merkle trees.
+func createTestEdsWithNMT(t *testing.T, codec Codec, shareSize, namespaceSize int, sharesValue ...int) *ExtendedDataSquare {
+	// the first namespaceSize bytes of each share are the namespace
+	assert.True(t, shareSize > namespaceSize)
+
+	// create shares of shareSize bytes
+	shares := make([][]byte, len(sharesValue))
+	for i, shareValue := range sharesValue {
+		shares[i] = bytes.Repeat([]byte{byte(shareValue)}, shareSize)
+	}
+	edsWidth := 4            // number of shares per row/column in the extended data square
+	odsWidth := edsWidth / 2 // number of shares per row/column in the original data square
+
+	eds, err := ComputeExtendedDataSquare(shares, codec, newConstructor(uint64(odsWidth), nmt.NamespaceIDSize(namespaceSize)))
+	if err != nil {
+		panic(err)
+	}
+
+	return eds
+}

go.mod

@@ -4,14 +4,17 @@ go 1.20
 
 require (
 	github.com/celestiaorg/merkletree v0.0.0-20210714075610-a84dc3ddbbe4
-	github.com/stretchr/testify v1.7.0
+	github.com/celestiaorg/nmt v0.17.0
+	github.com/stretchr/testify v1.8.4
 )
 
 require (
 	github.com/klauspost/reedsolomon v1.11.1
 	golang.org/x/sync v0.0.0-20220929204114-8fcdb60fdcc0
 )
 
+require github.com/stretchr/objx v0.5.0 // indirect
+
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/klauspost/cpuid/v2 v2.1.1 // indirect
@@ -22,5 +25,5 @@ require (
 	golang.org/x/crypto v0.1.0 // indirect
 	golang.org/x/sys v0.1.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
-	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -1,5 +1,7 @@
 github.com/celestiaorg/merkletree v0.0.0-20210714075610-a84dc3ddbbe4 h1:CJdIpo8n5MFP2MwK0gSRcOVlDlFdQJO1p+FqdxYzmvc=
 github.com/celestiaorg/merkletree v0.0.0-20210714075610-a84dc3ddbbe4/go.mod h1:fzuHnhzj1pUygGz+1ZkB3uQbEUL4htqCGJ4Qs2LwMZA=
+github.com/celestiaorg/nmt v0.17.0 h1:/k8YLwJvuHgT/jQ435zXKaDX811+sYEMXL4B/vYdSLU=
+github.com/celestiaorg/nmt v0.17.0/go.mod h1:ZndCeAR4l9lxm7W51ouoyTo1cxhtFgK+4DpEIkxRA3A=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
@@ -20,8 +22,16 @@ github.com/minio/sha256-simd v1.0.0/go.mod h1:OuYzVNI5vcoYIAmbIvHPl3N3jUzVedXbKy
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
+github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
 github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
+github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 gitlab.com/NebulousLabs/errors v0.0.0-20171229012116-7ead97ef90b8/go.mod h1:ZkMZ0dpQyWwlENaeZVBiQRjhMEZvk6VTXquzl3FOFP8=
 gitlab.com/NebulousLabs/errors v0.0.0-20200929122200-06c536cf6975 h1:L/ENs/Ar1bFzUeKx6m3XjlmBgIUlykX9dzvp5k9NGxc=
 gitlab.com/NebulousLabs/errors v0.0.0-20200929122200-06c536cf6975/go.mod h1:ZkMZ0dpQyWwlENaeZVBiQRjhMEZvk6VTXquzl3FOFP8=
@@ -46,3 +56,5 @@ gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EV
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo=
 gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=