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

test: genRowShares and genOrigRowShares #1049

Closed
rootulp opened this issue Nov 22, 2022 · 0 comments · Fixed by #1276
Closed

test: genRowShares and genOrigRowShares #1049

rootulp opened this issue Nov 22, 2022 · 0 comments · Fixed by #1276
Labels
testing items that are strictly related to adding or extending test coverage

Comments

@rootulp
Copy link
Collaborator

rootulp commented Nov 22, 2022

Context

There exist two functions in pkg/prove/proof.go that do not contain unit tests.

celestia-app/pkg/prove/proof.go

Lines 127 to 128 in 8604be9

// genRowShares progessively generates data square rows from block data
func genRowShares(codec rsmt2d.Codec, data types.Data, startRow, endRow uint64) ([][]shares.Share, error) {

celestia-app/pkg/prove/proof.go

Lines 154 to 157 in 8604be9

// genOrigRowShares progressively generates data square rows for the original
// data square, meaning the rows only half the full square length, as there is
// not erasure data
func genOrigRowShares(data types.Data, startRow, endRow uint64) []shares.Share {

There were previously unit tests that can be used for inspiration when working on this issue. The relevant tests were commented out in #692 and removed in #1027

Proposal

Add unit tests for the functions above

Related

#703
@rootulp rootulp added the testing items that are strictly related to adding or extending test coverage label Nov 22, 2022
@rootulp rootulp mentioned this issue Nov 22, 2022
Merged
@rootulp rootulp mentioned this issue Jan 24, 2023
Merged
evan-forbes pushed a commit that referenced this issue Feb 27, 2023
@rootulp @rach-id
@staheri14 @evan-forbes
feat!: adapt tx inclusion proofs to use share inclusion proofs (#1276)
8a8b058 
Closes celestiaorg/celestia-core#920
Closes #1226 b/c
deletes that code
Closes #1049 b/c
deletes that code

## Description

Previously the logic to generate tx proofs was flawed because it did not
accurately identify which shares a transaction belongs to. Additionally
the struct `TxProof` (originally defined by Tendermint but later
modified in celestia-core) was incomplete because it did not contain
proofs for the inclusion of a set of rows to the data square root. This
PR modifies the TxInclusion logic to return a share inclusion proof
instead of a transaction inclusion proof. The share inclusion proof does
contain proofs for the rows to the data square root. In order for txs to
use share inclusion proofs, `SplitTxs` was updated to keep track of the
range of shares that a tx occupies.

---------

Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: Sanaz Taheri <[email protected]>
cmwaters pushed a commit to celestiaorg/go-square that referenced this issue Dec 14, 2023
@rootulp @rach-id @staheri14
feat!: adapt tx inclusion proofs to use share inclusion proofs (#1276)
2e13319 
Closes celestiaorg/celestia-core#920
Closes celestiaorg/celestia-app#1226 b/c
deletes that code
Closes celestiaorg/celestia-app#1049 b/c
deletes that code

## Description

Previously the logic to generate tx proofs was flawed because it did not
accurately identify which shares a transaction belongs to. Additionally
the struct `TxProof` (originally defined by Tendermint but later
modified in celestia-core) was incomplete because it did not contain
proofs for the inclusion of a set of rows to the data square root. This
PR modifies the TxInclusion logic to return a share inclusion proof
instead of a transaction inclusion proof. The share inclusion proof does
contain proofs for the rows to the data square root. In order for txs to
use share inclusion proofs, `SplitTxs` was updated to keep track of the
range of shares that a tx occupies.

---------

Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: Sanaz Taheri <[email protected]>
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@rootulp @rach-id @staheri14
feat!: adapt tx inclusion proofs to use share inclusion proofs (#1276)
cb9984d 
Closes celestiaorg/celestia-core#920
Closes celestiaorg/celestia-app#1226 b/c
deletes that code
Closes celestiaorg/celestia-app#1049 b/c
deletes that code

## Description

Previously the logic to generate tx proofs was flawed because it did not
accurately identify which shares a transaction belongs to. Additionally
the struct `TxProof` (originally defined by Tendermint but later
modified in celestia-core) was incomplete because it did not contain
proofs for the inclusion of a set of rows to the data square root. This
PR modifies the TxInclusion logic to return a share inclusion proof
instead of a transaction inclusion proof. The share inclusion proof does
contain proofs for the rows to the data square root. In order for txs to
use share inclusion proofs, `SplitTxs` was updated to keep track of the
range of shares that a tx occupies.

---------

Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: Sanaz Taheri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
testing items that are strictly related to adding or extending test coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: adapt tx inclusion proofs to use share inclusion proofs rootulp/celestia-app
1 participant
@rootulp