feat: extends error handling to detect byzantine data and unordered shares in tree construction #242

staheri14 · 2023-07-07T18:22:58Z

Overview

Closes #191

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

…arity shares

…axis fails

…ng the error

…lation-failures

…rdered-shares-mocked-wrapper

rootulp

No blocking feedback. Question about the copied file nmtwrapper.go: is it only needed for tests? If yes, is it possible to use the _test.go suffix?

extendeddatacrossword_test.go

rootulp · 2023-07-07T20:19:36Z

extendeddatacrossword_test.go

+		values        [][]byte
+		wantErr       bool
+		corruptedAxis Axis
+		corruptedInd  uint


[nit] it wasn't immediately obvious what Ind meant but it appears short for index. This suggestion can't be applied directly b/c it implies renaming the variable in this scope

Suggested change

corruptedInd uint

corruptedIndex uint

This suggestion can't be applied directly b/c it implies renaming the variable in this scope

Can you please expand on this second part of your comment? Nevertheless, I applied the first part of your feedback in 2dcfb41

rootulp · 2023-07-07T20:21:23Z

extendeddatacrossword_test.go

+		},
+	}
+
+	for codecName, codec := range codecs {


[optional] there's only one codec so we can remove a layer of nesting from this test case

See: #216

Thanks for mentioning that! I totally agree with eliminating that nested loop. But to keep our PRs more focused and organized, I'd suggest saving this change for a separate PR. Especially since we will have an upcoming PR specifically targeting that issue.

extendeddatacrossword_test.go

nmtwrapper.go

evan-forbes

LGTM 👍

evan-forbes · 2023-07-08T02:18:55Z

nmtwrapper.go

@@ -0,0 +1,142 @@
+package rsmt2d
+
+// The contents of this file have been adapted from the source file available at https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go,


the only thing the wrapper imports is the consts, so maybe it makes sense to accept those consts as args, then move the entire wrapper here

Thanks for the suggestion, here is the tracking issue #248.

That also makes sense to me, cannot see any immediate issue for doing so, however, since it requires some refactoring in the code, I'd say it merits a separate PR.

Wondertan · 2023-07-08T09:46:43Z

nmtwrapper.go

@@ -0,0 +1,142 @@
+package rsmt2d
+
+// The contents of this file have been adapted from the source file available at https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go,


Why copy and why not have these tests next to the wrapper?

Or why not move the wrapper here completely then?

Or why not move the wrapper here completely then?

That also makes sense to me, cannot see any immediate issue for doing so, however, since it requires some refactoring in the code, I'd say it merits a separate PR. Please see the tracking issue #248.

staheri14 · 2023-07-10T20:38:42Z

No blocking feedback. Question about the copied file nmtwrapper.go: is it only needed for tests? If yes, is it possible to use the _test.go suffix

Indeed, it serves solely for testing purposes, and I have implemented your suggestion by adding the _test suffix. Please see 799ee73.

rootulp

Still LGTM

nmtwrapper_test.go

staheri14 added 30 commits June 29, 2023 13:52

todos

72b640b

Merge remote-tracking branch 'origin/master' into BEFP-for-nmt-failures

d5bc116

returns ByzantineErrData on root calculation failure or mismatching p…

48296aa

…arity shares

adds godoc for verifyAgainstColRoots and renames oldShares to shares

0e8ca50

return ErrByzantineData when the root verification of the orthogonal …

cc82d3f

…axis fails

cleans up the docs and comments

6b77271

fixes a typo

9271b33

fixes a wrong index, and updates the orthogonal axis prior to returni…

9ed797d

…ng the error

removes redundant copy of rebuilt shares

ec15029

reverts setting the missing share

6caeb28

Merge remote-tracking branch 'origin/master' into BEFP-for-root-calcu…

45d651a

…lation-failures

Merge remote-tracking branch 'origin/master' into BEFP-for-root-calcu…

e7380b0

…lation-failures

use the already copied shares

12a449c

Merge branch 'master' into BEFP-for-root-calculation-failures

5c0cbbd

initial tests

72c5510

resolves nmt wrapper dependency

113f1c3

uses NMT for the unordered shares test

b75d1f2

adds the wrapper package with implementation

a523a65

adds some incomplete tests

5cf9a86

updates the test to consume the wrapper implementation

9546941

refactors the wrapper to access to a custom namespace size

ace3ea5

sets proper value for parity share namespace

c0f00d8

adds shareValues as argument to the createTestEdsWithNMT

291cae6

adds tests for unordered shares

8ac7b14

Merge branch 'main' into BEFP-for-root-calculation-failures

44ff983

Merge branch 'BEFP-for-root-calculation-failures' into sanaz/BEFP-uno…

ab4b0b4

…rdered-shares-mocked-wrapper

adds further criteria to the tests

7d464eb

includes test description

d3101f5

makes nmtwrapper types and functions un-importable

d4a564b

adds godoc for newDataSquare

145ec9f

staheri14 added 2 commits July 7, 2023 13:04

removes error type as all tests have identical error type

976934a

fixes a bug

a525e49

staheri14 requested review from evan-forbes and rootulp July 7, 2023 20:11

rootulp previously approved these changes Jul 7, 2023

View reviewed changes

evan-forbes previously approved these changes Jul 8, 2023

View reviewed changes

Wondertan approved these changes Jul 8, 2023

View reviewed changes

addresses reviewers feedback

2dcfb41

staheri14 dismissed stale reviews from evan-forbes and rootulp via 2dcfb41 July 10, 2023 20:13

staheri14 added 3 commits July 10, 2023 13:14

Merge branch 'main' into sanaz/BEFP-unordered-shares-mocked-wrapper

7e4b44e

fixes a variable name

1a4e055

replaces reference to the nmtwrapper with a permalink

a364d79

staheri14 mentioned this pull request Jul 10, 2023

Moving nmt wrapper to the rsmt2d #248

Open

adds _test suffix to the nmtwrapper file

799ee73

staheri14 requested review from rootulp and evan-forbes July 10, 2023 20:40

rootulp previously approved these changes Jul 11, 2023

View reviewed changes

nmtwrapper_test.go Outdated Show resolved Hide resolved

removes line number from the permalink

1e7b68f

staheri14 dismissed rootulp’s stale review via 1e7b68f July 11, 2023 17:12

staheri14 requested a review from rootulp July 11, 2023 17:12

rootulp approved these changes Jul 11, 2023

View reviewed changes

staheri14 requested review from cmwaters, rach-id and adlerjohn July 11, 2023 18:11

evan-forbes approved these changes Jul 11, 2023

View reviewed changes

staheri14 changed the title ~~feat: handles unordered shares~~ feat: extends error handling to detect byzantine data and unordered shares in tree construction Jul 12, 2023

staheri14 merged commit b97517c into main Jul 12, 2023
5 checks passed

staheri14 deleted the sanaz/BEFP-unordered-shares-mocked-wrapper branch July 12, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extends error handling to detect byzantine data and unordered shares in tree construction #242

feat: extends error handling to detect byzantine data and unordered shares in tree construction #242

staheri14 commented Jul 7, 2023 •

edited

Loading

rootulp left a comment

rootulp Jul 7, 2023

staheri14 Jul 10, 2023 •

edited

Loading

rootulp Jul 7, 2023

staheri14 Jul 10, 2023

evan-forbes left a comment

evan-forbes Jul 8, 2023

staheri14 Jul 10, 2023

staheri14 Jul 10, 2023 •

edited

Loading

Wondertan Jul 8, 2023

Wondertan Jul 8, 2023

staheri14 Jul 10, 2023

staheri14 commented Jul 10, 2023

rootulp left a comment

		@@ -0,0 +1,142 @@
		package rsmt2d

		// The contents of this file have been adapted from the source file available at https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go,

feat: extends error handling to detect byzantine data and unordered shares in tree construction #242

feat: extends error handling to detect byzantine data and unordered shares in tree construction #242

Conversation

staheri14 commented Jul 7, 2023 • edited Loading

Overview

Checklist

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 commented Jul 10, 2023

rootulp left a comment

Choose a reason for hiding this comment

staheri14 commented Jul 7, 2023 •

edited

Loading

staheri14 Jul 10, 2023 •

edited

Loading

staheri14 Jul 10, 2023 •

edited

Loading