Add a MerkleTree builder

[Amxx]

Structure inspired by Tornardo Cash's commitment tree.

• Initialize a complete merkle tree of given depth, with all leaves having the same "zero" value.
• Insert leaves one at a time, recomputing the merkle root in the process
• Keep a "limited" (rotating) history of past merkle roots.

Fixes: #4758

PR Checklist

• Use Extend Checkpoints with new sizes and lookup mechanisms #3589 when merged
• Tests
• Documentation
• Changelog entry

[Amxx]

Just realized that if someone could be creative and commit merle roots of subtrees that use the same hashings function. That would allow verification of the leafs deep inside the subtrees

[changeset-bot]

🦋 Changeset detected

Latest commit: eca9085

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ernestognw]

Just realized this function is commutative. Should the tree work with non-commutative functions?

I guess it should, so I'd feel better if we add tests with a custom non-commutative keccak256 function. It may require updating the javascript library for testing.

If the tree expects only commutative functions we should be explicit about it.

[Amxx]

We should support non commutative hashes, so yes, we should test that.

[ernestognw]

Right, we should do this after the javascript library is released.

.

[ernestognw]

Took the freedom to update the documentation. I left some comments and I think this still needs some work but it's looking really good in my opinion.

[ernestognw]

I gave it a thought and I started to see why you wanted to call the function stdPairKeccak256 like that in the Hashes library considering the context of the Javascript library (assuming it's for standard). I'd like to discuss the naming again.

Right now I'm not convinced of sortedPairKeccak256 either. The reason is that I think Pair is redundant. I feel we should agree on a naming convention for future additions to the Hashes library, and the distinction between sorting a pair or a single value should come from the function signature.

For example, for a sortedKeccak256, it'd be:

function sortedKeccak256(bytes32 a, bytes32 b) internal pure returns (bytes32) {
    return a < b ? _efficientKeccak256(a, b) : _efficientKeccak256(b, a);
}

function sortedKeccak256(bytes32[] memory ptr) {
  ...
}

I'd say users would need to decide three things: the algorithm, the sorting (for multiproofs), and some particularity of the algorithm. In this case, it's just keccak256 with sorting, so it's fine if we call it sortedKeccak256 but I have a couple of questions:

• Would it make sense an inverse sorted hashing function at all? For example, we could have sortedKeccak256(bytes32 a, bytes32 b, bool asc)
• Furthermore, would it make sense to have a function with signature keccak256(bytes32, bytes32, Sorting) assuming keccak256 is not creating a conflict? In this case, Sorting would be either None, Asc, or Desc.

So far I'm feeling the best naming convention we can use would be using the name of the algorithm and that's it, but then I'd reconsider calling it stdHash to avoid the naming conflict.

I know the naming convention and variants would make it difficult for a user to decide what to use but we can wrap the common use cases:

function stdHash(bytes32 a, bytes32 b) {} // Default to sorted for multiproofs
function stdHash(bytes32 a, bytes32 b, Sorting sorting) {}
function poseidon(bytes32 a) {}
function poseidon(bytes32 a, bytes32 b) {} // Default to sorted for multiproofs
functon posiedon(bytes32 a, bytes32 b, Sorting sorting) {}

The main issue I see with this is the gas overhead of each hashing operation since it'd be evaluating the sorting each time, perhaps unnecessarily, but I think this is cleaner.

Sorry for bringing it up again, just want to make sure we release an API that behaves well over time, he. What do you think @Amxx?

[ernestognw]

Almost there!

[ernestognw]

Right, we should do this after the javascript library is released.

[ernestognw]

LGTM, pending to discuss whether to keep the root stored or not