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

Wrong hasher for of DefaultBinaryTree #1606

Open
vmx opened this issue May 30, 2022 · 2 comments
Open

Wrong hasher for of DefaultBinaryTree #1606

vmx opened this issue May 30, 2022 · 2 comments
Assignees

Comments

@vmx
Copy link
Contributor

vmx commented May 30, 2022

Description

The DefaultBinaryTree is defined as BinaryMerkleTree<DefaultTreeHasher>, while DefaultTreeHasher = PoseidonHasher:

pub type DefaultBinaryTree = BinaryMerkleTree<DefaultTreeHasher>;

Though the DefaultBinaryTree is also used at code paths which use SHA2 hashing, e.g.:

let base_tree_size = get_base_tree_size::<DefaultBinaryTree>(porep_config.sector_size)?;
.

The current code works, because the code paths the DefaultBinaryTree is used on, it only cares about the arity and the size of the hashes, which is the same for SHA2 and Poseidon. Though for correctness and clarity, the correct hasher for DefaultBinaryTree should be used.

Acceptance criteria

The correct hashers are uses for the corresponding trees.

Risks + pitfalls

Sometimes the Poseidon hasher might be needed. It needs thorough review whether changing the DefaultBinaryTree to SHA2 is enough, or whether some new type needs to be introduced.

Where to begin

Look at all occurrences of DefaultBinaryTree and check if SHA2 or Poseidon hashing should be used.

@vmx
Copy link
Contributor Author

vmx commented May 30, 2022

I've assigned this issue to @storojs72 as you are familiar with the merkle trees and it's a good way to dig deeper into how they are used.

@storojs72
Copy link
Contributor

Yes, I agree. We indeed have some confusing places in the codebase (where it is not obvious whether Poseidon or Sha256 hasher should be really used) that require clarification

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