celestiaorg / nmt

Namespaced Merkle Tree
Apache License 2.0
117 stars 42 forks source link

clarifying the merkletree package usage #125

Closed staheri14 closed 1 year ago

staheri14 commented 1 year ago

Problem

Inline with #109 and identified as part of this EPIC: https://github.com/celestiaorg/celestia-app/issues/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 commented 1 year ago

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: https://github.com/celestiaorg/nmt/blob/eec4bc0cb01d1256c6882ede4920c4ebd70235ff/proof_test.go#L177-L178 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: https://github.com/celestiaorg/nmt/pull/126.

staheri14 commented 1 year ago

Based on the following, it seems to be fine to archive the merkletree package. As of https://github.com/celestiaorg/celestia-app/commit/0c7806a9ace4754870da53b059a15f092ee8b2cd no usage was found for the merkletree package in the celestia-app repo. As of https://github.com/celestiaorg/celestia-core/commit/a11b8cb04887687fd55d12be58cfe18e9ac2831f no usage was found for the merkletree package in the celestia-core repo. As of https://github.com/celestiaorg/celestia-node/commit/b7213bfa9e84098834bc869a465cfc4cbb85b7ba 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 commented 1 year ago

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

rootulp commented 1 year ago

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

Screenshot 2023-03-10 at 9 20 50 AM
staheri14 commented 1 year ago

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.