fix: validSequenceLen for padding shares (#1827)

Closes #1816 Tagging @jcstein @tuxcanfly for awareness. I plan on cutting a v0.13.3 release after this merges.
celestiaorg · May 24, 2023 · ab64b67 · ab64b67
1 parent 1c51eb3
commit ab64b67
Show file tree

Hide file tree

Showing 3 changed files with 141 additions and 7 deletions.
diff --git a/pkg/shares/parse_test.go b/pkg/shares/parse_test.go
@@ -49,9 +49,6 @@ func TestParseShares(t *testing.T) {
 	largeSequenceLen := 1000 // it takes more than one share to store a sequence of 1000 bytes
 	oneShareWithTooLargeSequenceLen := generateRawShare(blobOneNamespace, start, uint32(largeSequenceLen))
 
-	shortSequenceLen := 0
-	oneShareWithTooShortSequenceLen := generateRawShare(blobOneNamespace, start, uint32(shortSequenceLen))
-
 	tests := []testCase{
 		{
 			"empty",
@@ -130,10 +127,37 @@ func TestParseShares(t *testing.T) {
 			true,
 		},
 		{
-			"one share with too short sequence length",
-			[][]byte{oneShareWithTooShortSequenceLen},
-			[]ShareSequence{},
-			true,
+			"tail padding share",
+			[][]byte{TailPaddingShare()},
+			[]ShareSequence{
+				{
+					NamespaceID: appconsts.TailPaddingNamespaceID,
+					Shares:      []Share{TailPaddingShare()},
+				},
+			},
+			false,
+		},
+		{
+			"reserved padding share",
+			[][]byte{ReservedPaddingShare()},
+			[]ShareSequence{
+				{
+					NamespaceID: appconsts.ReservedPaddingNamespaceID,
+					Shares:      []Share{ReservedPaddingShare()},
+				},
+			},
+			false,
+		},
+		{
+			"namespaced padding share",
+			[][]byte{NamespacePaddingShare(blobOneNamespace)},
+			[]ShareSequence{
+				{
+					NamespaceID: blobOneNamespace,
+					Shares:      []Share{NamespacePaddingShare(blobOneNamespace)},
+				},
+			},
+			false,
 		},
 	}
 	for _, tt := range tests {

diff --git a/pkg/shares/share_sequence.go b/pkg/shares/share_sequence.go
@@ -50,6 +50,14 @@ func (s ShareSequence) validSequenceLen() error {
 	if len(s.Shares) == 0 {
 		return fmt.Errorf("invalid sequence length because share sequence %v has no shares", s)
 	}
+	isPadding, err := s.isPadding()
+	if err != nil {
+		return err
+	}
+	if isPadding {
+		return nil
+	}
+
 	firstShare := s.Shares[0]
 	sharesNeeded, err := numberOfSharesNeeded(firstShare)
 	if err != nil {
@@ -62,6 +70,17 @@ func (s ShareSequence) validSequenceLen() error {
 	return nil
 }
 
+func (s ShareSequence) isPadding() (bool, error) {
+	if len(s.Shares) != 1 {
+		return false, nil
+	}
+	isPadding, err := s.Shares[0].IsPadding()
+	if err != nil {
+		return false, err
+	}
+	return isPadding, nil
+}
+
 // numberOfSharesNeeded extracts the sequenceLen written to the share
 // firstShare and returns the number of shares needed to store a sequence of
 // that length.

diff --git a/pkg/shares/share_sequence_test.go b/pkg/shares/share_sequence_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/celestiaorg/celestia-app/pkg/appconsts"
 	testns "github.com/celestiaorg/celestia-app/testutil/namespace"
+	"github.com/celestiaorg/celestia-app/testutil/testfactory"
 	"github.com/celestiaorg/nmt/namespace"
 	"github.com/stretchr/testify/assert"
 )
@@ -135,3 +136,93 @@ func shareWithData(namespace namespace.ID, isSequenceStart bool, sequenceLen uin
 
 	return padShare(rawShare)
 }
+
+func Test_validSequenceLen(t *testing.T) {
+	type testCase struct {
+		name          string
+		shareSequence ShareSequence
+		wantErr       bool
+	}
+
+	tailPadding := ShareSequence{
+		NamespaceID: appconsts.TailPaddingNamespaceID,
+		Shares:      []Share{TailPaddingShare()},
+	}
+
+	ns1 := bytes.Repeat([]byte{1}, appconsts.NamespaceSize)
+	share := NamespacePaddingShare(ns1)
+	namespacePadding := ShareSequence{
+		NamespaceID: ns1,
+		Shares:      []Share{share},
+	}
+
+	reservedPadding := ShareSequence{
+		NamespaceID: appconsts.ReservedPaddingNamespaceID,
+		Shares:      []Share{ReservedPaddingShare()},
+	}
+
+	notSequenceStart := ShareSequence{
+		NamespaceID: ns1,
+		Shares: []Share{
+			shareWithData(ns1, false, 0, []byte{0x0f}),
+		},
+	}
+
+	testCases := []testCase{
+		{
+			name:          "empty share sequence",
+			shareSequence: ShareSequence{},
+			wantErr:       true,
+		},
+		{
+			name:          "valid share sequence",
+			shareSequence: generateValidShareSequence(t),
+			wantErr:       false,
+		},
+		{
+			name:          "tail padding",
+			shareSequence: tailPadding,
+			wantErr:       false,
+		},
+		{
+			name:          "namespace padding",
+			shareSequence: namespacePadding,
+			wantErr:       false,
+		},
+		{
+			name:          "reserved padding",
+			shareSequence: reservedPadding,
+			wantErr:       false,
+		},
+		{
+			name:          "sequence length where first share is not sequence start",
+			shareSequence: notSequenceStart,
+			wantErr:       true, // error: "share sequence has 1 shares but needed 0 shares"
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.shareSequence.validSequenceLen()
+			if tc.wantErr {
+				assert.Error(t, err)
+				return
+			}
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func generateValidShareSequence(t *testing.T) ShareSequence {
+	css := NewCompactShareSplitter(appconsts.TxNamespaceID, appconsts.ShareVersionZero)
+	txs := testfactory.GenerateRandomTxs(5, 200)
+	for _, tx := range txs {
+		css.WriteTx(tx)
+	}
+	shares, _ := css.Export(0)
+
+	return ShareSequence{
+		NamespaceID: appconsts.TxNamespaceID,
+		Shares:      shares,
+	}
+}