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.

Sign up for GitHub

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

Jump to bottom

Stop forcing the last reserved byte to 0 for compact shares #779

Closed
rootulp opened this issue Sep 23, 2022 · 2 comments · Fixed by #819
Closed

Stop forcing the last reserved byte to 0 for compact shares #779

rootulp opened this issue Sep 23, 2022 · 2 comments · Fixed by #819
Assignees
@rootulp
Labels
bug Something isn't working consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules

Comments

@rootulp
Copy link
Collaborator

rootulp commented Sep 23, 2022

Question

Why is reserved byte of last compact share forced to zero? This behavior seems undesirable if the last compact share contains a new transaction.

Context

Link

celestia-app/pkg/shares/split_compact_shares.go

Lines 141 to 145 in 907da5f

// here we force the last share reserved byte to be zero to avoid any
// confusion for light clients parsing these shares, as the rest of the
// data after transaction is padding. See
// https://github.com/celestiaorg/celestia-specs/blob/master/src/specs/data_structures.md#share
rawLastShare[appconsts.NamespaceSize+appconsts.ShareInfoBytes+i] = byte(0)

@rootulp rootulp added the discussion inherently unactionable issue for the sole purpose of discussion label Sep 23, 2022
@evan-forbes
Copy link
Member

I'm confused by this as well.

As such, the reserved bytes should use the well-defined zero value to indicate a termination of the transaction stream.

@adlerjohn ?

perhaps we should rethink this now that we are adding the length delimiter to compact shares (formerly contiguous shares)

@rootulp rootulp mentioned this issue Sep 29, 2022
Merged
@rootulp rootulp self-assigned this Sep 29, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 29, 2022

Based on synchronous discussion, it seems like the last transaction share shouldn't force the reserve byte to zero.

@rootulp rootulp added bug Something isn't working and removed discussion inherently unactionable issue for the sole purpose of discussion labels Sep 29, 2022
@evan-forbes evan-forbes added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Sep 29, 2022
@evan-forbes evan-forbes changed the title Why is reserved byte of last compact share forced to zero? Stop forcing the last reserved byte to 0 for compact shares Sep 29, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Sep 29, 2022
@evan-forbes evan-forbes moved this from TODO to In Progress in Celestia Node Sep 29, 2022
@evan-forbes evan-forbes moved this from In Progress to TODO in Celestia Node Sep 29, 2022
rootulp added a commit to rootulp/celestia-app that referenced this issue Sep 29, 2022
@rootulp
feat: stop forcing reserved byte to zero
7b4a44d 
Closes celestiaorg#779
rootulp added a commit to rootulp/celestia-app that referenced this issue Sep 29, 2022
@rootulp
feat!: stop forcing reserved byte to zero
2ee86a4 
Closes celestiaorg#779
This was referenced Sep 29, 2022
Closed
Closed
rootulp added a commit to rootulp/celestia-app that referenced this issue Sep 29, 2022
@rootulp
feat!: stop forcing reserved byte to zero
5ba8f2e 
Closes celestiaorg#779
@rootulp rootulp mentioned this issue Sep 29, 2022
Merged
rootulp added a commit that referenced this issue Sep 30, 2022
@rootulp
feat!: stop forcing reserved byte to zero (#819)
abb9829 
Closes #779
Repository owner moved this from TODO to Done in Celestia Node Sep 30, 2022
rach-id pushed a commit to rach-id/celestia-app that referenced this issue Nov 16, 2022
@rootulp @rach-id
feat!: stop forcing reserved byte to zero (celestiaorg#819)
4082c3b 
Closes celestiaorg#779
cmwaters pushed a commit to celestiaorg/go-square that referenced this issue Dec 14, 2023
@rootulp
feat!: stop forcing reserved byte to zero (#819)
a3c6961 
Closes celestiaorg/celestia-app#779
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@rootulp
feat!: stop forcing reserved byte to zero (#819)
c48311b 
Closes celestiaorg/celestia-app#779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
bug Something isn't working consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: stop forcing reserved byte to zero rootulp/celestia-app
2 participants
@rootulp @evan-forbes