feat: verify the range of the namespace IDs of the children in the HashNode #102

staheri14 · 2023-02-16T18:38:29Z

Overview

This PR closes #95.
Inline with celestiaorg/celestia-app#1296.

The implementation of this pull request does not exactly follow the original suggestion in the LazyLedge paper. Specifically, it does not treat leftMaxNs = rightMinNs as a violation of leaf ordering. The reason for this is that a namespace may span across multiple subtrees.
cc: @liamsi

Quoting from the article:

An adversarial consensus node may attempt to produce a block that contains a Merkle tree with children that are not ordered correctly. To prevent this, we can set a condition in nsHash such that there is no valid hash when leftMaxNs ≥ rightMinNs, and thus there would be no valid Merkle root for incorrectly ordered children

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

…shNode

codecov · 2023-02-16T19:02:50Z

Codecov Report

Merging #102 (e81c6ca) into master (1bc0bb0) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   96.21%   96.27%   +0.05%     
==========================================
  Files           6        6              
  Lines         423      429       +6     
==========================================
+ Hits          407      413       +6     
  Misses         10       10              
  Partials        6        6

Impacted Files	Coverage Δ
hasher.go	`96.05% <100.00%> (+0.33%)`	⬆️

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

staheri14 · 2023-02-16T19:04:38Z

hasher_test.go

-			children{[]byte{0, 0, 1, 1}, []byte{0, 0, 0, 1}},
-			append(
-				[]byte{0, 0, 1, 1},
-				sum(crypto.SHA256, []byte{NodePrefix}, []byte{0, 0, 1, 1}, []byte{0, 0, 0, 1})...,


This test case is deleted as this represents an invalid state given the newly added logic i.e., the max namespace of the right child cannot be greater than the min namespace of the right child.

evan-forbes

one question, otherwise LGTM!

evan-forbes · 2023-02-17T20:45:27Z

hasher.go

+	rightMinNID := namespace.ID(rightMinNs)
+	leftMaxNID := namespace.ID(leftMaxNs)
+	if rightMinNID.Less(leftMaxNID) {
+		panic("nodes are out of order: the maximum namespace of the left child is greater than the min namespace of the right child")


would panicking here mean that someone could send a light node an invalid proof or sample and their node would crash?

As you mentioned, the HashNode method may only fail in the event of an attack on the system. However, it is important to note that the method was previously susceptible to panic, even prior to this pull request. For example, panic can occur while extracting the namespace identifier of the left or right children if they are not namespaced (i.e., if their length is less than NamespaceLen). It is not possible to return an error from this method without violating the interfaces it is intended to fulfill, namely NodeHasher and hash.
At this time, it may be better to allow the light client to crash rather than enter an invalid state.

I will investigate the implementation of proper error handling in the HashNode method, including handling other potential errors beyond just the namespace ID range, in a follow-up pull request. As this has a broad scope, it requires careful attention and consideration during implementation.
Tracking issue #109

Hmm, I think when this was implemented, we might have panic'ed bc it was supposed to be impossible to construct an invalid tree. But it is good thinking that someone might do so to crash light nodes and I do think we should error instead.

edit: I see. The Hash interface was another reason. Either we make it impossible client side that this panics (e.g. by doing some verification before calling this; then this should be very explicitly documented) or we have to break with the iface.

For the sake of getting this PR merged: can we add this property/panic to the godoc? So the caller has a hint that verification has to happen before to not cause unexpected panics?

For the sake of getting this PR merged: can we add this property/panic to the godoc? So the caller has a hint that verification has to happen before to not cause unexpected panics?

Sure! please check the following commit e7415b7

liamsi

Specifically, it does not treat leftMaxNs = rightMinNs as a violation of leaf ordering. The reason for this is that a namespace may span across multiple subtrees.

I see. That makes sense. Maybe @musalbas wants to update that case in the paper too.

…in-hashnpde

evan-forbes

👍 👍

we might also want to handle #109 before we cut a new release to avoid any unintentional panics

staheri14 · 2023-02-21T21:46:18Z

👍 👍

we might also want to handle #109 before we cut a new release to avoid any unintentional panics

Sure @evan-forbes, I am on it!

staheri14 · 2023-02-21T22:13:56Z

@liamsi I'd appreciate if you could please take another look and let me know if further updates are required on the PR before merging. Thanks!

liamsi

🙏🏼

staheri14 added 8 commits February 16, 2023 08:32

examines the namespace of children in the HashNode

622cc16

adds unit test for the correctness of namespace range check in the Ha…

ed08c84

…shNode

removes spaces from the test titles

27d3763

adds two intermediate variables to hold the left and right children

20a2f53

picks more clear variable names

72a99b4

fixes var name capitalization

19d349f

removes an invalid testcase

7caa9c8

renames panic to wantPanic for consistency with other tests

0cb2c49

staheri14 commented Feb 16, 2023

View reviewed changes

staheri14 marked this pull request as ready for review February 16, 2023 19:37

staheri14 self-assigned this Feb 16, 2023

staheri14 requested review from liamsi and evan-forbes February 16, 2023 21:00

Merge branch 'master' into verify-nodes-order-in-hashnpde

64ee062

staheri14 added the enhancement New feature or request label Feb 17, 2023

evan-forbes reviewed Feb 17, 2023

View reviewed changes

staheri14 mentioned this pull request Feb 18, 2023

feat: Handling Panics in HashNode Method #109

Closed

staheri14 requested a review from evan-forbes February 18, 2023 01:33

liamsi reviewed Feb 20, 2023

View reviewed changes

staheri14 added 3 commits February 21, 2023 12:50

explains when HashNode can panic

e7415b7

Merge remote-tracking branch 'origin/master' into verify-nodes-order-…

5866f52

…in-hashnpde

fixes formatting issue causing CI failure

887c86f

staheri14 requested a review from liamsi February 21, 2023 20:57

fixes gofumpt complain

e81c6ca

evan-forbes approved these changes Feb 21, 2023

View reviewed changes

liamsi approved these changes Feb 23, 2023

View reviewed changes

staheri14 merged commit 1d69de9 into master Feb 23, 2023

staheri14 deleted the verify-nodes-order-in-hashnpde branch February 23, 2023 15:59

staheri14 mentioned this pull request Mar 9, 2023

EPIC: Specify data root construction using rsmt2d, nmt, and the nmt wrapper celestiaorg/celestia-app#1296

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: verify the range of the namespace IDs of the children in the HashNode #102

feat: verify the range of the namespace IDs of the children in the HashNode #102

staheri14 commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

staheri14 Feb 16, 2023 •

edited

Loading

evan-forbes left a comment

evan-forbes Feb 17, 2023

staheri14 Feb 18, 2023 •

edited

Loading

liamsi Feb 20, 2023 •

edited

Loading

liamsi Feb 20, 2023

staheri14 Feb 21, 2023

liamsi left a comment

evan-forbes left a comment

staheri14 commented Feb 21, 2023

staheri14 commented Feb 21, 2023

liamsi left a comment

feat: verify the range of the namespace IDs of the children in the HashNode #102

feat: verify the range of the namespace IDs of the children in the HashNode #102

Conversation

staheri14 commented Feb 16, 2023 • edited Loading

Overview

Checklist

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

staheri14 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

evan-forbes Feb 17, 2023

Choose a reason for hiding this comment

staheri14 Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

liamsi Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

liamsi Feb 20, 2023

Choose a reason for hiding this comment

staheri14 Feb 21, 2023

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

staheri14 commented Feb 21, 2023

staheri14 commented Feb 21, 2023

liamsi left a comment

Choose a reason for hiding this comment

staheri14 commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

staheri14 Feb 16, 2023 •

edited

Loading

staheri14 Feb 18, 2023 •

edited

Loading

liamsi Feb 20, 2023 •

edited

Loading