feat: throw error if incorrect sequenceLength (celestiaorg#865)

Closes celestiaorg#839

Author: rootulp

pkg/shares/compact_shares_test.go

@@ -123,8 +123,8 @@ func TestCompactShareContainsInfoByte(t *testing.T) {
 
 	infoByte := shares[0][appconsts.NamespaceSize : appconsts.NamespaceSize+appconsts.ShareInfoBytes][0]
 
-	isMessageStart := true
-	want, err := NewInfoByte(appconsts.ShareVersion, isMessageStart)
+	isSequenceStart := true
+	want, err := NewInfoByte(appconsts.ShareVersion, isSequenceStart)
 
 	require.NoError(t, err)
 	assert.Equal(t, byte(want), infoByte)
@@ -143,8 +143,8 @@ func TestContiguousCompactShareContainsInfoByte(t *testing.T) {
 
 	infoByte := shares[1][appconsts.NamespaceSize : appconsts.NamespaceSize+appconsts.ShareInfoBytes][0]
 
-	isMessageStart := false
-	want, err := NewInfoByte(appconsts.ShareVersion, isMessageStart)
+	isSequenceStart := false
+	want, err := NewInfoByte(appconsts.ShareVersion, isSequenceStart)
 
 	require.NoError(t, err)
 	assert.Equal(t, byte(want), infoByte)

pkg/shares/info_byte.go

@@ -8,17 +8,17 @@ import (
 
 // InfoByte is a byte with the following structure: the first 7 bits are
 // reserved for version information in big endian form (initially `0000000`).
-// The last bit is a "message start indicator", that is `1` if the share is at
-// the start of a message and `0` otherwise.
+// The last bit is a "sequence start indicator", that is `1` if this is the
+// first share of a sequence and `0` if this is a continuation share.
 type InfoByte byte
 
-func NewInfoByte(version uint8, isMessageStart bool) (InfoByte, error) {
+func NewInfoByte(version uint8, isSequenceStart bool) (InfoByte, error) {
 	if version > appconsts.MaxShareVersion {
 		return 0, fmt.Errorf("version %d must be less than or equal to %d", version, appconsts.MaxShareVersion)
 	}
 
 	prefix := version << 1
-	if isMessageStart {
+	if isSequenceStart {
 		return InfoByte(prefix + 1), nil
 	}
 	return InfoByte(prefix), nil
@@ -31,13 +31,13 @@ func (i InfoByte) Version() uint8 {
 	return version
 }
 
-// IsMessageStart returns whether this share is the start of a message.
-func (i InfoByte) IsMessageStart() bool {
+// IsSequenceStart returns whether this share is the start of a message.
+func (i InfoByte) IsSequenceStart() bool {
 	return uint(i)%2 == 1
 }
 
 func ParseInfoByte(i byte) (InfoByte, error) {
-	isMessageStart := i%2 == 1
+	isSequenceStart := i%2 == 1
 	version := uint8(i) >> 1
-	return NewInfoByte(version, isMessageStart)
+	return NewInfoByte(version, isSequenceStart)
 }

pkg/shares/info_byte_test.go

@@ -7,8 +7,8 @@ func TestInfoByte(t *testing.T) {
 	notMessageStart := false
 
 	type testCase struct {
-		version        uint8
-		isMessageStart bool
+		version         uint8
+		isSequenceStart bool
 	}
 	tests := []testCase{
 		{0, messageStart},
@@ -23,15 +23,15 @@ func TestInfoByte(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		irb, err := NewInfoByte(test.version, test.isMessageStart)
+		irb, err := NewInfoByte(test.version, test.isSequenceStart)
 		if err != nil {
 			t.Errorf("got %v want no error", err)
 		}
 		if got := irb.Version(); got != test.version {
 			t.Errorf("got version %v want %v", got, test.version)
 		}
-		if got := irb.IsMessageStart(); got != test.isMessageStart {
-			t.Errorf("got isMessageStart %v want %v", got, test.isMessageStart)
+		if got := irb.IsSequenceStart(); got != test.isSequenceStart {
+			t.Errorf("got IsSequenceStart %v want %v", got, test.isSequenceStart)
 		}
 	}
 }
@@ -41,8 +41,8 @@ func TestInfoByteErrors(t *testing.T) {
 	notMessageStart := false
 
 	type testCase struct {
-		version        uint8
-		isMessageStart bool
+		version         uint8
+		isSequenceStart bool
 	}
 
 	tests := []testCase{
@@ -61,11 +61,11 @@ func TestInfoByteErrors(t *testing.T) {
 }
 
 func FuzzNewInfoByte(f *testing.F) {
-	f.Fuzz(func(t *testing.T, version uint8, isMessageStart bool) {
+	f.Fuzz(func(t *testing.T, version uint8, isSequenceStart bool) {
 		if version > 127 {
 			t.Skip()
 		}
-		_, err := NewInfoByte(version, isMessageStart)
+		_, err := NewInfoByte(version, isSequenceStart)
 		if err != nil {
 			t.Errorf("got nil but want error when version > 127")
 		}
@@ -74,9 +74,9 @@ func FuzzNewInfoByte(f *testing.F) {
 
 func TestParseInfoByte(t *testing.T) {
 	type testCase struct {
-		b                  byte
-		wantVersion        uint8
-		wantIsMessageStart bool
+		b                   byte
+		wantVersion         uint8
+		wantisSequenceStart bool
 	}
 
 	tests := []testCase{
@@ -96,8 +96,8 @@ func TestParseInfoByte(t *testing.T) {
 		if got.Version() != test.wantVersion {
 			t.Errorf("got version %v want %v", got.Version(), test.wantVersion)
 		}
-		if got.IsMessageStart() != test.wantIsMessageStart {
-			t.Errorf("got isMessageStart %v want %v", got.IsMessageStart(), test.wantIsMessageStart)
+		if got.IsSequenceStart() != test.wantisSequenceStart {
+			t.Errorf("got IsSequenceStart %v want %v", got.IsSequenceStart(), test.wantisSequenceStart)
 		}
 	}
 }

pkg/shares/parse_compact_shares.go

@@ -56,7 +56,7 @@ func (ss *shareStack) resolve() ([][]byte, error) {
 	if err != nil {
 		panic(err)
 	}
-	if !infoByte.IsMessageStart() {
+	if !infoByte.IsSequenceStart() {
 		return nil, errors.New("first share is not a message start")
 	}
 	err = ss.peel(ss.shares[0][appconsts.NamespaceSize+appconsts.ShareInfoBytes+appconsts.FirstCompactShareSequenceLengthBytes+appconsts.CompactShareReservedBytes:], true)

pkg/shares/parse_sparse_shares.go

@@ -61,8 +61,8 @@ func parseSparseShares(rawShares [][]byte, supportedShareVersions []uint8) ([]co
 			if err != nil {
 				panic(err)
 			}
-			if infoByte.IsMessageStart() != isNewMessage {
-				return nil, fmt.Errorf("expected message start indicator to be %t but got %t", isNewMessage, infoByte.IsMessageStart())
+			if infoByte.IsSequenceStart() != isNewMessage {
+				return nil, fmt.Errorf("expected sequence start indicator to be %t but got %t", isNewMessage, infoByte.IsSequenceStart())
 			}
 			currentMsg = coretypes.Message{
 				NamespaceID: nid,

pkg/shares/share_merging.go

@@ -156,7 +156,7 @@ func ParseShares(rawShares [][]byte) ([]ShareSequence, error) {
 		if err != nil {
 			return sequences, err
 		}
-		if infoByte.IsMessageStart() {
+		if infoByte.IsSequenceStart() {
 			if len(currentSequence.Shares) > 0 {
 				sequences = append(sequences, currentSequence)
 			}
@@ -176,5 +176,78 @@ func ParseShares(rawShares [][]byte) ([]ShareSequence, error) {
 		sequences = append(sequences, currentSequence)
 	}
 
+	for _, sequence := range sequences {
+		if err := sequence.validSequenceLength(); err != nil {
+			return sequences, err
+		}
+	}
+
 	return sequences, nil
 }
+
+// validSequenceLength extracts the sequenceLength written to the first share
+// and returns an error if the number of shares needed to store a sequence of
+// length sequenceLength doesn't match the number of shares in this share
+// sequence. Returns nil if there is no error.
+func (s ShareSequence) validSequenceLength() error {
+	if len(s.Shares) == 0 {
+		return fmt.Errorf("invalid sequence length because share sequence %v has no shares", s)
+	}
+	firstShare := s.Shares[0]
+	sharesNeeded, err := numberOfSharesNeeded(firstShare)
+	if err != nil {
+		return err
+	}
+
+	if len(s.Shares) != sharesNeeded {
+		return fmt.Errorf("share sequence has %d shares but needed %d shares", len(s.Shares), sharesNeeded)
+	}
+	return nil
+}
+
+// numberOfSharesNeeded extracts the sequenceLength written to the share
+// firstShare and returns the number of shares needed to store a sequence of
+// that length.
+func numberOfSharesNeeded(firstShare Share) (sharesUsed int, err error) {
+	sequenceLength, err := firstShare.SequenceLength()
+	if err != nil {
+		return 0, err
+	}
+
+	if firstShare.isCompactShare() {
+		return compactSharesNeeded(int(sequenceLength)), nil
+	}
+	return sparseSharesNeeded(int(sequenceLength)), nil
+}
+
+// compactSharesNeeded returns the number of compact shares needed to store a
+// sequence of length sequenceLength. The parameter sequenceLength is the number
+// of bytes of transaction, intermediate state root, or evidence data in a
+// sequence.
+func compactSharesNeeded(sequenceLength int) (sharesNeeded int) {
+	if sequenceLength == 0 {
+		return 0
+	}
+
+	if sequenceLength < appconsts.FirstCompactShareContentSize {
+		return 1
+	}
+	sequenceLength -= appconsts.FirstCompactShareContentSize
+	sharesNeeded++
+
+	for sequenceLength > 0 {
+		sequenceLength -= appconsts.ContinuationCompactShareContentSize
+		sharesNeeded++
+	}
+	return sharesNeeded
+}
+
+// sparseSharesNeeded returns the number of shares needed to store a sequence of
+// length sequenceLength.
+func sparseSharesNeeded(sequenceLength int) (sharesNeeded int) {
+	sharesNeeded = sequenceLength / appconsts.SparseShareContentSize
+	if sequenceLength%appconsts.SparseShareContentSize != 0 {
+		sharesNeeded++
+	}
+	return sharesNeeded
+}

pkg/shares/share_merging_test.go

@@ -1,12 +1,14 @@
 package shares
 
 import (
+	"encoding/binary"
 	"math/rand"
 	"reflect"
 	"testing"
 
 	"github.com/celestiaorg/celestia-app/pkg/appconsts"
 	"github.com/celestiaorg/nmt/namespace"
+	"github.com/stretchr/testify/assert"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
 	"github.com/tendermint/tendermint/types"
 )
@@ -41,8 +43,14 @@ func TestParseShares(t *testing.T) {
 	messageTwoStart := messageTwoShares[0]
 	messageTwoContinuation := messageTwoShares[1]
 
-	invalidShare := generateRawShare(messageOneNamespace, start)
-	invalidShare = append(invalidShare, []byte{0}...)
+	invalidShare := generateRawShare(messageOneNamespace, start, 1)
+	invalidShare = append(invalidShare, []byte{0}...) // invalidShare is now longer than the length of a valid share
+
+	largeSequenceLength := 1000 // it takes more than one share to store a sequence of 1000 bytes
+	oneShareWithTooLargeSequenceLength := generateRawShare(messageOneNamespace, start, uint64(largeSequenceLength))
+
+	shortSequenceLength := 0
+	oneShareWithTooShortSequenceLength := generateRawShare(messageOneNamespace, start, uint64(shortSequenceLength))
 
 	tests := []testCase{
 		{
@@ -115,12 +123,25 @@ func TestParseShares(t *testing.T) {
 			[]ShareSequence{},
 			true,
 		},
+		{
+			"one share with too large sequence length",
+			[][]byte{oneShareWithTooLargeSequenceLength},
+			[]ShareSequence{},
+			true,
+		},
+		{
+			"one share with too short sequence length",
+			[][]byte{oneShareWithTooShortSequenceLength},
+			[]ShareSequence{},
+			true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := ParseShares(tt.shares)
-			if tt.expectErr && err == nil {
-				t.Errorf("ParseShares() error %v, expectErr %v", err, tt.expectErr)
+			if tt.expectErr {
+				assert.Error(t, err)
+				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("ParseShares() got %v, want %v", got, tt.want)
@@ -129,16 +150,64 @@ func TestParseShares(t *testing.T) {
 	}
 }
 
-func generateRawShare(namespace namespace.ID, isMessageStart bool) (rawShare []byte) {
-	infoByte, _ := NewInfoByte(appconsts.ShareVersion, isMessageStart)
-	rawData := make([]byte, appconsts.ShareSize-len(rawShare))
-	rand.Read(rawData)
+func Test_compactSharesNeeded(t *testing.T) {
+	type testCase struct {
+		sequenceLength int
+		want           int
+	}
+	testCases := []testCase{
+		{0, 0},
+		{1, 1},
+		{2, 1},
+		{appconsts.FirstCompactShareContentSize, 1},
+		{appconsts.FirstCompactShareContentSize + 1, 2},
+		{appconsts.FirstCompactShareContentSize + appconsts.ContinuationCompactShareContentSize, 2},
+		{appconsts.FirstCompactShareContentSize + appconsts.ContinuationCompactShareContentSize*100, 101},
+	}
+	for _, tc := range testCases {
+		got := compactSharesNeeded(tc.sequenceLength)
+		assert.Equal(t, tc.want, got)
+	}
+}
+
+func Test_sparseSharesNeeded(t *testing.T) {
+	type testCase struct {
+		sequenceLength int
+		want           int
+	}
+	testCases := []testCase{
+		{0, 0},
+		{1, 1},
+		{2, 1},
+		{appconsts.SparseShareContentSize, 1},
+		{appconsts.SparseShareContentSize + 1, 2},
+		{appconsts.SparseShareContentSize * 2, 2},
+		{appconsts.SparseShareContentSize*100 + 1, 101},
+	}
+	for _, tc := range testCases {
+		got := sparseSharesNeeded(tc.sequenceLength)
+		assert.Equal(t, tc.want, got)
+	}
+}
+
+func generateRawShare(namespace namespace.ID, isSequenceStart bool, sequenceLength uint64) (rawShare []byte) {
+	infoByte, _ := NewInfoByte(appconsts.ShareVersion, isSequenceStart)
+
+	sequenceLengthVarint := make([]byte, binary.MaxVarintLen64)
+	numBytesWritten := binary.PutUvarint(sequenceLengthVarint, sequenceLength)
 
 	rawShare = append(rawShare, namespace...)
 	rawShare = append(rawShare, byte(infoByte))
-	rawShare = append(rawShare, rawData...)
+	rawShare = append(rawShare, sequenceLengthVarint[:numBytesWritten]...)
+
+	return padWithRandomBytes(rawShare)
+}
 
-	return rawShare
+func padWithRandomBytes(partialShare Share) (paddedShare Share) {
+	paddedShare = make([]byte, appconsts.ShareSize)
+	copy(paddedShare, partialShare)
+	rand.Read(paddedShare[len(partialShare):])
+	return paddedShare
 }
 
 func generateRandomTxs(count, size int) types.Txs {