From 7c3a449cf03cbcecf38756978ef40a657df0da5d Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Tue, 30 Jul 2024 19:29:21 +0200 Subject: [PATCH] chore!: remove excess blobTx file and add more test coverage (#99) I forgot to remove the blobTx file when moving it to it's own folder. This PR deletes it and tidies up the imports. I also added a few tests to the marshalling/unmarshalling which weren't previously covered --- builder.go | 8 +-- builder_test.go | 14 ++-- inclusion/commitment.go | 2 +- internal/test/factory.go | 3 +- share/blob.go | 25 ++++--- share/blob_test.go | 142 +++++++++++++++++++++++++++++++++++++++ share/blob_tx.go | 78 --------------------- share/range_test.go | 64 ++++++++++++++++++ share/utils_test.go | 59 ++++++++++++++++ square.go | 14 ++-- square_test.go | 5 +- tx/blob_tx.go | 9 ++- 12 files changed, 312 insertions(+), 111 deletions(-) create mode 100644 share/blob_test.go delete mode 100644 share/blob_tx.go create mode 100644 share/range_test.go diff --git a/builder.go b/builder.go index ad0d434..959bafa 100644 --- a/builder.go +++ b/builder.go @@ -50,8 +50,8 @@ func NewBuilder(maxSquareSize int, subtreeRootThreshold int, txs ...[]byte) (*Bu PfbCounter: share.NewCompactShareCounter(), } seenFirstBlobTx := false - for idx, tx := range txs { - blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx) + for idx, txBytes := range txs { + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes) if err != nil && isBlobTx { return nil, fmt.Errorf("unmarshalling blob tx at index %d: %w", idx, err) } @@ -64,7 +64,7 @@ func NewBuilder(maxSquareSize int, subtreeRootThreshold int, txs ...[]byte) (*Bu if seenFirstBlobTx { return nil, fmt.Errorf("normal tx at index %d can not be appended after blob tx", idx) } - if !builder.AppendTx(tx) { + if !builder.AppendTx(txBytes) { return nil, fmt.Errorf("not enough space to append tx at index %d", idx) } } @@ -88,7 +88,7 @@ func (b *Builder) AppendTx(tx []byte) bool { // AppendBlobTx attempts to allocate the blob transaction to the square. It returns false if there is not // enough space in the square to fit the transaction. -func (b *Builder) AppendBlobTx(blobTx *share.BlobTx) bool { +func (b *Builder) AppendBlobTx(blobTx *tx.BlobTx) bool { iw := tx.NewIndexWrapper(blobTx.Tx, worstCaseShareIndexes(len(blobTx.Blobs))...) size := proto.Size(iw) pfbShareDiff := b.PfbCounter.Add(size) diff --git a/builder_test.go b/builder_test.go index a3029a4..7e5b2f8 100644 --- a/builder_test.go +++ b/builder_test.go @@ -86,7 +86,7 @@ func TestBuilderRejectsBlobTransactions(t *testing.T) { require.NoError(t, err) txs := generateBlobTxsWithNamespaces(ns1.Repeat(len(tc.blobSize)), [][]int{tc.blobSize}) require.Len(t, txs, 1) - blobTx, isBlobTx, err := share.UnmarshalBlobTx(txs[0]) + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txs[0]) require.NoError(t, err) require.True(t, isBlobTx) require.Equal(t, tc.added, builder.AppendBlobTx(blobTx)) @@ -119,11 +119,11 @@ func TestBuilderFindTxShareRange(t *testing.T) { size := dataSquare.Size() * dataSquare.Size() var lastEnd int - for idx, tx := range blockTxs { - blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx) + for idx, txBytes := range blockTxs { + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes) if isBlobTx { require.NoError(t, err) - tx = blobTx.Tx + txBytes = blobTx.Tx } shareRange, err := builder.FindTxShareRange(idx) require.NoError(t, err) @@ -138,7 +138,7 @@ func TestBuilderFindTxShareRange(t *testing.T) { txShares := dataSquare[shareRange.Start : shareRange.End+1] parsedShares, err := rawData(txShares) require.NoError(t, err) - require.True(t, bytes.Contains(parsedShares, tx)) + require.True(t, bytes.Contains(parsedShares, txBytes)) lastEnd = shareRange.End } } @@ -294,8 +294,8 @@ func TestSquareBlobPostions(t *testing.T) { t.Run(fmt.Sprintf("case%d", i), func(t *testing.T) { builder, err := square.NewBuilder(tt.squareSize, defaultSubtreeRootThreshold) require.NoError(t, err) - for _, tx := range tt.blobTxs { - blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx) + for _, txBytes := range tt.blobTxs { + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes) require.NoError(t, err) require.True(t, isBlobTx) _ = builder.AppendBlobTx(blobTx) diff --git a/inclusion/commitment.go b/inclusion/commitment.go index 8e78dbb..c8de38b 100644 --- a/inclusion/commitment.go +++ b/inclusion/commitment.go @@ -107,7 +107,7 @@ func MerkleMountainRangeSizes(totalSize, maxTreeSize uint64) ([]uint64, error) { return treeSizes, nil } -// SplitBlobs splits the provided blobs into shares. +// splitBlobs splits the provided blobs into shares. func splitBlobs(blobs ...*sh.Blob) ([]sh.Share, error) { writer := sh.NewSparseShareSplitter() for _, blob := range blobs { diff --git a/internal/test/factory.go b/internal/test/factory.go index f6aa222..fa33ff5 100644 --- a/internal/test/factory.go +++ b/internal/test/factory.go @@ -7,6 +7,7 @@ import ( "math/rand" "github.com/celestiaorg/go-square/v2/share" + "github.com/celestiaorg/go-square/v2/tx" ) var DefaultTestNamespace = share.MustNewV0Namespace([]byte("test")) @@ -52,7 +53,7 @@ func GenerateBlobTxWithNamespace(namespaces []share.Namespace, blobSizes []int, panic(err) } } - blobTx, err := share.MarshalBlobTx(MockPFB(toUint32(blobSizes)), blobs...) + blobTx, err := tx.MarshalBlobTx(MockPFB(toUint32(blobSizes)), blobs...) if err != nil { panic(err) } diff --git a/share/blob.go b/share/blob.go index fb59bfc..7f934c2 100644 --- a/share/blob.go +++ b/share/blob.go @@ -62,6 +62,18 @@ func UnmarshalBlob(blob []byte) (*Blob, error) { return NewBlobFromProto(pb) } +// Marshal marshals the blob to the proto encoded bytes +func (b *Blob) Marshal() ([]byte, error) { + pb := &v1.BlobProto{ + NamespaceId: b.namespace.ID(), + NamespaceVersion: uint32(b.namespace.Version()), + ShareVersion: uint32(b.shareVersion), + Data: b.data, + Signer: b.signer, + } + return proto.Marshal(pb) +} + // NewBlobFromProto creates a new blob from the proto generated type func NewBlobFromProto(pb *v1.BlobProto) (*Blob, error) { if pb.NamespaceVersion > NamespaceVersionMax { @@ -102,16 +114,9 @@ func (b *Blob) Data() []byte { return b.data } -// Marshal marshals the blob to the proto encoded bytes -func (b *Blob) Marshal() ([]byte, error) { - pb := &v1.BlobProto{ - NamespaceId: b.namespace.ID(), - NamespaceVersion: uint32(b.namespace.Version()), - ShareVersion: uint32(b.shareVersion), - Data: b.data, - Signer: b.signer, - } - return proto.Marshal(pb) +// DataLen returns the length of the data of the blob +func (b *Blob) DataLen() int { + return len(b.data) } // Compare is used to order two blobs based on their namespace diff --git a/share/blob_test.go b/share/blob_test.go new file mode 100644 index 0000000..a4e553b --- /dev/null +++ b/share/blob_test.go @@ -0,0 +1,142 @@ +package share + +import ( + "bytes" + "crypto/rand" + "testing" + + v1 "github.com/celestiaorg/go-square/v2/proto/blob/v1" + "github.com/stretchr/testify/require" +) + +func TestProtoEncoding(t *testing.T) { + signer := make([]byte, 20) + _, err := rand.Read(signer) + require.NoError(t, err) + blob, err := NewBlob(RandomNamespace(), []byte{1, 2, 3, 4, 5}, 1, signer) + require.NoError(t, err) + + blobBytes, err := blob.Marshal() + require.NoError(t, err) + + newBlob, err := UnmarshalBlob(blobBytes) + require.NoError(t, err) + + require.Equal(t, blob, newBlob) +} + +func TestBlobConstructor(t *testing.T) { + signer := make([]byte, 20) + _, err := rand.Read(signer) + require.NoError(t, err) + + ns := RandomNamespace() + data := []byte{1, 2, 3, 4, 5} + + // test all invalid cases + _, err = NewBlob(ns, data, 0, signer) + require.Error(t, err) + require.Contains(t, err.Error(), "share version 0 does not support signer") + + _, err = NewBlob(ns, nil, 0, nil) + require.Error(t, err) + require.Contains(t, err.Error(), "data can not be empty") + + _, err = NewBlob(ns, data, 1, nil) + require.Error(t, err) + require.Contains(t, err.Error(), "share version 1 requires signer of size") + + _, err = NewBlob(ns, data, 128, nil) + require.Error(t, err) + require.Contains(t, err.Error(), "share version can not be greater than MaxShareVersion") +} + +func TestNewBlobFromProto(t *testing.T) { + namespace := RandomNamespace() + testCases := []struct { + name string + proto *v1.BlobProto + expectedErr string + }{ + { + name: "valid blob", + proto: &v1.BlobProto{ + NamespaceId: namespace.ID(), + NamespaceVersion: uint32(namespace.Version()), + ShareVersion: 0, + Data: []byte{1, 2, 3, 4, 5}, + }, + expectedErr: "", + }, + { + name: "invalid namespace version", + proto: &v1.BlobProto{ + NamespaceId: namespace.ID(), + NamespaceVersion: 256, + ShareVersion: 0, + Data: []byte{1, 2, 3, 4, 5}, + }, + expectedErr: "namespace version can not be greater than MaxNamespaceVersion", + }, + { + name: "empty data", + proto: &v1.BlobProto{ + NamespaceId: namespace.ID(), + NamespaceVersion: 0, + ShareVersion: 0, + Data: []byte{}, + }, + expectedErr: "blob data can not be empty", + }, + { + name: "invalid namespace ID length", + proto: &v1.BlobProto{ + NamespaceId: []byte{1, 2, 3}, + NamespaceVersion: 0, + ShareVersion: 0, + Data: []byte{1, 2, 3, 4, 5}, + }, + expectedErr: "invalid namespace", + }, + { + name: "valid blob with signer", + proto: &v1.BlobProto{ + NamespaceId: namespace.ID(), + NamespaceVersion: 0, + ShareVersion: 1, + Data: []byte{1, 2, 3, 4, 5}, + Signer: bytes.Repeat([]byte{1}, SignerSize), + }, + expectedErr: "", + }, + { + name: "invalid signer length", + proto: &v1.BlobProto{ + NamespaceId: namespace.ID(), + NamespaceVersion: 0, + ShareVersion: 1, + Data: []byte{1, 2, 3, 4, 5}, + Signer: []byte{1, 2, 3}, + }, + expectedErr: "share version 1 requires signer of size", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + blob, err := NewBlobFromProto(tc.proto) + if tc.expectedErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedErr) + } else { + require.NoError(t, err) + require.NotNil(t, blob) + require.Equal(t, tc.proto.NamespaceId, blob.Namespace().ID()) + require.Equal(t, uint8(tc.proto.NamespaceVersion), blob.Namespace().Version()) + require.Equal(t, uint8(tc.proto.ShareVersion), blob.ShareVersion()) + require.Equal(t, tc.proto.Data, blob.Data()) + require.Equal(t, tc.proto.Signer, blob.Signer()) + } + }) + } +} diff --git a/share/blob_tx.go b/share/blob_tx.go deleted file mode 100644 index b443d72..0000000 --- a/share/blob_tx.go +++ /dev/null @@ -1,78 +0,0 @@ -package share - -import ( - "errors" - - v1 "github.com/celestiaorg/go-square/v2/proto/blob/v1" - "google.golang.org/protobuf/proto" -) - -const ( - // ProtoBlobTxTypeID is included in each encoded BlobTx to help prevent - // decoding binaries that are not actually BlobTxs. - ProtoBlobTxTypeID = "BLOB" -) - -type BlobTx struct { - Tx []byte - Blobs []*Blob -} - -// UnmarshalBlobTx attempts to unmarshal a transaction into blob transaction. It returns a boolean -// If the bytes are of type BlobTx and an error if there is a problem with decoding -func UnmarshalBlobTx(tx []byte) (*BlobTx, bool, error) { - bTx := v1.BlobTx{} - err := proto.Unmarshal(tx, &bTx) - if err != nil { - return nil, false, err - } - // perform some quick basic checks to prevent false positives - if bTx.TypeId != ProtoBlobTxTypeID { - return nil, false, errors.New("invalid type id") - } - if len(bTx.Blobs) == 0 { - return nil, true, errors.New("no blobs provided") - } - blobs := make([]*Blob, len(bTx.Blobs)) - for i, b := range bTx.Blobs { - blobs[i], err = NewBlobFromProto(b) - if err != nil { - return nil, true, err - } - } - return &BlobTx{ - Tx: bTx.Tx, - Blobs: blobs, - }, true, nil -} - -// MarshalBlobTx creates a BlobTx using a normal transaction and some number of -// blobs. -// -// NOTE: Any checks on the blobs or the transaction must be performed in the -// application -func MarshalBlobTx(tx []byte, blobs ...*Blob) ([]byte, error) { - if len(blobs) == 0 { - return nil, errors.New("at least one blob must be provided") - } - bTx := &v1.BlobTx{ - Tx: tx, - Blobs: blobsToProto(blobs), - TypeId: ProtoBlobTxTypeID, - } - return proto.Marshal(bTx) -} - -func blobsToProto(blobs []*Blob) []*v1.BlobProto { - pb := make([]*v1.BlobProto, len(blobs)) - for i, b := range blobs { - pb[i] = &v1.BlobProto{ - NamespaceId: b.Namespace().ID(), - NamespaceVersion: uint32(b.Namespace().Version()), - ShareVersion: uint32(b.ShareVersion()), - Signer: b.Signer(), - Data: b.Data(), - } - } - return pb -} diff --git a/share/range_test.go b/share/range_test.go new file mode 100644 index 0000000..a353d25 --- /dev/null +++ b/share/range_test.go @@ -0,0 +1,64 @@ +package share_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/celestiaorg/go-square/v2/internal/test" + "github.com/celestiaorg/go-square/v2/share" +) + +func TestGetShareRangeForNamespace(t *testing.T) { + blobs := test.GenerateBlobs(100, 200, 300, 400) + share.SortBlobs(blobs) + writer := share.NewSparseShareSplitter() + for _, blob := range blobs { + err := writer.Write(blob) + require.NoError(t, err) + } + shares := writer.Export() + firstNamespace := shares[0].Namespace() + lastNamespace := shares[len(shares)-1].Namespace() + ns := share.RandomBlobNamespace() + + testCases := []struct { + name string + shares []share.Share + namespace share.Namespace + expectedRange share.Range + }{ + { + name: "Empty shares", + shares: []share.Share{}, + namespace: ns, + expectedRange: share.EmptyRange(), + }, + { + name: "Namespace not found", + shares: shares, + namespace: ns, + expectedRange: share.EmptyRange(), + }, + { + name: "Namespace found", + shares: shares, + namespace: firstNamespace, + expectedRange: share.NewRange(0, 1), + }, + { + name: "Namespace at end", + shares: shares, + namespace: lastNamespace, + expectedRange: share.NewRange(3, 4), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := share.GetShareRangeForNamespace(tc.shares, tc.namespace) + assert.Equal(t, tc.expectedRange, result) + }) + } +} diff --git a/share/utils_test.go b/share/utils_test.go index bb4b065..6899b26 100644 --- a/share/utils_test.go +++ b/share/utils_test.go @@ -51,3 +51,62 @@ func TestParseDelimiter(t *testing.T) { assert.Equal(t, tx, res) } } + +func TestAvailableBytesFromCompactShares(t *testing.T) { + testCases := []struct { + name string + numShares int + expectedBytes int + }{ + { + name: "1 share", + numShares: 1, + expectedBytes: 474, + }, + { + name: "10 shares", + numShares: 10, + expectedBytes: 4776, + }, + { + name: "negative", + numShares: -1, + expectedBytes: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expectedBytes, AvailableBytesFromCompactShares(tc.numShares)) + }) + } +} + +func TestAvailableBytesFromSparseShares(t *testing.T) { + testCases := []struct { + name string + numShares int + expectedBytes int + }{ + { + name: "1 share", + numShares: 1, + expectedBytes: 478, + }, + { + name: "10 shares", + numShares: 10, + expectedBytes: 4816, + }, + { + name: "negative", + numShares: -1, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expectedBytes, AvailableBytesFromSparseShares(tc.numShares)) + }) + } +} diff --git a/square.go b/square.go index 21cde5b..e9fbd08 100644 --- a/square.go +++ b/square.go @@ -24,18 +24,18 @@ func Build(txs [][]byte, maxSquareSize, subtreeRootThreshold int) (Square, [][]b } normalTxs := make([][]byte, 0, len(txs)) blobTxs := make([][]byte, 0, len(txs)) - for idx, tx := range txs { - blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx) + for idx, txBytes := range txs { + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes) if err != nil && isBlobTx { return nil, nil, fmt.Errorf("unmarshalling blob tx at index %d: %w", idx, err) } if isBlobTx { if builder.AppendBlobTx(blobTx) { - blobTxs = append(blobTxs, tx) + blobTxs = append(blobTxs, txBytes) } } else { - if builder.AppendTx(tx) { - normalTxs = append(normalTxs, tx) + if builder.AppendTx(txBytes) { + normalTxs = append(normalTxs, txBytes) } } } @@ -130,11 +130,11 @@ func Deconstruct(s Square, decoder PFBDecoder) ([][]byte, error) { blobs[j] = parsedBlobs[0] } - tx, err := share.MarshalBlobTx(wpfb.Tx, blobs...) + txBytes, err := tx.MarshalBlobTx(wpfb.Tx, blobs...) if err != nil { return nil, err } - txs = append(txs, tx) + txs = append(txs, txBytes) } return txs, nil diff --git a/square_test.go b/square_test.go index baeaded..4a7e9f2 100644 --- a/square_test.go +++ b/square_test.go @@ -8,6 +8,7 @@ import ( "github.com/celestiaorg/go-square/v2" "github.com/celestiaorg/go-square/v2/internal/test" "github.com/celestiaorg/go-square/v2/share" + "github.com/celestiaorg/go-square/v2/tx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -120,8 +121,8 @@ func TestSquareBlobShareRange(t *testing.T) { dataSquare, err := builder.Export() require.NoError(t, err) - for pfbIdx, tx := range txs { - blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx) + for pfbIdx, txBytes := range txs { + blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes) require.NoError(t, err) require.True(t, isBlobTx) for blobIdx := range blobTx.Blobs { diff --git a/tx/blob_tx.go b/tx/blob_tx.go index 435913b..dca1739 100644 --- a/tx/blob_tx.go +++ b/tx/blob_tx.go @@ -2,8 +2,9 @@ package tx import ( "errors" + "fmt" - "github.com/celestiaorg/go-square/v2/proto/blob/v1" + v1 "github.com/celestiaorg/go-square/v2/proto/blob/v1" "github.com/celestiaorg/go-square/v2/share" "google.golang.org/protobuf/proto" ) @@ -56,6 +57,12 @@ func MarshalBlobTx(tx []byte, blobs ...*share.Blob) ([]byte, error) { if len(blobs) == 0 { return nil, errors.New("at least one blob must be provided") } + // nil check + for i, b := range blobs { + if b == nil { + return nil, fmt.Errorf("blob %d is nil", i) + } + } bTx := &v1.BlobTx{ Tx: tx, Blobs: blobsToProto(blobs),