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.

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

types: blob - extract createcommitment #32

Merged
merged 16 commits into from
Jul 28, 2023
Merged

Conversation

tuxcanfly
Copy link
Contributor

@tuxcanfly tuxcanfly commented Jul 17, 2023

Overview

This PR extracts CreateCommitment from celestia-app. This can be used to submit blob commitments with minimal dependencies.

Fixes #25

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@tuxcanfly
Copy link
Contributor Author

tuxcanfly commented Jul 17, 2023

There seems to be an issue that happens only when passing commitment:

unsupported namespace id length: id [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 2 3 4 5 6 7 8] must be 28 bytes but it was 29 bytes

Edit: Fixed. We're reusing type Blob as both proto and container struct so we need to pass namespace id and version separately when passing the blob in CreateCommitment. See: https://github.com/celestiaorg/celestia-node/blob/6a6c68c42ff940467476c0a6876e7d986b0e84aa/blob/blob.go#L128

@gupadhyaya
Copy link
Contributor

@tuxcanfly can you split the code in types/blob/commitment.go to multiple files? its too large and not a good idea to dump everything to one file. you can retain the structure/names as in celestia-app or make up your own meaningful structure/names.

@tuxcanfly
Copy link
Contributor Author

tuxcanfly commented Jul 17, 2023

@gupadhyaya Done. Also modified CreateCommitment and dependencies to use the combined Namespace id and version bytes to ease confusion. This means that any code that's copied from celestia-app in the future needs to be modified to use the full Namespace bytes e.g. in SpareShareSplitter.Write:

appns.New(blob.NamespaceVersion, blob.NamespaceID)

becomes

namespace.From(blob.Namespace)

As a reminder, celestia-core/types/block.go defines separate version and id bytes:

type Blob struct {
	// NamespaceVersion is the version of the namespace. Used in conjunction
	// with NamespaceID to determine the namespace of this blob.
	NamespaceVersion uint8

	// NamespaceID defines the namespace ID of this blob. Used in conjunction
	// with NamespaceVersion to determine the namespace of this blob.
	NamespaceID []byte

	// Data is the actual data of the blob.
	// (e.g. a block of a virtual sidechain).
	Data []byte

	// ShareVersion is the version of the share format that this blob should use
	// when encoded into shares.
	ShareVersion uint8
}

while celestia-node/blob/blob.go defines:

type Blob struct {
	types.Blob `json:"blob"`

	Commitment Commitment `json:"commitment"`

	// the celestia-node's namespace type
	// this is to avoid converting to and from app's type
	namespace share.Namespace
}

where share.Namespace contains combined version and id bytes:

// Namespace represents namespace of a Share.
// Consists of version byte and namespace ID.
type Namespace []byte

Since we need to pass the combined Namespace bytes in the rpc client, it makes sense for us to always use this and not have to maintain two different structs.

tldr: types/blob/blob.go:Blob.Namespace must always contain both id and version bytes and references from CreateCommitment should always be updated to handle this.

@tuxcanfly tuxcanfly requested a review from Wondertan July 18, 2023 13:53
@nashqueue
Copy link
Member

cc @rootulp @evan-forbes

types/blob/share_builder.go Outdated Show resolved Hide resolved
@nashqueue nashqueue requested a review from MSevey July 24, 2023 13:58
MSevey
MSevey previously approved these changes Jul 24, 2023
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

LGTM: just resolve any of my previous comments once the corresponding issues have been created in app

types/share/share.go Outdated Show resolved Hide resolved
types/blob/info_byte.go Outdated Show resolved Hide resolved
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

maybe we could add a readme that has a brief explanation of why the code is currently here, and also points to the original code. That might help the folks that are source diving and trying to link this code to the rest of what is going on.

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I'll defer to other reviewers, but I didn't see anything weird or out of the ordinary 🙂

@gupadhyaya gupadhyaya self-requested a review July 26, 2023 08:44
Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

Thank you !

@gupadhyaya gupadhyaya added this pull request to the merge queue Jul 28, 2023
Merged via the queue into main with commit 2774cb3 Jul 28, 2023
9 checks passed
@gupadhyaya gupadhyaya deleted the tux/extract-createcommitment branch July 28, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request T:dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

types: Blob API needs CreateCommitment
5 participants