feat!: updates the HashNode and HashLeaf methods to return error instead of panic and refactors the code #136

staheri14 · 2023-03-14T20:35:44Z

Overview

Closes #109 and #99.
This pull request ensures that the HashNode and HashLeaf functions return errors instead of panicking, and that caller functions properly handle any emitted errors. When generating proofs, any hashing errors are propagated with additional context. During verification, hashing errors are treated as unsuccessful proof verification.

Note that in this PR, the term Irrecoverable errors used to describe errors that any number of retries can correct the them.

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

codecov · 2023-03-15T19:20:26Z

Codecov Report

Merging #136 (e5f5e42) into master (c63e970) will decrease coverage by 0.14%.
The diff coverage is 96.99%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   96.33%   96.20%   -0.14%     
==========================================
  Files           6        6              
  Lines         464      527      +63     
==========================================
+ Hits          447      507      +60     
- Misses         11       12       +1     
- Partials        6        8       +2

Impacted Files	Coverage Δ
proof.go	`93.80% <90.69%> (-3.04%)`	⬇️
hasher.go	`97.45% <100.00%> (+1.16%)`	⬆️
nmt.go	`99.02% <100.00%> (+0.20%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

staheri14 · 2023-03-21T23:24:08Z

This PR is ready for review, with the exception of the code coverage percentage, which is currently 0.14% below the threshold. This 0.14% difference in the coverage is related to two lines (i.e., this and this) of the proof.go file, where the HashLeaf function emits an error. However, these situations should not occur in practice, as the input is already validated in earlier lines (e.g., via ValidateLeaf or by explicitly adding a namespace to the data prior to hashing it), making it impossible for any test scenario to hit those lines. Despite this, I have chosen not to silence the errors, as doing so could make the code vulnerable to unwanted bugs if someone updates the HashLeaf logic without updating the ValidateLeaf logic as well.
With the exception of these two lines, all other files are fully covered by an extensive set of tests.

rootulp

No blocking feedback, great work!

rootulp · 2023-03-22T08:07:27Z

hasher_test.go

+			ErrInvalidLeafLen,
+		},
+		{
+			"invalid node: left.max > right.imn",


Suggested change

"invalid node: left.max > right.imn",

"invalid node: left.max > right.min",

Fixed 7dfb628

rootulp · 2023-03-22T08:16:55Z

nmt_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := tt.tree.Root()


[question] should this invoke computeRoot? It currently seems identical to the above test:

func Test_Root_Error(t *testing.T) { // ... _, err := tt.tree.Root()

Thanks for finding this, please see the corrected version in this commit 3fd5dab

rootulp · 2023-03-22T08:19:04Z

proof.go

-			gotLeafHashes = append(gotLeafHashes, hashLeafFunc(leafData))
+			leafHash, err := hashLeafFunc(gotLeaf)
+			if err != nil { // this can never happen due to the initial validation of the leaf at the beginning of the loop
+				return false


[optional] if this can never happen, should it panic?

The reason it cannot happen is that the leaf is validated earlier in the code. However, if that section of the code changes in the future, we may encounter an error at this line. If the leaf is found to be invalid, we should return false instead of panicking.

If you check the line where the leaf gets validated, you will see that we return false. So, the same behaviour is replicated here.

rootulp · 2023-03-22T08:25:01Z

proof.go

-		hashes[i] = nth.HashLeaf(leafData)
+		res, err := nth.HashLeaf(leafData)
+		if err != nil {
+			return false // this never can happen since the leafData is guaranteed to be namespaced


[question] given this should never happen, should it panic?

The same argument as in this also applies in here.

rootulp · 2023-03-22T08:28:51Z

proof_test.go

+	// proof4 is the inclusion proof for the leaf at index 3
+	leaf4WONamespace := nmt.leaves[3][nmt.NamespaceSize():] // the VerifyInclusion function expects the leaf without the namespace ID


[question] what does the variable name here mean? It appears to truncate some leading bytes from the leaf so maybe leaf 4 without namespace? If so, proposal to rename

Suggested change

// proof4 is the inclusion proof for the leaf at index 3

leaf4WONamespace := nmt.leaves[3][nmt.NamespaceSize():] // the VerifyInclusion function expects the leaf without the namespace ID

// proof4 is the inclusion proof for the leaf at index 3

leaf4WithoutNamespace := nmt.leaves[3][nmt.NamespaceSize():] // the VerifyInclusion function expects the leaf without the namespace ID

[question] if VerifyInclusion expects the leaf without the namespace, why does this tampered test data fail?

[question] what does the variable name here mean? It appears to truncate some leading bytes from the leaf so maybe leaf 4 without namespace? If so, proposal to rename

In this variable name i.e., leaf4WONamespace, the WO part is the abbreviation of without, however, it looks like unclear (since you asked), so I'm going to change it to the full version.

[question] if VerifyInclusion expects the leaf without the namespace, why does this tampered test data fail?

The reason for the function's failure is not related to the structure of leaf4WithoutNamespace. The format of leaf4WithoutNamespace is correct, and in the line you referred to, I trimmed the namespace to ensure it is a valid input for the function. The error is caused by the corrupted proof.nodes section that follows that line. The node length is cut to nmt.NamespaceSize() bytes, resulting in the error.

[question] what does the variable name here mean? It appears to truncate some leading bytes from the leaf so maybe leaf 4 without namespace? If so, proposal to rename

Please see the new variable name in 3f1b523

proof.go

evan-forbes

only one question atm, but will continue to review

evan-forbes · 2023-03-22T10:52:38Z

hasher.go

+		if err != nil {
+			panic(err) // this should never happen since the data is already validated in the Write method
+		}


[question]
could this still be hit when verifying a proof since we're not calling the write method ahead of time?

If the Write method is not called prior to the Sum, then the code panics due to the absence of the tp field of the Hasher. More details follow:
For the Sum to get through, the tp field of the hasher should be set to one of the LeafPrefix or NodePrefix based on which the hasher determines whether the input data belongs to a leaf or a non-leaf node. Without making call to the Write method, the Hasher.tp field remains unset, hence in the mentioned switch statement, none of the LeafPrefix and NodePrefix cases are true, and the code goes straight to the default part which panics due to unset tp field.

Another point is that, it is not possible to call Sum without making a call to the Write method, as it corresponds to doing nothing (the Hasher state is empty). Doing so always results in panic due to undefined state

nmt/hasher.go

Line 121 in 85ec3c9

default:

.

evan-forbes

LGTM!

staheri14 · 2023-03-23T17:37:27Z

benchstat results (Update: this is no longer needed as the gobencher has been fixed now):

goos: darwin
goarch: arm64
pkg: github.com/celestiaorg/nmt
                          │ master.txt  │        nmt-err-handling.txt        │
                          │   sec/op    │   sec/op     vs base               │
ComputeRoot/64-leaves-10    38.30µ ± 2%   38.93µ ± 4%       ~ (p=0.089 n=10)
ComputeRoot/128-leaves-10   74.73µ ± 1%   75.29µ ± 1%  +0.75% (p=0.023 n=10)
ComputeRoot/256-leaves-10   152.1µ ± 3%   152.6µ ± 1%       ~ (p=0.579 n=10)
geomean                     75.79µ        76.48µ       +0.91%

staheri14 added 16 commits March 8, 2023 13:11

returns error from HashNode and HashLeaf

c3fd4a0

removes merkletree dependency

b72b47e

updates go.sum

7b2c42b

removes the mention to the merkletree repo from the readme file

f9f65fa

Merge branch 'removes-merkletree-dep' into err-handling

bff2b29

captures errors in the Write and Sum methods

3204f9b

returns error from HashLeaf

99ab77b

revises godocs to remove panic behavior

7ddfc8c

renames IsNamespaced to ValidateLeaf

f803988

replaces empty return result with nil

fef0f2c

returns error from computeRoot

2d25f58

wip

a0b146a

consumes and handles error in the verification related functions

1e66c2e

handles error emitted by HashNode and HashLeaf

0fee4de

refactors tests to handle errors

efd2636

adds some comments about when errors cannot happen

fbd7df6

staheri14 changed the title ~~feat!: updates the HashNode and HashLeaf method to return error instead of panic~~ feat!: updates the HashNode and HashLeaf methods to return error instead of panic Mar 14, 2023

staheri14 added 9 commits March 14, 2023 13:39

Merge branch 'master' into nmt-err-handling

f203df5

imports require package

ea7e976

makes Root return error instead of panic

c75f656

returns error from the Root methods and min and max calculators

6cb4c31

resolves golangi-lint issues

ed18029

Merge branch 'master' into nmt-err-handling

9c26689

adds more documentations and edits an error identifier

53eaa6d

Merge branch 'master' into nmt-err-handling

d25cfc6

fixes the HashNode signature

dbb0808

staheri14 added 3 commits March 16, 2023 10:26

clarifies irrecoverable errors vs recoverable ones

7927a17

corrects and clarifies some of the errors

26f748a

adds a missing validation criteria to the ProveRange

9575976

staheri14 added 5 commits March 20, 2023 12:28

adds description to the completeness error type

0cc7307

expands on a test scenario

ab5b0d9

adds another test vector for the Write methodd

083153d

fixes a mismatch bw the method name and the desc

bcc1e3d

refines the documentation

85ec3c9

staheri14 marked this pull request as ready for review March 21, 2023 17:21

staheri14 added 8 commits March 21, 2023 10:24

a minor change

ce18f2d

adds TestVerifyInclusion_False

e5aacdc

removes return types identifiers

2eb7ca8

implements TestNamespaceHasherSun_Err

ed3fea7

renames some of the tests

1e34574

removes an excess line

5d80fe4

makes some renaming

62dbe31

implements TestValidateLeafWithHash

e6741b9

staheri14 requested review from liamsi, evan-forbes and rootulp March 21, 2023 23:26

rootulp approved these changes Mar 22, 2023

View reviewed changes

evan-forbes reviewed Mar 22, 2023

View reviewed changes

staheri14 added 4 commits March 22, 2023 08:34

fixes a typo

7dfb628

correct the implementation of Test_computeRoot_Error

3fd5dab

renames leaf4WONamespace to leaf4WithoutNamespace

3f1b523

clarifies a variable name in the test

5f2829e

evan-forbes approved these changes Mar 23, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into nmt-err-handling

e5f5e42

staheri14 merged commit b34f794 into master Mar 23, 2023

staheri14 deleted the nmt-err-handling branch March 23, 2023 17:39

staheri14 mentioned this pull request Mar 23, 2023

fix: Resolving Inconsistency in the ProveNamespace Signature for Error Handling #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: updates the HashNode and HashLeaf methods to return error instead of panic and refactors the code #136

feat!: updates the HashNode and HashLeaf methods to return error instead of panic and refactors the code #136

staheri14 commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

staheri14 commented Mar 21, 2023 •

edited

Loading

rootulp left a comment

rootulp Mar 22, 2023

staheri14 Mar 22, 2023

rootulp Mar 22, 2023

staheri14 Mar 22, 2023

rootulp Mar 22, 2023

staheri14 Mar 22, 2023

staheri14 Mar 22, 2023 •

edited

Loading

rootulp Mar 22, 2023

staheri14 Mar 22, 2023

rootulp Mar 22, 2023

staheri14 Mar 22, 2023

staheri14 Mar 22, 2023 •

edited

Loading

staheri14 Mar 22, 2023

evan-forbes left a comment

evan-forbes Mar 22, 2023

staheri14 Mar 22, 2023

staheri14 Mar 22, 2023

evan-forbes left a comment

staheri14 commented Mar 23, 2023 •

edited

Loading

	"invalid node: left.max > right.imn",
	"invalid node: left.max > right.min",

		// proof4 is the inclusion proof for the leaf at index 3
		leaf4WONamespace := nmt.leaves[3][nmt.NamespaceSize():] // the VerifyInclusion function expects the leaf without the namespace ID

feat!: updates the HashNode and HashLeaf methods to return error instead of panic and refactors the code #136

feat!: updates the HashNode and HashLeaf methods to return error instead of panic and refactors the code #136

Conversation

staheri14 commented Mar 14, 2023 • edited Loading

Overview

Checklist

codecov bot commented Mar 15, 2023 • edited Loading

Codecov Report

staheri14 commented Mar 21, 2023 • edited Loading

rootulp left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Mar 22, 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

Choose a reason for hiding this comment

staheri14 Mar 22, 2023 • edited Loading

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

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

staheri14 commented Mar 23, 2023 • edited Loading

staheri14 commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

staheri14 commented Mar 21, 2023 •

edited

Loading

staheri14 Mar 22, 2023 •

edited

Loading

staheri14 Mar 22, 2023 •

edited

Loading

staheri14 commented Mar 23, 2023 •

edited

Loading