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

code hygiene: idiomatic error naming #24

Merged
merged 2 commits into from
Apr 3, 2021
Merged

code hygiene: idiomatic error naming #24

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
310b72b
rename errors, remove unused EDS in errors
liamsi Apr 2, 2021
6610ba4
rename another error and use errors.As instead of casting
liamsi Apr 2, 2021
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
49 changes: 20 additions & 29 deletions extendeddatacrossword.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package rsmt2d

import (
"bytes"
"errors"
"fmt"

"gonum.org/v1/gonum/mat"
Expand All @@ -12,34 +13,27 @@ const (
column = 1
)

// ByzantineRowError is thrown when there is a repaired row does not match the expected row merkle root.
type ByzantineRowError struct {
RowNumber uint
LastGoodSquare ExtendedDataSquare
// ErrUnrepairableDataSquare is thrown when there is insufficient chunks to repair the square.
var ErrUnrepairableDataSquare = errors.New("failed to solve data square")

// ErrByzantineRow is thrown when there is a repaired row does not match the expected row merkle root.
type ErrByzantineRow struct {
RowNumber uint
}

func (e *ByzantineRowError) Error() string {
func (e *ErrByzantineRow) Error() string {
return fmt.Sprintf("byzantine row: %d", e.RowNumber)
}

// ByzantineColumnError is thrown when there is a repaired column does not match the expected column merkle root.
type ByzantineColumnError struct {
ColumnNumber uint
LastGoodSquare ExtendedDataSquare
Copy link
Member Author

@liamsi liamsi Apr 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is it really OK to remove this LastGoodSquare? It's currently not really used but I'm not sure if this will be needed to proof that this column was actually byzantine? (cc @adlerjohn) (same for rows below)

In other words, will this be useful to construct these: https://github.com/lazyledger/lazyledger-specs/blob/master/specs/data_structures.md#badencodingfraudproof ?

Copy link
Member

@adlerjohn adlerjohn Apr 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really see the need for returning a backup copy of the entire square?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's what I thought too. Thanks for confirming. The data is in the hands of the caller anyways. If required, the caller can construct everything from the row/col index and the data again (not sure if they need to in any case though).

It would help if the spec actually explained under what circumstances nodes and (which nodes?) would construct a BadEncodingProof from what data. Should I open an issue about that?

Copy link
Member

@adlerjohn adlerjohn Apr 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should I open an issue about that?

Maybe a comment in celestiaorg/celestia-specs#23 that explains what specifically you want to see? I'm going to start working on Evidence relatively soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

celestiaorg/celestia-specs#23 (comment)
Is that clear enough?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks.

// ErrByzantineColumn is thrown when there is a repaired column does not match the expected column merkle root.
type ErrByzantineColumn struct {
ColumnNumber uint
}

func (e *ByzantineColumnError) Error() string {
func (e *ErrByzantineColumn) Error() string {
return fmt.Sprintf("byzantine column: %d", e.ColumnNumber)
}

// UnrepairableDataSquareError is thrown when there is insufficient chunks to repair the square.
type UnrepairableDataSquareError struct {
}

func (e *UnrepairableDataSquareError) Error() string {
return "failed to solve data square"
}

// RepairExtendedDataSquare repairs an incomplete extended data square, against its expected row and column merkle roots.
// Missing data chunks should be represented as nil.
func RepairExtendedDataSquare(
Expand All @@ -63,7 +57,7 @@ func RepairExtendedDataSquare(
}

if chunkSize == 0 {
return nil, &UnrepairableDataSquareError{}
return nil, ErrUnrepairableDataSquare
}

fillerChunk := bytes.Repeat([]byte{0}, chunkSize)
Expand Down Expand Up @@ -136,9 +130,6 @@ func (eds *ExtendedDataSquare) solveCrossword(rowRoots [][]byte, columnRoots [][
if err == nil { // repair successful
progressMade = true

// Make backup of square
edsBackup, _ := eds.deepCopy()

// Insert rebuilt shares into square
for p, s := range rebuiltShares {
if mode == row {
Expand Down Expand Up @@ -170,11 +161,11 @@ func (eds *ExtendedDataSquare) solveCrossword(rowRoots [][]byte, columnRoots [][
// Check that rebuilt vector matches given merkle root
if mode == row {
if !bytes.Equal(eds.RowRoot(i), rowRoots[i]) {
return &ByzantineRowError{i, edsBackup}
return &ErrByzantineRow{i}
}
} else if mode == column {
if !bytes.Equal(eds.ColRoot(i), columnRoots[i]) {
return &ByzantineColumnError{i, edsBackup}
return &ErrByzantineColumn{i}
}
}

Expand All @@ -184,12 +175,12 @@ func (eds *ExtendedDataSquare) solveCrossword(rowRoots [][]byte, columnRoots [][
if mode == row {
adjMask := mask.ColView(int(j))
if vecNumTrue(adjMask) == adjMask.Len()-1 && !bytes.Equal(eds.ColRoot(j), columnRoots[j]) {
return &ByzantineColumnError{j, edsBackup}
return &ErrByzantineColumn{j}
}
} else if mode == column {
adjMask := mask.RowView(int(j))
if vecNumTrue(adjMask) == adjMask.Len()-1 && !bytes.Equal(eds.RowRoot(j), rowRoots[j]) {
return &ByzantineRowError{j, edsBackup}
return &ErrByzantineRow{j}
}
}
}
Expand All @@ -215,7 +206,7 @@ func (eds *ExtendedDataSquare) solveCrossword(rowRoots [][]byte, columnRoots [][
if solved {
break
} else if !progressMade {
return &UnrepairableDataSquareError{}
return ErrUnrepairableDataSquare
}
}

Expand Down Expand Up @@ -253,7 +244,7 @@ func (eds *ExtendedDataSquare) prerepairSanityCheck(rowRoots [][]byte, columnRoo
return err
}
if !bytes.Equal(flattenChunks(shares), flattenChunks(eds.rowSlice(i, eds.originalDataWidth, eds.originalDataWidth))) {
return &ByzantineRowError{i, *eds}
return &ErrByzantineRow{i}
}
}

Expand All @@ -263,7 +254,7 @@ func (eds *ExtendedDataSquare) prerepairSanityCheck(rowRoots [][]byte, columnRoo
return err
}
if !bytes.Equal(flattenChunks(shares), flattenChunks(eds.columnSlice(eds.originalDataWidth, i, eds.originalDataWidth))) {
return &ByzantineColumnError{i, *eds}
return &ErrByzantineColumn{i}
}
}
}
Expand Down
20 changes: 11 additions & 9 deletions extendeddatacrossword_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package rsmt2d

import (
"bytes"
"errors"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -62,15 +63,15 @@ func TestRepairExtendedDataSquare(t *testing.T) {
t.Errorf("did not return an error on trying to repair a square with bad roots")
}

var ok bool
corrupted, err = original.deepCopy()
if err != nil {
t.Fatalf("unexpected err while copying original data: %v, codec: :%v", err, codec)
}
corrupted.setCell(0, 0, corruptChunk)
_, err = RepairExtendedDataSquare(corrupted.RowRoots(), corrupted.ColumnRoots(), corrupted.flattened(), codec, NewDefaultTree)
if err, ok = err.(*ByzantineRowError); !ok {
t.Errorf("did not return a ByzantineRowError for a bad row; got: %v", err)
var byzRow *ErrByzantineRow
if !errors.As(err, &byzRow) {
t.Errorf("did not return a ErrByzantineRow for a bad row; got: %v", err)
}

corrupted, err = original.deepCopy()
Expand All @@ -79,8 +80,8 @@ func TestRepairExtendedDataSquare(t *testing.T) {
}
corrupted.setCell(0, 3, corruptChunk)
_, err = RepairExtendedDataSquare(corrupted.RowRoots(), corrupted.ColumnRoots(), corrupted.flattened(), codec, NewDefaultTree)
if err, ok = err.(*ByzantineRowError); !ok {
t.Errorf("did not return a ByzantineRowError for a bad row; got %v", err)
if !errors.As(err, &byzRow) {
t.Errorf("did not return a ErrByzantineRow for a bad row; got %v", err)
}

corrupted, err = original.deepCopy()
Expand All @@ -91,8 +92,9 @@ func TestRepairExtendedDataSquare(t *testing.T) {
flattened = corrupted.flattened()
flattened[1], flattened[2], flattened[3] = nil, nil, nil
_, err = RepairExtendedDataSquare(corrupted.RowRoots(), corrupted.ColumnRoots(), flattened, codec, NewDefaultTree)
if err, ok = err.(*ByzantineColumnError); !ok {
t.Errorf("did not return a ByzantineColumnError for a bad column; got %v", err)
var byzColumn *ErrByzantineColumn
if !errors.As(err, &byzColumn) {
t.Errorf("did not return a ErrByzantineColumn for a bad column; got %v", err)
}

corrupted, err = original.deepCopy()
Expand All @@ -103,8 +105,8 @@ func TestRepairExtendedDataSquare(t *testing.T) {
flattened = corrupted.flattened()
flattened[1], flattened[2], flattened[3] = nil, nil, nil
_, err = RepairExtendedDataSquare(corrupted.RowRoots(), corrupted.ColumnRoots(), flattened, codec, NewDefaultTree)
if err, ok = err.(*ByzantineColumnError); !ok {
t.Errorf("did not return a ByzantineColumnError for a bad column; got %v", err)
if !errors.As(err, &byzColumn) {
t.Errorf("did not return a ErrByzantineColumn for a bad column; got %v", err)
}
}
}