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

bug: Allow to set chunkSize to be set in dataSquare #231

Closed
walldiss opened this issue Jul 6, 2023 · 3 comments · Fixed by #236
Closed

bug: Allow to set chunkSize to be set in dataSquare #231

walldiss opened this issue Jul 6, 2023 · 3 comments · Fixed by #236
Assignees
Labels
bug Something isn't working

Comments

@walldiss
Copy link
Member

walldiss commented Jul 6, 2023

If DataSquare initialised without chunks, SetCell will return chunk size mismatch error on setting first chunk with non 0 size. We need a way to create square without chunks, that will allow SetCell and will be possible to use GetCell on. Currently, there is no way to initialise square, providing no chunks and setting chunkSize to non zero value at the same time.

Steps to reproduce:

  1. Create new DataSquare by newDataSquare with data list of 0 length.
  2. Set shares by SetCell

Solution:
Add chunkSize param to the newDataSquare constructor

@walldiss walldiss added the bug Something isn't working label Jul 6, 2023
@Wondertan
Copy link
Member

Additionally, we can follow implicit approach of the constructor and set chunk size on the first SetCell call

@Wondertan
Copy link
Member

@walldiss could you provide an example of the code that is required to make it work correctly? Can be copied/directed from our hotfix

@rootulp
Copy link
Collaborator

rootulp commented Jul 6, 2023

Solution:
Add chunkSize param to the newDataSquare constructor

Since newDataSquare isn't exported, celestia-node invokes ImportExtendedDataSquare. The chunkSize param needs to be added to both newDataSquare and ImportExtendedDataSquare. Additionally, celestia-node is calling ImportExtendedDataSquare on an empty data square which seems odd. Related #85

@rootulp rootulp self-assigned this Jul 6, 2023
rootulp added a commit that referenced this issue Jul 7, 2023
Closes #231

This PR adds a new constructor called `NewExtendedDataSquare` that
allows @celestia-node to explicitly set the `chunkSize` on an empty EDS.
I choose to not drop the existing `ImportExtendedDataSquare` as proposed
in #85 because that would be
public API breaking.

cc: @Wondertan @walldiss
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this issue Aug 1, 2024
Closes celestiaorg/rsmt2d#231

This PR adds a new constructor called `NewExtendedDataSquare` that
allows @celestia-node to explicitly set the `chunkSize` on an empty EDS.
I choose to not drop the existing `ImportExtendedDataSquare` as proposed
in celestiaorg/rsmt2d#85 because that would be
public API breaking.

cc: @Wondertan @walldiss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants