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: Add number of tail padding shares to the header #577

Closed
Tracked by #514
evan-forbes opened this issue Jul 28, 2022 · 7 comments
Closed
Tracked by #514

feat: Add number of tail padding shares to the header #577

evan-forbes opened this issue Jul 28, 2022 · 7 comments
Labels
ABCI modifies an ABCI method enhancement New feature or request

Comments

@evan-forbes
Copy link
Member

evan-forbes commented Jul 28, 2022

There will usually be some number of tail padding shares included in the original data square mainly due to the fact that the square size is always a power of two. Since tail padding shares are already known, there's no reason for the light clients to sample these shares (or potentially rows).

We should therefore include either the number of non tail padding shares or the number of tail padding in the header, so that light clients can optimize sampling. This will require changes to the PrepareProposal method, along with keeping track of the number of tail padding shares when creating the square. There will also need to be an additional check in ProcessProposal, to ensure that the number included in the header is actually accurate.

Since we're adding a field to the header, and adding a block validity rule on that new field, this change is consensus breaking. It is a also a good opportunity to figure out how to preserve compatibility of old headers and new headers if we ever include this change in an upgrade to mamaki.

@evan-forbes evan-forbes added enhancement New feature or request C: Celestia app ABCI modifies an ABCI method labels Jul 28, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Jul 28, 2022
@evan-forbes evan-forbes changed the title feat: Add number of parity shares to the header feat: Add number of tail padding shares to the header Jul 28, 2022
@adlerjohn
Copy link
Member

Could the new field be added to the DAH instead of the header proper?

@evan-forbes
Copy link
Member Author

We could, but do we need to include it in the header hash?

continuing our sync conversation:

could most of the benefit be implemented by the light client by hardcoding the row root for a row full of tail padding?

I think this is especially relevant if we don't include this tail padding number in the header hash, otherwise the light client cannot rely on it and has to check for the row roots themselves.

@adlerjohn
Copy link
Member

adlerjohn commented Jul 30, 2022

Hm I see your point. That would allow entire rows that are padding to be elided by implementations. But it wouldn't work for rows that aren't entirely padding. Still could be pretty good.

@liamsi
Copy link
Member

liamsi commented Jul 31, 2022

Could the new field be added to the DAH instead of the header proper?

Is the reason to include this in the DA header that it is a number per row actually instead of a total number of orig shares used? Or simply because it would be more orderly (as it belongs to the DA header in some sense).

@adlerjohn
Copy link
Member

adlerjohn commented Jul 31, 2022

It's the total number of original shares used.

It's a suggestion mostly because it wouldn't involve changing the Tendermint header, but only the Celestia-specific DAH which already has modification logic.

Note that if we do include it in the DAH then any proofs against the data root would be required to provide that field.

@liamsi
Copy link
Member

liamsi commented Jul 31, 2022

My intuition is that it's cleaner to include the field in the header instead but need to think this through 🤔

@evan-forbes
Copy link
Member Author

could most of the benefit be implemented by the light client by hardcoding the row root for a row full of tail padding?

per a synchronous discussion, we think we can most of the benefit by hardcoding roots of full rows of tail padding for different square sizes in the light client, so I'm closing this for now.

pls reopen if we thing differently cc @liamsi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABCI modifies an ABCI method enhancement New feature or request
Projects
No open projects
Archived in project
Development

No branches or pull requests

3 participants