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

refactor: simplify rebuildShares #197

Merged
merged 3 commits into from
Jun 28, 2023
Merged

refactor: simplify rebuildShares #197

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eaeac81
refactor: remove unnecessary conditional
rootulp Jun 26, 2023
1f1904a
refactor: remove unnecessary param
rootulp Jun 26, 2023
fd86dfb
Merge branch 'master' into rp/remove-logic
rootulp Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 2 additions & 42 deletions extendeddatacrossword.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,8 @@ func (eds *ExtendedDataSquare) solveCrosswordRow(
shares[c] = vectorData[c]
}

isExtendedPartIncomplete := !eds.rowRangeNoMissingData(uint(r), eds.originalDataWidth, eds.width)
// Attempt rebuild
rebuiltShares, isDecoded, err := eds.rebuildShares(isExtendedPartIncomplete, shares)
rebuiltShares, isDecoded, err := eds.rebuildShares(shares)
if err != nil {
return false, false, err
}
Expand Down Expand Up @@ -201,9 +200,8 @@ func (eds *ExtendedDataSquare) solveCrosswordCol(

}

isExtendedPartIncomplete := !eds.colRangeNoMissingData(uint(c), eds.originalDataWidth, eds.width)
// Attempt rebuild
rebuiltShares, isDecoded, err := eds.rebuildShares(isExtendedPartIncomplete, shares)
rebuiltShares, isDecoded, err := eds.rebuildShares(shares)
if err != nil {
return false, false, err
}
Expand Down Expand Up @@ -249,7 +247,6 @@ func (eds *ExtendedDataSquare) solveCrosswordCol(
// 2. Whether the original shares could be decoded from the shares parameter.
// 3. [Optional] an error.
func (eds *ExtendedDataSquare) rebuildShares(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this function at all if its just decoding?

Copy link
Collaborator Author

@rootulp rootulp Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, I can explore deleting it entirely in a FLUP issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I'm only creating a new issue rather than tackling in this PR b/c I don't want to dismiss existing approvals 😞 #199

isExtendedPartIncomplete bool,
shares [][]byte,
) ([][]byte, bool, error) {
rebuiltShares, err := eds.codec.Decode(shares)
Expand All @@ -259,25 +256,6 @@ func (eds *ExtendedDataSquare) rebuildShares(
return nil, false, nil
}

if isExtendedPartIncomplete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] I suppose there have not been any tests associated with this part of the code, as no test file has been updated in this PR, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov claims these lines were covered: https://app.codecov.io/gh/celestiaorg/rsmt2d/blob/master/extendeddatacrossword.go

I assume via https://github.com/rootulp/rsmt2d/blob/1f1904acc114b41dff838ad1d530feacc1d9f199/extendeddatacrossword_test.go#L21

// If needed, rebuild the parity shares too.
rebuiltExtendedShares, err := eds.codec.Encode(rebuiltShares[0:eds.originalDataWidth])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] The only potential issue could arise if the parity portion is incomplete and the rebuilt extended shares obtained from rebuiltShares, err := eds.codec.Decode(shares) (the second half of rebuiltShares) and rebuiltExtendedShares, err := eds.codec.Encode(rebuiltShares[0:eds.originalDataWidth]) turn out to be different. Have we conducted any tests to verify this?

In the original code, we initially decode the entire row, and if the extended part (before decoding) is incomplete, we discard that part from the decoded version and replace it with the encoding result. However, in the new version, we retain whatever the decode function provides us with. If we are confident that these two have the same result, then everything looks good to me! (I think they should be the same)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only potential issue could arise if the parity portion is incomplete

I don't expect this to happen because Decode calls Reconstruct here and the godoc for Reconstruct claims:

// If there are too few shards to reconstruct the missing
// ones, ErrTooFewShards will be returned.

Ref: https://github.com/klauspost/reedsolomon/blob/523164698be98f1603cf1235f5a1de17728b2091/reedsolomon.go#L56-L57

if err != nil {
return nil, true, err
}
rebuiltShares = append(
rebuiltShares[0:eds.originalDataWidth],
rebuiltExtendedShares...,
)
} else {
// Otherwise copy them from the EDS.
startIndex := len(shares) - int(eds.originalDataWidth)
rebuiltShares = append(
rebuiltShares[0:eds.originalDataWidth],
shares[startIndex:]...,
)
}

return rebuiltShares, true, nil
}

Expand Down Expand Up @@ -427,21 +405,3 @@ func (eds *ExtendedDataSquare) computeSharesRootWithRebuiltShare(shares [][]byte
}
return tree.Root()
}

func (eds *ExtendedDataSquare) rowRangeNoMissingData(r, start, end uint) bool {
for c := start; c <= end && c < eds.width; c++ {
if eds.squareRow[r][c] == nil {
return false
}
}
return true
}

func (eds *ExtendedDataSquare) colRangeNoMissingData(c, start, end uint) bool {
for r := start; r <= end && r < eds.width; r++ {
if eds.squareRow[r][c] == nil {
return false
}
}
return true
}