Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support encoding and decoding of v1 shares #80

Merged
merged 12 commits into from
Jun 21, 2024
Merged

feat: support encoding and decoding of v1 shares #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7ab5c0b
feat: add signer to blob proto
cmwaters Jun 14, 2024
7657933
Merge branch 'main' into cal/blob-proto
cmwaters Jun 14, 2024
5d4b190
Merge branch 'main' into cal/blob-proto
cmwaters Jun 14, 2024
f8a8cbf
rename blob to blob proto
cmwaters Jun 14, 2024
921b730
Merge branch 'cal/blob-proto' into cal/new-blob-struct
cmwaters Jun 14, 2024
cf6c382
feat!: create native blob struct
cmwaters Jun 14, 2024
5cb0184
Merge branch 'main' into cal/new-blob-struct
cmwaters Jun 18, 2024
487766e
Merge branch 'main' into cal/new-blob-struct
cmwaters Jun 18, 2024
e517232
use nil instead of ""
cmwaters Jun 18, 2024
46e9e0b
fix tests
cmwaters Jun 18, 2024
a5ae4fe
feat: support encoding and decoding of v1 shares
cmwaters Jun 19, 2024
3f48ebf
Merge branch 'main' into cal/v1-shares
cmwaters Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions blob/blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ const ProtoIndexWrapperTypeID = "INDX"
// MaxShareVersion is the maximum value a share version can be. See: [shares.MaxShareVersion].
const MaxShareVersion = 127

// SignerSize is the size of the signer in bytes.
const SignerSize = 20

// Blob (stands for binary large object) is a core type that represents data
// to be submitted to the Celestia network alongside an accompanying namespace
// and optional signer (for proving the signer of the blob)
// and optional signer (for proving the author of the blob)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] I think this was accidental b/c we renamed it to signer in a different PR

Suggested change
// and optional signer (for proving the author of the blob)
// and optional signer (for proving the signer of the blob)

type Blob struct {
namespace ns.Namespace
data []byte
Expand All @@ -37,13 +40,31 @@ type Blob struct {

// New creates a new coretypes.Blob from the provided data after performing
// basic stateless checks over it.
func New(ns ns.Namespace, data []byte, shareVersion uint8, signer []byte) *Blob {
func New(ns ns.Namespace, data []byte, shareVersion uint8, signer []byte) (*Blob, error) {
if shareVersion == 0 && signer != nil {
return nil, errors.New("share version 0 does not support signer")
}
if shareVersion == 1 && len(signer) != SignerSize {
return nil, errors.New("share version 1 requires signer of size 20 bytes")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] use the constant defined above

Suggested change
return nil, errors.New("share version 1 requires signer of size 20 bytes")
return nil, fmt.Errorf("share version 1 requires signer of size %d bytes", SignerSize)

}
return &Blob{
namespace: ns,
data: data,
shareVersion: shareVersion,
signer: signer,
}, nil
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not blocking]
is there a place with v0 and v1 blobs are documented? what do they contain and how they're constructed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we just have this cip: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-21.md

But it makes sense to definitely make sure the new blob type is clearly documented. I will need to think about where the best place for that is. We should probably have it mentioned in docs.celestia.org but perhaps we want to have a go.doc or readme.md in the code to explain it.

Will just jot this down as an issue so I don't forget

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we have https://celestiaorg.github.io/celestia-app/specs/shares.html#share-version so it probably warrants a mention there (eventually)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add it to the issue

func NewV0(ns ns.Namespace, data []byte) *Blob {
blob, err := New(ns, data, 0, nil)
if err != nil {
panic(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this panic kinda scares me. Can a malicious entity provide an invalid v0 namespace to an honest light node or consensus node and cause it to hit this panic? I think it is preferable to bubble up errors from low level code like this and let the application (celestia-app or celestia-node) decide what to do with the error if it encounters one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the panic is not possible, because the namespace is encapsulated so we know it's valid and there's not a signer that could be of an invalid length. However since there could be changes in the future that could make errors acceptable, I think you're right and I will switch this to being an error

}
return blob
}

func NewV1(ns ns.Namespace, data []byte, signer []byte) (*Blob, error) {
return New(ns, data, 1, signer)
}

// NewFromProto creates a Blob from the proto format and performs
Expand All @@ -62,12 +83,12 @@ func NewFromProto(pb *BlobProto) (*Blob, error) {
if err != nil {
return nil, fmt.Errorf("invalid namespace: %w", err)
}
return &Blob{
namespace: ns,
data: pb.Data,
shareVersion: uint8(pb.ShareVersion),
signer: pb.Signer,
}, nil
return New(
ns,
pb.Data,
uint8(pb.ShareVersion),
pb.Signer,
)
}

// Namespace returns the namespace of the blob
Expand Down
12 changes: 11 additions & 1 deletion inclusion/commitment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func TestCreateCommitment(t *testing.T) {
expected []byte
expectErr bool
shareVersion uint8
signer []byte
}
tests := []test{
{
Expand All @@ -81,6 +82,14 @@ func TestCreateCommitment(t *testing.T) {
expected: []byte{0x31, 0xf5, 0x15, 0x6d, 0x5d, 0xb9, 0xa7, 0xf5, 0xb4, 0x3b, 0x29, 0x7a, 0x14, 0xc0, 0x70, 0xc2, 0xcc, 0x4e, 0xf3, 0xd6, 0x9d, 0x87, 0xed, 0x8, 0xad, 0xdd, 0x21, 0x6d, 0x9b, 0x9f, 0xa1, 0x18},
shareVersion: shares.ShareVersionZero,
},
{
name: "blob of one share with signer succeeds",
namespace: ns1,
blob: bytes.Repeat([]byte{0xFF}, shares.AvailableBytesFromSparseShares(2)-blob.SignerSize),
expected: []byte{0x88, 0x3c, 0x74, 0x6, 0x4e, 0x8e, 0x26, 0x27, 0xad, 0x58, 0x8, 0x38, 0x9f, 0x1f, 0x19, 0x24, 0x19, 0x4c, 0x1a, 0xe2, 0x3c, 0x7d, 0xf9, 0x62, 0xc8, 0xd5, 0x6d, 0xf0, 0x62, 0xa9, 0x2b, 0x2b},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not blocking]
how was this value generated? it would be great to have more information on how to generate the test values

shareVersion: shares.ShareVersionOne,
signer: bytes.Repeat([]byte{1}, blob.SignerSize),
},
{
name: "blob with unsupported share version should return error",
namespace: ns1,
Expand All @@ -91,7 +100,8 @@ func TestCreateCommitment(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
blob := blob.New(tt.namespace, tt.blob, tt.shareVersion, nil)
blob, err := blob.New(tt.namespace, tt.blob, tt.shareVersion, tt.signer)
require.NoError(t, err)
res, err := inclusion.CreateCommitment(blob, twoLeafMerkleRoot, defaultSubtreeRootThreshold)
if tt.expectErr {
assert.Error(t, err)
Expand Down
14 changes: 11 additions & 3 deletions internal/test/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,21 @@ func RandomBytes(size int) []byte {
return b
}

func GenerateBlobTxWithNamespace(namespaces []namespace.Namespace, blobSizes []int) []byte {
func GenerateBlobTxWithNamespace(namespaces []namespace.Namespace, blobSizes []int, version uint8) []byte {
blobs := make([]*blob.Blob, len(blobSizes))
if len(namespaces) != len(blobSizes) {
panic("number of namespaces should match number of blob sizes")
}
var err error
rach-id marked this conversation as resolved.
Show resolved Hide resolved
var signer []byte
if version == shares.ShareVersionOne {
signer = RandomBytes(blob.SignerSize)
}
for i, size := range blobSizes {
blobs[i] = blob.New(namespaces[i], RandomBytes(size), shares.DefaultShareVersion, nil)
blobs[i], err = blob.New(namespaces[i], RandomBytes(size), version, signer)
if err != nil {
panic(err)
}
}
blobTx, err := blob.MarshalBlobTx(MockPFB(toUint32(blobSizes)), blobs...)
if err != nil {
Expand All @@ -54,7 +62,7 @@ func GenerateBlobTxWithNamespace(namespaces []namespace.Namespace, blobSizes []i
}

func GenerateBlobTx(blobSizes []int) []byte {
return GenerateBlobTxWithNamespace(Repeat(DefaultTestNamespace, len(blobSizes)), blobSizes)
return GenerateBlobTxWithNamespace(Repeat(DefaultTestNamespace, len(blobSizes)), blobSizes, shares.DefaultShareVersion)
}

func GenerateBlobTxs(numTxs, blobsPerPfb, blobSize int) [][]byte {
Expand Down
8 changes: 1 addition & 7 deletions shares/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,7 @@ func ParseShares(shares []Share, ignorePadding bool) ([]ShareSequence, error) {
currentSequence := ShareSequence{}

for _, share := range shares {
if err := share.Validate(); err != nil {
cristaloleg marked this conversation as resolved.
Show resolved Hide resolved
return sequences, err
}
isStart, err := share.IsSequenceStart()
if err != nil {
return sequences, err
}
isStart := share.IsSequenceStart()
ns, err := share.Namespace()
if err != nil {
return sequences, err
Expand Down
25 changes: 10 additions & 15 deletions shares/parse_sparse_shares.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ type sequence struct {
shareVersion uint8
data []byte
sequenceLen uint32
signer []byte
}

// parseSparseShares iterates through rawShares and parses out individual
Expand All @@ -25,10 +26,7 @@ func parseSparseShares(shares []Share, supportedShareVersions []uint8) (blobs []
sequences := make([]sequence, 0)

for _, share := range shares {
version, err := share.Version()
if err != nil {
return nil, err
}
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)
}
Expand All @@ -41,16 +39,8 @@ func parseSparseShares(shares []Share, supportedShareVersions []uint8) (blobs []
continue
}

isStart, err := share.IsSequenceStart()
if err != nil {
return nil, err
}

if isStart {
sequenceLen, err := share.SequenceLen()
if err != nil {
return nil, err
}
if share.IsSequenceStart() {
sequenceLen := share.SequenceLen()
data, err := share.RawData()
if err != nil {
return nil, err
Expand All @@ -64,6 +54,7 @@ func parseSparseShares(shares []Share, supportedShareVersions []uint8) (blobs []
shareVersion: version,
data: data,
sequenceLen: sequenceLen,
signer: GetSigner(share),
})
} else { // continuation share
if len(sequences) == 0 {
Expand All @@ -81,7 +72,11 @@ func parseSparseShares(shares []Share, supportedShareVersions []uint8) (blobs []
for _, sequence := range sequences {
// trim any padding from the end of the sequence
sequence.data = sequence.data[:sequence.sequenceLen]
blobs = append(blobs, blob.New(sequence.ns, sequence.data, sequence.shareVersion, nil))
blob, err := blob.New(sequence.ns, sequence.data, sequence.shareVersion, sequence.signer)
if err != nil {
return nil, err
}
blobs = append(blobs, blob)
}

return blobs, nil
Expand Down
14 changes: 13 additions & 1 deletion shares/parse_sparse_shares_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,25 @@ func Test_parseSparseSharesWithNamespacedPadding(t *testing.T) {
require.Equal(t, blobs, pblobs)
}

func Test_parseShareVersionOne(t *testing.T) {
v1blob, err := blob.NewV1(ns.MustNewV0(bytes.Repeat([]byte{1}, ns.NamespaceVersionZeroIDSize)), []byte("data"), bytes.Repeat([]byte{1}, blob.SignerSize))
require.NoError(t, err)
v1shares, err := SplitBlobs(v1blob)
require.NoError(t, err)

parsedBlobs, err := parseSparseShares(v1shares, SupportedShareVersions)
require.NoError(t, err)
require.Equal(t, v1blob, parsedBlobs[0])
require.Len(t, parsedBlobs, 1)
}

func generateRandomBlobWithNamespace(namespace ns.Namespace, size int) *blob.Blob {
data := make([]byte, size)
_, err := crand.Read(data)
if err != nil {
panic(err)
}
return blob.New(namespace, data, ShareVersionZero, nil)
return blob.NewV0(namespace, data)
}

func generateRandomBlob(dataSize int) *blob.Blob {
Expand Down
10 changes: 0 additions & 10 deletions shares/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ func TestParseShares(t *testing.T) {
blobTwoStart := blobTwoShares[0]
blobTwoContinuation := blobTwoShares[1]

// invalidShare is longer than the length of a valid share
invalidShare := Share{data: append(generateRawShare(t, ns1, true, 1), []byte{0}...)}

// tooLargeSequenceLen is a single share with too large of a sequence len
// because it takes more than one share to store a sequence of 1000 bytes
tooLargeSequenceLen := generateRawShare(t, ns1, true, uint32(1000))
Expand Down Expand Up @@ -116,13 +113,6 @@ func TestParseShares(t *testing.T) {
},
expectErr: false,
},
{
name: "one share with invalid size",
shares: []Share{invalidShare},
ignorePadding: false,
want: []ShareSequence{},
expectErr: true,
},
{
name: "blob one start followed by blob two continuation",
shares: []Share{blobOneStart, blobTwoContinuation},
Expand Down
11 changes: 11 additions & 0 deletions shares/share_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,17 @@ func (b *Builder) WriteSequenceLen(sequenceLen uint32) error {
return nil
}

// WriteSigner writes the signer's information to the share.
func (b *Builder) WriteSigner(signer []byte) {
// only write the signer if it is the first share and the share version is 1
if b == nil || !b.isFirstShare || b.shareVersion != ShareVersionOne {
return
}
// NOTE: we don't check whether previous data has already been expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit]
I guess this note should also be written in the function docs so that users can see it when they're using it, instead of having to read the code to know

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead I want to make an assertion about the size of the rawShareData before making the append to know whether the other writes have happened. Personally, I'm not sure about the entire builder struct (I think it should at least be private)

// like the sequence length (we just assume it has)
b.rawShareData = append(b.rawShareData, signer...)
}

// FlipSequenceStart flips the sequence start indicator of the share provided
func (b *Builder) FlipSequenceStart() {
infoByteIndex := b.indexOfInfoBytes()
Expand Down
4 changes: 1 addition & 3 deletions shares/share_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,7 @@ func TestShareBuilderWriteSequenceLen(t *testing.T) {
share, err := tc.builder.Build()
require.NoError(t, err)

length, err := share.SequenceLen()
require.NoError(t, err)

length := share.SequenceLen()
assert.Equal(t, tc.wantLen, length)
})
}
Expand Down
7 changes: 2 additions & 5 deletions shares/share_sequence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (s ShareSequence) SequenceLen() (uint32, error) {
return 0, fmt.Errorf("invalid sequence length because share sequence %v has no shares", s)
}
firstShare := s.Shares[0]
return firstShare.SequenceLen()
return firstShare.SequenceLen(), nil
}

// validSequenceLen extracts the sequenceLen written to the first share
Expand Down Expand Up @@ -84,10 +84,7 @@ func (s ShareSequence) isPadding() (bool, error) {
// firstShare and returns the number of shares needed to store a sequence of
// that length.
func numberOfSharesNeeded(firstShare Share) (sharesUsed int, err error) {
sequenceLen, err := firstShare.SequenceLen()
if err != nil {
return 0, err
}
sequenceLen := firstShare.SequenceLen()

isCompact, err := firstShare.IsCompactShare()
if err != nil {
Expand Down
Loading
Loading