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

Add a 'sortLeaves' options #29

Merged
merged 18 commits into from
Jan 10, 2024
Merged

Add a 'sortLeaves' options #29

merged 18 commits into from
Jan 10, 2024

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Dec 14, 2023

By default, leaves are sorted. This is done so to simplify the multiproof process.

When building a multiproof, all the leaves being proven must be in the same order as they appear in the tree. This means that when verifying a multiproof, the contract will have to order the leaves in an order that matches their position in the tree. Best practice is to order the leaves of the tree such that the smart contract can replicate that order without knowledge of the tree, and thus verify the multiproofs.

However, there are also usecases where the tree is constructed iterativelly, from unsorted leaves. See:

In order to support that usecase, I'm proposing we add a option object to StandardMerkleTree.of. Leaves will be sorted by default to replicate existing behavior. Disabling the sorting addresses the need of the usecases describe above. Note that since these usecase use full/complete tree, the reversal part (done by the core) is irrelevant.

In ay b more difficult for contracts to verify multiproofs for trees constructed without leave sorting. This may not be a major drawback though. In any case, it is still possible to verify such multiproof, as long as the caller of the contract can force the ordering of the leaves that must be proven.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, otherwise good.

I didn't understand this comment:

Note that since these usecase use full/complete tree, the reversal part (done by the core) is irrelevant.

Tangentially related: #26

src/standard.ts Outdated Show resolved Hide resolved
src/standard.ts Outdated Show resolved Hide resolved
src/standard.ts Outdated Show resolved Hide resolved
@Amxx
Copy link
Contributor Author

Amxx commented Dec 15, 2023

If a tree has 2**n leaves, with n the depth of the tree, then all leaves are at the same level. In that case, reverting the order of leaves (or not) does not change the resulting root. That is because the hash used is symmetric.

That is a property of the tree we are building in the Solidity structure we plan to introduce.

The fact that the leaves are reversed only affect trees where all leaves are not at the same level. In that case, the reversal part affects which leaves are at which level. In that case reverting the leaves (or not) changes the overall tree.

Note that before merging we plan to update the README to document that feature, and its consequences.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Amxx and others added 4 commits January 5, 2024 16:03
README.md Outdated Show resolved Hide resolved
Co-authored-by: Hadrien Croubois <[email protected]>
@Amxx Amxx merged commit e15b2d3 into master Jan 10, 2024
3 checks passed
@Amxx Amxx deleted the options branch January 10, 2024 10:44
@Amxx Amxx restored the options branch January 10, 2024 10:44
@frangio frangio deleted the options branch January 10, 2024 19:15
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

Successfully merging this pull request may close these issues.

3 participants