chore: fix share and blob validation (#101)

I've now integrated celestia-app locally. These are the last set of
changes in order to make everything work. These are:
- Remove validation of share verision. Erasure coded data doesn't comply
with this rule. The validation has been added to the parsing function
- Check for empty namespaces in the blob constructor. There's a bit of a
footgun here where users could set an empty namespace.

Author: cmwaters

inclusion/commitment_test.go

@@ -88,13 +88,6 @@ func TestCreateCommitment(t *testing.T) {
 			shareVersion: share.ShareVersionOne,
 			signer:       bytes.Repeat([]byte{1}, share.SignerSize),
 		},
-		{
-			name:         "blob with unsupported share version should return error",
-			namespace:    ns1,
-			blob:         bytes.Repeat([]byte{0xFF}, share.AvailableBytesFromSparseShares(2)),
-			expectErr:    true,
-			shareVersion: uint8(2), // unsupported share version
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

share/blob.go

@@ -25,14 +25,25 @@ func NewBlob(ns Namespace, data []byte, shareVersion uint8, signer []byte) (*Blo
 	if len(data) == 0 {
 		return nil, errors.New("data can not be empty")
 	}
-	if shareVersion == 0 && signer != nil {
-		return nil, errors.New("share version 0 does not support signer")
+	if ns.IsEmpty() {
+		return nil, errors.New("namespace can not be empty")
 	}
-	if shareVersion == 1 && len(signer) != SignerSize {
-		return nil, fmt.Errorf("share version 1 requires signer of size %d bytes", SignerSize)
+	if ns.Version() != NamespaceVersionZero {
+		return nil, fmt.Errorf("namespace version must be %d got %d", NamespaceVersionZero, ns.Version())
 	}
-	if shareVersion > MaxShareVersion {
-		return nil, fmt.Errorf("share version can not be greater than MaxShareVersion %d", MaxShareVersion)
+	switch shareVersion {
+	case ShareVersionZero:
+		if signer != nil {
+			return nil, errors.New("share version 0 does not support signer")
+		}
+	case ShareVersionOne:
+		if len(signer) != SignerSize {
+			return nil, fmt.Errorf("share version 1 requires signer of size %d bytes", SignerSize)
+		}
+	// Note that we don't specifically check that shareVersion is less than 128 as this is caught
+	// by the default case
+	default:
+		return nil, fmt.Errorf("share version %d not supported. Please use 0 or 1", shareVersion)
 	}
 	return &Blob{
 		namespace:    ns,
@@ -79,8 +90,8 @@ func NewBlobFromProto(pb *v1.BlobProto) (*Blob, error) {
 	if pb.NamespaceVersion > NamespaceVersionMax {
 		return nil, errors.New("namespace version can not be greater than MaxNamespaceVersion")
 	}
-	if len(pb.Data) == 0 {
-		return nil, errors.New("blob data can not be empty")
+	if pb.ShareVersion > MaxShareVersion {
+		return nil, fmt.Errorf("share version can not be greater than MaxShareVersion %d", MaxShareVersion)
 	}
 	ns, err := NewNamespace(uint8(pb.NamespaceVersion), pb.NamespaceId)
 	if err != nil {
@@ -124,6 +135,14 @@ func (b *Blob) Compare(other *Blob) int {
 	return b.namespace.Compare(other.namespace)
 }
 
+// IsEmpty returns true if the blob is empty. This is an invalid
+// construction that can only occur if using the nil value. We
+// only check that the data is empty but this also implies that
+// all other fields would have their zero value
+func (b *Blob) IsEmpty() bool {
+	return len(b.data) == 0
+}
+
 // Sort sorts the blobs by their namespace.
 func SortBlobs(blobs []*Blob) {
 	sort.SliceStable(blobs, func(i, j int) bool {

share/blob_test.go

@@ -48,7 +48,21 @@ func TestBlobConstructor(t *testing.T) {
 
 	_, err = NewBlob(ns, data, 128, nil)
 	require.Error(t, err)
-	require.Contains(t, err.Error(), "share version can not be greater than MaxShareVersion")
+	require.Contains(t, err.Error(), "share version 128 not supported")
+
+	_, err = NewBlob(ns, data, 2, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "share version 2 not supported")
+
+	_, err = NewBlob(Namespace{}, data, 1, signer)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "namespace can not be empty")
+
+	ns2, err := NewNamespace(NamespaceVersionMax, ns.ID())
+	require.NoError(t, err)
+	_, err = NewBlob(ns2, data, 0, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "namespace version must be 0")
 }
 
 func TestNewBlobFromProto(t *testing.T) {
@@ -86,7 +100,7 @@ func TestNewBlobFromProto(t *testing.T) {
 				ShareVersion:     0,
 				Data:             []byte{},
 			},
-			expectedErr: "blob data can not be empty",
+			expectedErr: "data can not be empty",
 		},
 		{
 			name: "invalid namespace ID length",

share/namespace.go

@@ -125,6 +125,11 @@ func validateID(version uint8, id []byte) error {
 	return nil
 }
 
+// IsEmpty returns true if the namespace is empty
+func (n Namespace) IsEmpty() bool {
+	return len(n.data) == 0
+}
+
 // IsReserved returns true if the namespace is reserved
 // for the Celestia state machine
 func (n Namespace) IsReserved() bool {

share/parse.go

@@ -7,7 +7,7 @@ import (
 
 // ParseTxs collects all of the transactions from the shares provided
 func ParseTxs(shares []Share) ([][]byte, error) {
-	// parse the shares
+	// parse the shares. Only share version 0 is supported for transactions
 	rawTxs, err := parseCompactShares(shares)
 	if err != nil {
 		return nil, err
@@ -18,7 +18,7 @@ func ParseTxs(shares []Share) ([][]byte, error) {
 
 // ParseBlobs collects all blobs from the shares provided
 func ParseBlobs(shares []Share) ([]*Blob, error) {
-	blobList, err := parseSparseShares(shares, SupportedShareVersions)
+	blobList, err := parseSparseShares(shares)
 	if err != nil {
 		return []*Blob{}, err
 	}

share/parse_compact_shares.go

@@ -1,5 +1,7 @@
 package share
 
+import "fmt"
+
 // parseCompactShares returns data (transactions or intermediate state roots
 // based on the contents of rawShares and supportedShareVersions. If rawShares
 // contains a share with a version that isn't present in supportedShareVersions,
@@ -11,6 +13,12 @@ func parseCompactShares(shares []Share) (data [][]byte, err error) {
 		return nil, nil
 	}
 
+	for _, share := range shares {
+		if share.Version() != ShareVersionZero {
+			return nil, fmt.Errorf("unsupported share version for compact shares %v", share.Version())
+		}
+	}
+
 	rawData, err := extractRawData(shares)
 	if err != nil {
 		return nil, err

share/parse_sparse_shares.go

@@ -16,16 +16,16 @@ type sequence struct {
 // parseSparseShares iterates through rawShares and parses out individual
 // blobs. It returns an error if a rawShare contains a share version that
 // isn't present in supportedShareVersions.
-func parseSparseShares(shares []Share, supportedShareVersions []uint8) (blobs []*Blob, err error) {
+func parseSparseShares(shares []Share) (blobs []*Blob, err error) {
 	if len(shares) == 0 {
 		return nil, nil
 	}
 	sequences := make([]sequence, 0)
 
 	for _, share := range shares {
 		version := share.Version()
-		if !bytes.Contains(supportedShareVersions, []byte{version}) {
-			return nil, fmt.Errorf("unsupported share version %v is not present in supported share versions %v", version, supportedShareVersions)
+		if !bytes.Contains(SupportedShareVersions, []byte{version}) {
+			return nil, fmt.Errorf("unsupported share version %v is not present in supported share versions %v", version, SupportedShareVersions)
 		}
 
 		if share.IsPadding() {

share/parse_sparse_shares_test.go

@@ -60,7 +60,7 @@ func Test_parseSparseShares(t *testing.T) {
 
 			shares, err := splitBlobs(blobs...)
 			require.NoError(t, err)
-			parsedBlobs, err := parseSparseShares(shares, SupportedShareVersions)
+			parsedBlobs, err := parseSparseShares(shares)
 			if err != nil {
 				t.Error(err)
 			}
@@ -77,7 +77,7 @@ func Test_parseSparseShares(t *testing.T) {
 			blobs := generateRandomlySizedBlobs(tc.blobCount, tc.blobSize)
 			shares, err := splitBlobs(blobs...)
 			require.NoError(t, err)
-			parsedBlobs, err := parseSparseShares(shares, SupportedShareVersions)
+			parsedBlobs, err := parseSparseShares(shares)
 			if err != nil {
 				t.Error(err)
 			}
@@ -114,7 +114,7 @@ func Test_parseSparseSharesWithNamespacedPadding(t *testing.T) {
 	require.NoError(t, err)
 
 	shares := sss.Export()
-	pblobs, err := parseSparseShares(shares, SupportedShareVersions)
+	pblobs, err := parseSparseShares(shares)
 	require.NoError(t, err)
 	require.Equal(t, blobs, pblobs)
 }
@@ -125,7 +125,7 @@ func Test_parseShareVersionOne(t *testing.T) {
 	v1shares, err := splitBlobs(v1blob)
 	require.NoError(t, err)
 
-	parsedBlobs, err := parseSparseShares(v1shares, SupportedShareVersions)
+	parsedBlobs, err := parseSparseShares(v1shares)
 	require.NoError(t, err)
 	require.Equal(t, v1blob, parsedBlobs[0])
 	require.Len(t, parsedBlobs, 1)

share/share.go

@@ -23,11 +23,7 @@ func NewShare(data []byte) (*Share, error) {
 	if err := validateSize(data); err != nil {
 		return nil, err
 	}
-	sh := &Share{data}
-	if err := sh.doesSupportVersions(SupportedShareVersions); err != nil {
-		return nil, err
-	}
-	return sh, nil
+	return &Share{data}, nil
 }
 
 func validateSize(data []byte) error {
@@ -54,11 +50,11 @@ func (s *Share) Version() uint8 {
 	return s.InfoByte().Version()
 }
 
-// doesSupportVersions checks if the share version is supported
-func (s *Share) doesSupportVersions(supportedShareVersions []uint8) error {
+// CheckVersionSupported checks if the share version is supported
+func (s *Share) CheckVersionSupported() 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)
+	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)
 	}
 	return nil
 }

share/share_test.go

@@ -222,8 +222,9 @@ func TestUnsupportedShareVersion(t *testing.T) {
 	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)
+	share, err := NewShare(rawShare)
+	require.NoError(t, err)
+	require.Error(t, share.CheckVersionSupported())
 }
 
 func TestShareToBytesAndFromBytes(t *testing.T) {

tx/blob_tx.go

@@ -59,7 +59,7 @@ func MarshalBlobTx(tx []byte, blobs ...*share.Blob) ([]byte, error) {
 	}
 	// nil check
 	for i, b := range blobs {
-		if b == nil {
+		if b == nil || b.IsEmpty() {
 			return nil, fmt.Errorf("blob %d is nil", i)
 		}
 	}