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
@evan-forbes evan-forbes moved this from TODO to In Progress in Celestia Node Aug 25, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Aug 25, 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

@rootulp rootulp changed the title Use a universal share encoding format Tracking: universal share prefix Oct 13, 2022
@evan-forbes evan-forbes changed the title Tracking: universal share prefix EPIC: Universal share prefix Nov 15, 2022
@evan-forbes evan-forbes moved this to In Progress in Core / App Epics Nov 15, 2022
@evan-forbes evan-forbes added this to the Q4 2022 milestone Nov 15, 2022
@evan-forbes evan-forbes moved this to In Progress in Engineering Team Epics Nov 17, 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
Repository owner moved this from In Progress to Done in Core / App Epics Dec 20, 2022
Repository owner moved this from In Progress to Done in Engineering Team Epics Dec 20, 2022
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