feat: support encoding and decoding of v1 shares #80

cmwaters · 2024-06-19T14:48:41Z

This adds the logic of encoding and decoding the signer bytes to the share (only for the first share in the sequence and only for v1 share versions).

Since we're encapsulating the types, many of the length checks can be removed and thus much data in the square does not need to return an error.

shares/parse.go

vgonkivs

Looks good to me. One idea for further implementation: include signer in the commitment, so we can choose between 2 equal blobs posted by different signers

rach-id · 2024-06-20T11:03:36Z

blob/blob.go

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


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

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

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

Good point, I'll add it to the issue

rach-id · 2024-06-20T11:04:35Z

inclusion/commitment_test.go

+			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},


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

internal/test/factory.go

rach-id · 2024-06-20T11:08:58Z

shares/share_builder.go

+	if b == nil || !b.isFirstShare || b.shareVersion != ShareVersionOne {
+		return
+	}
+	// NOTE: we don't check whether previous data has already been expected


[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

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)

rach-id · 2024-06-20T11:09:19Z

shares/shares.go

 func NewShare(data []byte) (*Share, error) {
 	if err := validateSize(data); err != nil {
 		return nil, err
 	}
+	// TODO: we should also validate namespace


should we create an issue for this?

rach-id · 2024-06-20T11:10:06Z

shares/shares.go

-func (s *Share) Len() int {
-	return len(s.data)
+// Namespace returns the shares namespace
+// TODO: we could validate it the namespace in the constructor


Suggested change

// TODO: we could validate it the namespace in the constructor

// TODO: we could validate the namespace in the constructor

Also similar, do we need an issue for this?

It's the same issue just mentioned in two different places. I will create the issue. Thanks for reminding

rootulp · 2024-06-20T19:06:26Z

blob/blob.go

 // 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)


[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)

rootulp · 2024-06-20T19:08:12Z

blob/blob.go

+		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")


[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)

rootulp · 2024-06-20T19:08:49Z

blob/blob.go

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


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

rootulp · 2024-06-20T19:09:58Z

blob/blob.go

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


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.

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

rootulp · 2024-06-20T19:15:31Z

shares/shares.go

 // SequenceLen returns the sequence length of this *share and optionally an
 // error. It returns 0, nil if this is a continuation share (i.e. doesn't
 // contain a sequence length).


godoc needs updating.

Suggested change

// SequenceLen returns the sequence length of this *share and optionally an

// error. It returns 0, nil if this is a continuation share (i.e. doesn't

// contain a sequence length).

// SequenceLen returns the sequence length of this share.

// It returns 0 if this is a continuation share because then it doesn't contain a sequence length.

cmwaters · 2024-06-21T08:35:03Z

@rootulp I will address your nits in a follow up PR (just so as to not block this)

@Rootul

Closes: #86 This PR also addresses the nits from @Rootul's review [here](#80 (comment))

cmwaters added 11 commits June 14, 2024 10:52

feat: add signer to blob proto

7ab5c0b

Merge branch 'main' into cal/blob-proto

7657933

Merge branch 'main' into cal/blob-proto

5d4b190

rename blob to blob proto

f8a8cbf

Merge branch 'cal/blob-proto' into cal/new-blob-struct

921b730

feat!: create native blob struct

cf6c382

Merge branch 'main' into cal/new-blob-struct

5cb0184

Merge branch 'main' into cal/new-blob-struct

487766e

use nil instead of ""

e517232

fix tests

46e9e0b

feat: support encoding and decoding of v1 shares

a5ae4fe

cmwaters requested review from a team as code owners June 19, 2024 14:48

cmwaters requested review from ramin, rach-id and vgonkivs and removed request for a team June 19, 2024 14:48

celestia-bot requested review from a team June 19, 2024 14:48

Merge branch 'main' into cal/v1-shares

3f48ebf

cmwaters self-assigned this Jun 19, 2024

cristaloleg reviewed Jun 20, 2024

View reviewed changes

shares/parse.go Show resolved Hide resolved

cmwaters added the go-square v2 label Jun 20, 2024

vgonkivs approved these changes Jun 20, 2024

View reviewed changes

rach-id reviewed Jun 20, 2024

View reviewed changes

This was referenced Jun 20, 2024

Document share versions celestiaorg/celestia-app#3715

Open

Validate the namespace in the Share constructor #86

Closed

rootulp approved these changes Jun 20, 2024

View reviewed changes

cmwaters merged commit 7f4a775 into main Jun 21, 2024
11 checks passed

cmwaters mentioned this pull request Jun 21, 2024

feat: validate namespace in share constructor #87

Merged

cmwaters added a commit that referenced this pull request Jun 26, 2024

feat: validate namespace in share constructor (#87)

3ac90d6

Closes: #86 This PR also addresses the nits from @Rootul's review [here](#80 (comment))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support encoding and decoding of v1 shares #80

feat: support encoding and decoding of v1 shares #80

cmwaters commented Jun 19, 2024

vgonkivs left a comment

rach-id Jun 20, 2024

cmwaters Jun 20, 2024

rootulp Jun 20, 2024

cmwaters Jun 21, 2024

rach-id Jun 20, 2024

rach-id Jun 20, 2024

cmwaters Jun 20, 2024

rach-id Jun 20, 2024

rach-id Jun 20, 2024

cmwaters Jun 20, 2024

rootulp Jun 20, 2024

rootulp Jun 20, 2024

rootulp Jun 20, 2024

rootulp Jun 20, 2024

cmwaters Jun 21, 2024

rootulp Jun 20, 2024

cmwaters commented Jun 21, 2024

	// TODO: we could validate it the namespace in the constructor
	// TODO: we could validate the namespace in the constructor

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

	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)

feat: support encoding and decoding of v1 shares #80

feat: support encoding and decoding of v1 shares #80

Conversation

cmwaters commented Jun 19, 2024

vgonkivs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Jun 21, 2024