fix: share shouldn't validate namespace versions (#98)

Whilst updating `celestia-app`, I realised that we falsely verify the
namespace of the shares using the criteria that it must be a "usable
namespace". This validation shouldn't be here because the protocol uses
arbitrary namespaces when erasure coding. This resolves the problem

---------

Co-authored-by: Rootul P <[email protected]>

Author: cmwaters

share/compact_shares_test.go

@@ -25,7 +25,7 @@ func TestCompactShareSplitter(t *testing.T) {
 	shares, err := css.Export()
 	require.NoError(t, err)
 
-	resTxs, err := parseCompactShares(shares, SupportedShareVersions)
+	resTxs, err := parseCompactShares(shares)
 	require.NoError(t, err)
 
 	assert.Equal(t, txs, resTxs)
@@ -80,7 +80,7 @@ func Test_processCompactShares(t *testing.T) {
 			shares, _, err := splitTxs(txs)
 			require.NoError(t, err)
 
-			parsedTxs, err := parseCompactShares(shares, SupportedShareVersions)
+			parsedTxs, err := parseCompactShares(shares)
 			if err != nil {
 				t.Error(err)
 			}
@@ -97,7 +97,7 @@ func Test_processCompactShares(t *testing.T) {
 
 			txShares, _, err := splitTxs(txs)
 			require.NoError(t, err)
-			parsedTxs, err := parseCompactShares(txShares, SupportedShareVersions)
+			parsedTxs, err := parseCompactShares(txShares)
 			if err != nil {
 				t.Error(err)
 			}
@@ -212,42 +212,6 @@ func TestContiguousCompactShareContainsInfoByte(t *testing.T) {
 	assert.Equal(t, byte(want), infoByte)
 }
 
-func Test_parseCompactSharesErrors(t *testing.T) {
-	type testCase struct {
-		name   string
-		shares []Share
-	}
-
-	txs := generateRandomTxs(2, ContinuationCompactShareContentSize*4)
-	txShares, _, err := splitTxs(txs)
-	require.NoError(t, err)
-	rawShares := ToBytes(txShares)
-
-	unsupportedShareVersion := 5
-	infoByte, _ := NewInfoByte(uint8(unsupportedShareVersion), true)
-	shareWithUnsupportedShareVersionBytes := rawShares[0]
-	shareWithUnsupportedShareVersionBytes[NamespaceSize] = byte(infoByte)
-
-	shareWithUnsupportedShareVersion, err := NewShare(shareWithUnsupportedShareVersionBytes)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	testCases := []testCase{
-		{
-			"share with unsupported share version",
-			[]Share{*shareWithUnsupportedShareVersion},
-		},
-	}
-
-	for _, tt := range testCases {
-		t.Run(tt.name, func(t *testing.T) {
-			_, err := parseCompactShares(tt.shares, SupportedShareVersions)
-			assert.Error(t, err)
-		})
-	}
-}
-
 func generateRandomlySizedTxs(count, maxSize int) [][]byte {
 	txs := make([][]byte, count)
 	for i := 0; i < count; i++ {

share/namespace.go

@@ -9,9 +9,10 @@ type Namespace struct {
 	data []byte
 }
 
-// NewNamespace returns a new namespace with the provided version and id.
+// NewNamespace validates the provided version and id and returns a new namespace.
+// This should be used for user specified namespaces.
 func NewNamespace(version uint8, id []byte) (Namespace, error) {
-	if err := validate(version, id); err != nil {
+	if err := ValidateUserNamespace(version, id); err != nil {
 		return Namespace{}, err
 	}
 
@@ -38,11 +39,12 @@ func MustNewNamespace(version uint8, id []byte) Namespace {
 }
 
 // NewNamespaceFromBytes returns a new namespace from the provided byte slice.
+// This is for user specified namespaces.
 func NewNamespaceFromBytes(bytes []byte) (Namespace, error) {
 	if len(bytes) != NamespaceSize {
 		return Namespace{}, fmt.Errorf("invalid namespace length: %d. Must be %d bytes", len(bytes), NamespaceSize)
 	}
-	if err := validate(bytes[VersionIndex], bytes[NamespaceVersionSize:]); err != nil {
+	if err := ValidateUserNamespace(bytes[VersionIndex], bytes[NamespaceVersionSize:]); err != nil {
 		return Namespace{}, err
 	}
 
@@ -90,7 +92,11 @@ func (n Namespace) ID() []byte {
 	return n.data[NamespaceVersionSize:]
 }
 
-func validate(version uint8, id []byte) error {
+// ValidateUserNamespace returns an error if the provided version is not
+// supported or the provided id does not meet the requirements
+// for the provided version. This should be used for validating
+// user specified namespaces
+func ValidateUserNamespace(version uint8, id []byte) error {
 	err := validateVersionSupported(version)
 	if err != nil {
 		return err

share/parse.go

@@ -8,7 +8,7 @@ import (
 // ParseTxs collects all of the transactions from the shares provided
 func ParseTxs(shares []Share) ([][]byte, error) {
 	// parse the shares
-	rawTxs, err := parseCompactShares(shares, SupportedShareVersions)
+	rawTxs, err := parseCompactShares(shares)
 	if err != nil {
 		return nil, err
 	}

share/parse_compact_shares.go

@@ -6,16 +6,11 @@ package share
 // an error is returned. The returned data [][]byte does not have namespaces,
 // info bytes, data length delimiter, or unit length delimiters and are ready to
 // be unmarshalled.
-func parseCompactShares(shares []Share, supportedShareVersions []uint8) (data [][]byte, err error) {
+func parseCompactShares(shares []Share) (data [][]byte, err error) {
 	if len(shares) == 0 {
 		return nil, nil
 	}
 
-	err = validateShareVersions(shares, supportedShareVersions)
-	if err != nil {
-		return nil, err
-	}
-
 	rawData, err := extractRawData(shares)
 	if err != nil {
 		return nil, err
@@ -29,18 +24,6 @@ func parseCompactShares(shares []Share, supportedShareVersions []uint8) (data []
 	return data, nil
 }
 
-// validateShareVersions returns an error if the shares contain a share with an
-// unsupported share version. Returns nil if all shares contain supported share
-// versions.
-func validateShareVersions(shares []Share, supportedShareVersions []uint8) error {
-	for i := 0; i < len(shares); i++ {
-		if err := shares[i].DoesSupportVersions(supportedShareVersions); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // parseRawData returns the units (transactions, PFB transactions, intermediate
 // state roots) contained in raw data by parsing the unit length delimiter
 // prefixed to each unit.

share/parse_sparse_shares_test.go

@@ -91,38 +91,6 @@ func Test_parseSparseShares(t *testing.T) {
 	}
 }
 
-func Test_parseSparseSharesErrors(t *testing.T) {
-	type testCase struct {
-		name   string
-		shares []Share
-	}
-
-	unsupportedShareVersion := 5
-	infoByte, _ := NewInfoByte(uint8(unsupportedShareVersion), true)
-
-	rawShare := RandomNamespace().Bytes()
-	rawShare = append(rawShare, byte(infoByte))
-	rawShare = append(rawShare, bytes.Repeat([]byte{0}, ShareSize-len(rawShare))...)
-	share, err := NewShare(rawShare)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	tests := []testCase{
-		{
-			"share with unsupported share version",
-			[]Share{*share},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(*testing.T) {
-			_, err := parseSparseShares(tt.shares, SupportedShareVersions)
-			assert.Error(t, err)
-		})
-	}
-}
-
 func Test_parseSparseSharesWithNamespacedPadding(t *testing.T) {
 	sss := NewSparseShareSplitter()
 	randomSmallBlob := generateRandomBlob(ContinuationSparseShareContentSize / 2)

share/shares.go renamed to share/share.go

@@ -23,10 +23,11 @@ func NewShare(data []byte) (*Share, error) {
 	if err := validateSize(data); err != nil {
 		return nil, err
 	}
-	if err := validate(data[0], data[1:NamespaceSize]); err != nil {
+	sh := &Share{data}
+	if err := sh.doesSupportVersions(SupportedShareVersions); err != nil {
 		return nil, err
 	}
-	return &Share{data}, nil
+	return sh, nil
 }
 
 func validateSize(data []byte) error {
@@ -53,8 +54,8 @@ func (s *Share) Version() uint8 {
 	return s.InfoByte().Version()
 }
 
-// DoesSupportVersions checks if the share version is supported
-func (s *Share) DoesSupportVersions(supportedShareVersions []uint8) error {
+// doesSupportVersions checks if the share version is supported
+func (s *Share) doesSupportVersions(supportedShareVersions []uint8) error {
 	ver := s.Version()
 	if !bytes.Contains(supportedShareVersions, []byte{ver}) {
 		return fmt.Errorf("unsupported share version %v is not present in the list of supported share versions %v", ver, supportedShareVersions)

share/shares_test.go renamed to share/share_test.go

@@ -214,3 +214,26 @@ func TestIsPadding(t *testing.T) {
 		})
 	}
 }
+
+func TestUnsupportedShareVersion(t *testing.T) {
+	unsupportedShareVersion := 5
+	infoByte, _ := NewInfoByte(uint8(unsupportedShareVersion), true)
+
+	rawShare := RandomNamespace().Bytes()
+	rawShare = append(rawShare, byte(infoByte))
+	rawShare = append(rawShare, bytes.Repeat([]byte{0}, ShareSize-len(rawShare))...)
+	_, err := NewShare(rawShare)
+	require.Error(t, err)
+}
+
+func TestShareToBytesAndFromBytes(t *testing.T) {
+	blobs := []*Blob{generateRandomBlob(580), generateRandomBlob(380), generateRandomBlob(1100)}
+	SortBlobs(blobs)
+	shares, err := splitBlobs(blobs...)
+	require.NoError(t, err)
+
+	shareBytes := ToBytes(shares)
+	reconstructedShares, err := FromBytes(shareBytes)
+	require.NoError(t, err)
+	assert.Equal(t, shares, reconstructedShares)
+}