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.

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

feat: Handling Panics in HashNode Method #109

Closed
staheri14 opened this issue Feb 18, 2023 · 1 comment · Fixed by #113 or #136
Closed

feat: Handling Panics in HashNode Method #109

staheri14 opened this issue Feb 18, 2023 · 1 comment · Fixed by #113 or #136
Assignees

Comments

@staheri14
Copy link
Contributor

staheri14 commented Feb 18, 2023

Problem

Identified as part of this EPIC: celestiaorg/celestia-app#1296.
The HashNode method of the NMT may potentially cause a panic if it receives invalid or malformed inputs (as mentioned by @evan-forbes in this comment #102 (comment)). For example, if the left or right children do not conform to the namespace hash format, meaning their length is less than the namespace ID size, or if the relation between the left and right children namespace range is invalid. If an invalid node is sent to a light node, it can result in a crash.

It is important to note that returning an error from this method is not possible as it would violate the interface it is intended to fulfill i.e., NodeHasher and Hash.

Acceptance Criteria

This issue is to implement proper error handling mechanism for HashNode.

@staheri14 staheri14 self-assigned this Feb 18, 2023
@staheri14 staheri14 changed the title Handling Panics in HashNode Method feat: Handling Panics in HashNode Method Feb 18, 2023
staheri14 added a commit that referenced this issue Feb 28, 2023
… to predict panics (#113)

## Overview
An incremental PR toward #109 
**Next**: The validation utility functions developed in this PR must be
further integrated into both the tree construction and proof
verification process to detect invalid inputs and notify the caller of
any errors. This will close #109 and is in line with the objectives
outlined in issues #99 and #97. I will address this in a subsequent pull
request.
## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
@staheri14 staheri14 reopened this Mar 1, 2023
@staheri14
Copy link
Contributor Author

Reopening this issue as it is not completely resolved.
As mentioned in the last relevant PR i.e., #113, to avoid situations in which light clients may panic, we need to integrate the node validity checks into the proof verification and construction and return error for invalid inputs. Also, as per conversation with @evan-forbes, we also decided to change the signature of the HashNode and HashLeaf to return errors for invalid inputs instead of panic. All these require some refactoring. Once done, we can close this issue.

staheri14 added a commit that referenced this issue Mar 23, 2023
…ead of panic and refactors the code (#136)

## 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


- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment