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

clarifying the merkletree package usage #125

Closed
staheri14 opened this issue Mar 8, 2023 · 5 comments
Closed

clarifying the merkletree package usage #125

staheri14 opened this issue Mar 8, 2023 · 5 comments
Assignees

Comments

@staheri14
Copy link
Contributor

staheri14 commented Mar 8, 2023

Problem

Inline with #109 and identified as part of this EPIC: celestiaorg/celestia-app#1296.

The HashNode and HashLeaf functions in the nmt package aim to satisfy the NodeHasher and LeafHasherz interfaces from the merkletree package.
Therefore, they need to comply with the signatures dictated by those interfaces. As a result, they cannot return any errors upon invalid inputs, as explained in issue #109. However, it is not clear whether this compliance is necessary, as pointed out by @evan-forbes. Therefore, this issue is to investigate the usage of the merkletree package in the nmt package and in celestia-app, celestia-core, and celestia-node. If no usage is found, the package should be removed from the set of dependencies.

@staheri14
Copy link
Contributor Author

staheri14 commented Mar 8, 2023

Sharing my findings here for future reference:

NMT repo: The usage of the merkletree package in the nmt repository is limited to a single dependency. Specifically, the nmt.Hasher type must implement the merkletree.TreeHasher interface to create Merkle range proofs using the merkletree package. This requirement is only applicable in the following lines:

nmt/proof_test.go

Lines 177 to 178 in eec4bc0

subTreeHasher := newCachedSubtreeHasher(n.leafHashes, n.treeHasher)
incompleteRange, err := merkletree.BuildRangeProof(start, end, subTreeHasher)

However, the NamespaceMerkleTree struct provides an alternative method for generating range proofs called buildRangeProof. Therefore, it is safe to remove the dependency on the merkletree package from the nmt repository.
Furthermore, the other repos i.e., celestia-app, celestia-core and celestia-node were also searched to determine if the Hasher type had ever been used as a merkletree.TreeHasher. However, no usage was found.

The necessary changes for this removal are available in the following pull request: #126.

@staheri14
Copy link
Contributor Author

staheri14 commented Mar 9, 2023

Based on the following, it seems to be fine to archive the merkletree package.
As of celestiaorg/celestia-app@0c7806a no usage was found for the merkletree package in the celestia-app repo.
As of celestiaorg/celestia-core@a11b8cb no usage was found for the merkletree package in the celestia-core repo.
As of celestiaorg/celestia-node@b7213bf no usage was found for the merkletree package in the celestia-node repo.

Awaiting celestia-node team's response on Slack to ensure archiving the merkletree repo is ok.
cc: @evan-forbes @rootulp

staheri14 added a commit that referenced this issue Mar 9, 2023
## Overview

Inline with #125.
For more context, please see the following
[comment](#125 (comment)).

## 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
Copy link
Contributor Author

Based on the response from the Celestia-node team, we are good to archive the merkletree repo.

@rootulp
Copy link
Collaborator

rootulp commented Mar 10, 2023

Cool! https://github.com/celestiaorg/merkletree was archived yesterday

Screenshot 2023-03-10 at 9 20 50 AM

@staheri14
Copy link
Contributor Author

Cool! https://github.com/celestiaorg/merkletree was archived yesterday

Screenshot 2023-03-10 at 9 20 50 AM

@rootulp Great! so going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants