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

Increase share size to 512 bytes #711

Closed
Tracked by #514
adlerjohn opened this issue Apr 15, 2022 · 7 comments
Closed
Tracked by #514

Increase share size to 512 bytes #711

adlerjohn opened this issue Apr 15, 2022 · 7 comments

Comments

@adlerjohn
Copy link
Member

I like big blocks and I cannot lie.

Change ShareSize to 512: https://github.com/celestiaorg/celestia-core/blob/c193de698f889a3c78565ac4a73899326a23d544/pkg/consts/consts.go#L15

This will also require a change to the reserved bytes to 2: https://github.com/celestiaorg/celestia-core/blob/c193de698f889a3c78565ac4a73899326a23d544/pkg/consts/consts.go#L21

@musalbas
Copy link
Member

Why would this require a change to the reserved bytes to 2?

@adlerjohn
Copy link
Member Author

adlerjohn commented Aug 23, 2022

If the reserved byte(s) points to the first byte of the message, it needs to be able to contain [0, 511]. With one byte it can only contain [0, 255].

@rootulp rootulp transferred this issue from celestiaorg/celestia-core Sep 15, 2022
@rootulp
Copy link
Collaborator

rootulp commented Sep 15, 2022

We want to do this as a fast follow after #659

rootulp added a commit that referenced this issue Sep 27, 2022
## Motivation

We will increase the # of reserved bytes from 1 to 2 when we do
#711. The current
implementation hardcodes only 1 reserved byte rather than relying on
`appconsts.CompactShareReservedBytes`.
@rootulp
Copy link
Collaborator

rootulp commented Sep 29, 2022

Based on synchronous discussion, this issue needs investigation to determine what the share size should be.

@evan-forbes
Copy link
Member

to help figure out what breaks, we are doing #825 first, which could potentially be merged and close this issue

@musalbas
Copy link
Member

musalbas commented Oct 3, 2022

ODS 256 byte shares 512 byte shares 1024 byte shares
4 53 MB/s 95 MB/s 169 MB/s
8 116 MB/s 203 MB/s 297 MB/s
16 208 MB/s 367 MB/s 540 MB/s
32 265 MB/s 478 MB/s 697 MB/s
64 300 MB/s 540 MB/s 745 MB/s
128 305 MB/s 534 MB/s 695 MB/s
256 293 MB/s 496 MB/s 634 MB/s
512 276 MB/s 458 MB/s 612 MB/s

512x512 blocks with:

  • 256 byte shares: 67 MB
  • 512 byte shares: 134 MB
  • 1024 byte shares: 268 MB

@rootulp
Copy link
Collaborator

rootulp commented Oct 18, 2022

#850 merged so this issue seems close-able. In the spirit of:

I like big blocks and I cannot lie.

if we can increase share size to 1024 bytes then can we please create a new issue to tackle that?

@rootulp rootulp closed this as completed Oct 18, 2022
rach-id pushed a commit to rach-id/celestia-app that referenced this issue Nov 16, 2022
## Motivation

We will increase the # of reserved bytes from 1 to 2 when we do
celestiaorg#711. The current
implementation hardcodes only 1 reserved byte rather than relying on
`appconsts.CompactShareReservedBytes`.
cmwaters pushed a commit to celestiaorg/go-square that referenced this issue Dec 14, 2023
## Motivation

We will increase the # of reserved bytes from 1 to 2 when we do
celestiaorg/celestia-app#711. The current
implementation hardcodes only 1 reserved byte rather than relying on
`appconsts.CompactShareReservedBytes`.
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
## Motivation

We will increase the # of reserved bytes from 1 to 2 when we do
celestiaorg/celestia-app#711. The current
implementation hardcodes only 1 reserved byte rather than relying on
`appconsts.CompactShareReservedBytes`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

No branches or pull requests

4 participants