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

EPIC: Universal share prefix #659

Closed
evan-forbes opened this issue Aug 24, 2022 · 4 comments
Closed

EPIC: Universal share prefix #659

evan-forbes opened this issue Aug 24, 2022 · 4 comments
Assignees
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules

Comments

@evan-forbes
Copy link
Member

evan-forbes commented Aug 24, 2022

We want to switch to more universal share encoding format, similar to what is discussed in celestiaorg/celestia-core#839. The main benefit of this approach being that parsing shares should be more similar in that we add a varint length delimiter to the total length of contiguous shares.

nid (8 bytes) | info byte | message length (varint) | share data for shares at the start of a message;
nid (8 bytes) | info byte | share data for other shares.

This means that the current format for contiguous shares (reserved namespace shares) will look like

starting shares (last info bit == 1)
nid (8 bytes) | info byte | message length (varint) | reserved byte | share data
non starting shares (last info bit == 0)
nid (8 bytes) | info byte | reserved byte | share data

note that depending on what we decide for celestiaorg/celestia-core#842, this issue might be moved to core.

@evan-forbes evan-forbes changed the title Universal share encoding format Use a universal share encoding format Aug 24, 2022
@rootulp rootulp closed this as completed Aug 24, 2022
@rootulp rootulp reopened this Aug 24, 2022
@rootulp rootulp self-assigned this Aug 26, 2022
@rootulp rootulp added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Aug 26, 2022
@rootulp
Copy link
Collaborator

rootulp commented Aug 31, 2022

Proposal to rename | message length (varint) | to | data length (varint) | because in universal share encoding, it applies to message shares and non-message shares.

@evan-forbes
Copy link
Member Author

I like that idea. our whole naming scheme has kinda been thrown off since we changed PayForMessage to PayForData (which was a really good change imo). Might just want to refactor the Message struct to ShareData or something and stick everything in that.

@rootulp
Copy link
Collaborator

rootulp commented Sep 2, 2022

I've been confused on this because I've been using Message for message shares and did not think that term applied to shares in the reserved namespace (i.e. transactions, ISRs, evidence). See #660 (comment)

If we've been using Message to also describe data in reserved namespaces, I totally agree with your suggestion and will create an issue for it.

Update: created celestiaorg/celestia-core#848

rach-id pushed a commit to rach-id/celestia-app that referenced this issue Nov 16, 2022
@evan-forbes evan-forbes removed this from the Q4 2022 milestone Dec 8, 2022
@rootulp rootulp added this to the Incentivized Testnet milestone Dec 8, 2022
@rootulp rootulp closed this as completed Dec 20, 2022
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
Status: Done
Development

No branches or pull requests

2 participants