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

fix: verifyEncoding should revert changes in verifiable data #318

Merged
merged 2 commits into from
Apr 29, 2024
Merged

fix: verifyEncoding should revert changes in verifiable data #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
672d09d
fix verifyEncoding to always set rebuild share back
walldiss Apr 19, 2024
38f58f5
revert changes to unrelated test
walldiss Apr 19, 2024
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
22 changes: 10 additions & 12 deletions extendeddatacrossword.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,11 +378,8 @@ func (eds *ExtendedDataSquare) preRepairSanityCheck(
return nil
})
errs.Go(func() error {
parityShares, err := eds.codec.Encode(eds.rowSlice(i, 0, eds.originalDataWidth))
err := eds.verifyEncoding(eds.row(i), noShareInsertion, nil)
if err != nil {
return err
}
if !bytes.Equal(flattenShares(parityShares), flattenShares(eds.rowSlice(i, eds.originalDataWidth, eds.originalDataWidth))) {
return &ErrByzantineData{Row, i, eds.row(i)}
}
return nil
Expand All @@ -407,12 +404,8 @@ func (eds *ExtendedDataSquare) preRepairSanityCheck(
return nil
})
errs.Go(func() error {
// check if we take the first half of the col and encode it, we get the second half
parityShares, err := eds.codec.Encode(eds.colSlice(0, i, eds.originalDataWidth))
err := eds.verifyEncoding(eds.col(i), noShareInsertion, nil)
if err != nil {
return err
}
if !bytes.Equal(flattenShares(parityShares), flattenShares(eds.colSlice(eds.originalDataWidth, i, eds.originalDataWidth))) {
return &ErrByzantineData{Col, i, eds.col(i)}
}
return nil
Expand Down Expand Up @@ -473,7 +466,14 @@ func (eds *ExtendedDataSquare) computeSharesRootWithRebuiltShare(shares [][]byte

// verifyEncoding checks the Reed-Solomon encoding of the provided data.
func (eds *ExtendedDataSquare) verifyEncoding(data [][]byte, rebuiltIndex int, rebuiltShare []byte) error {
data[rebuiltIndex] = rebuiltShare
if rebuiltShare != nil && rebuiltIndex >= 0 {
data[rebuiltIndex] = rebuiltShare
defer func() {
Copy link
Member

Choose a reason for hiding this comment

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

Style: do we want to use defer here? AFAIK, no where else in rsmt2d uses defer, primarily because it's a Go-specific thing that doesn't translate well into a language like C (C being a lower common denominator for readability). I don't have strong opinions, so opening for discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm in favor of using defer because it ensures cleanup operations are closely tied to their corresponding setups. This not only makes the code more robust by preventing omission in multiple return paths but also aligns with good programming practices by keeping related actions together, improving both maintainability and readability.

An alternative would be to manually add a revert of the addition to the data slice in every return statement, which could be error-prone.

Another option is to adopt an immutable approach by creating a copy of the data that includes the extra share, ensuring the original data remains unmodified. However, this method would lead to extra allocations, which is why I haven't used it here.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

Hmmmm. Perhaps creating a copy wouldn't be so bad if it's only for a single row or column. Could you open an issue that someone can work on to investigate the actual performance impact?

Other that than, no further comments. Not blocking merging.

Copy link
Member

Choose a reason for hiding this comment

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

Defer translates well into C or any other language by calling the deferred line after everything else, so we don't gain much by going with the alternative approach and only lose time (investigating performance impact through the issue) and readability(no defer based grouping). Overall, I don't think opening and spending time on the issue is worth it

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I'll defer (get it 😉) to you guys on this.

// revert the change to the data slice after the verification
data[rebuiltIndex] = nil
}()
}

half := len(data) / 2
original := data[:half]
parity, err := eds.codec.Encode(original)
Expand All @@ -483,10 +483,8 @@ func (eds *ExtendedDataSquare) verifyEncoding(data [][]byte, rebuiltIndex int, r

for i := half; i < len(data); i++ {
if !bytes.Equal(data[i], parity[i-half]) {
data[rebuiltIndex] = nil
return errors.New("parity data does not match encoded data")
}
}

return nil
}
33 changes: 33 additions & 0 deletions extendeddatacrossword_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,39 @@ func TestRepairExtendedDataSquare(t *testing.T) {
t.Errorf("did not return an error on trying to repair an unrepairable square")
}
})

t.Run("repair in random order", func(t *testing.T) {
for i := 0; i < 100; i++ {
newEds, err := NewExtendedDataSquare(codec, NewDefaultTree, original.Width(), shareSize)
require.NoError(t, err)
// Randomly set shares in the newEds from the original and repair.
for {
x := rand.Intn(int(original.Width()))
y := rand.Intn(int(original.Width()))
if newEds.GetCell(uint(x), uint(y)) != nil {
continue
}
err = newEds.SetCell(uint(x), uint(y), original.GetCell(uint(x), uint(y)))
require.NoError(t, err)

// Repair square.
err = newEds.Repair(rowRoots, colRoots)
if errors.Is(err, ErrUnrepairableDataSquare) {
continue
}
require.NoError(t, err)
break
}

require.True(t, newEds.Equals(original))
newRowRoots, err := newEds.RowRoots()
require.NoError(t, err)
require.Equal(t, rowRoots, newRowRoots)
newColRoots, err := newEds.ColRoots()
require.NoError(t, err)
require.Equal(t, colRoots, newColRoots)
}
})
}

func TestValidFraudProof(t *testing.T) {
Expand Down
Loading