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

feat!: introduce InfoReservedBytes #840

Closed
wants to merge 5 commits into from

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 19, 2022

Description

BREAKING CHANGE: MsgShareSize is now one byte smaller. This change will break clients who parse msg shares (ex. celestia-app)

Part of #839

BREAKING CHANGE: `MsgShareSize` is now one byte smaller. This change
will break clients who parse msg shares (ex. celestia-app)
@rootulp rootulp self-assigned this Aug 19, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Aug 19, 2022

CI failed on:

--- FAIL: TestTxInclusion (2.11s)
panic: input length must be a multiple of 64 [recovered]
	panic: input length must be a multiple of 64

Fixing this involves adding Info to the Message struct

type Message struct {
	// NamespaceID defines the namespace of this message, i.e. the
	// namespace it will use in the namespaced Merkle tree.
	//
	// TODO: spec out constrains and
	// introduce dedicated type instead of just []byte
	NamespaceID namespace.ID

	// Info is a byte with the following structure: the first 7 bits are
	// reserved for version information in big endian form (initially
	// `0000000`). The last bit is a "message start indicator", that is `1` if
	// the share is at the start of a message and `0` otherwise.
	Info byte

	// Data is the actual data contained in the message
	// (e.g. a block of a virtual sidechain).
	Data []byte
}

and then updating usage of Message, e.g. in https://github.com/rootulp/celestia-core/blob/27c3aec0b46bb6e7cbfa759c7968659d58161127/types/share_merging.go#L264

However, I'm aware that share_merging.go is being extracted to celestia-app in celestiaorg/celestia-app#637 so I'm not sure how best to proceed without conflicting with the in-flight refactor effort @evan-forbes

UPDATE: synced with Evan and I'll start working off a branch based on celestiaorg/celestia-app#637 and temporarily ignore the errors in celestia-core b/c much of this code will be deleted post move.

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 22, 2022

It's possible not all of the changes in this PR are necessary. In particular:

@rootulp rootulp closed this Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant