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

Document that sorted leaves are stored backwards #26

Open
vsergeev opened this issue Sep 15, 2023 · 7 comments
Open

Document that sorted leaves are stored backwards #26

vsergeev opened this issue Sep 15, 2023 · 7 comments

Comments

@vsergeev
Copy link

I was wondering if there was a technical reason why the sorted leaves are stored backwards in the merkle tree. With the current construction, the lowest leaf index corresponds to the largest leaf value, which seems counterintuitive when attempting to reproduce the tree.

@frangio
Copy link
Contributor

frangio commented Oct 6, 2023

I don't think there is a technical reason. Other than it being counterintuitive I assume it hasn't caused any issues? Where would you expect this to be documented?

when attempting to reproduce the tree

Can you share more? Where were you trying to reproduce it? In what programming language?

@frangio frangio changed the title Sorted leaves are stored backwards Document that sorted leaves are stored backwards Oct 6, 2023
@vsergeev
Copy link
Author

vsergeev commented Oct 7, 2023

I'm also using TypeScript. I had implemented a miniature version (~100 lines) of the merkle tree generation compatible with OZ MerkleProof.sol, partly to understand exactly how this library was building the tree and partly to provide a simplified version for our SDK. I ran into some issues validating against this library, until I realized the leaves were inserted in reverse. I figured I'd raise the issue in case it helps others trying to verify the results of this library in another language or using another merkle tree library.

Right now, I'm at the crossroads of deciding whether we want our SDK to build the tree in the natural order, or in the reversed order just to be compatible with this library.

@frangio
Copy link
Contributor

frangio commented Oct 7, 2023

to provide a simplified version for our SDK

This library is pretty simple and minimal, is there something in particular that you would like to see simplified?

@vsergeev
Copy link
Author

vsergeev commented Oct 7, 2023

I think the library is great, especially if you need to access additional details of the tree, and even if you don't, it's simple enough to wrap it for a more basic API. The internal hash lookup for leaves is efficient for generating many proofs. It's also the obvious choice if you've already settled on using the StandardMerkleTreeData format.

We elected not to use the StandardMerkleTreeData format to keep the metadata size down and instead reproduce the tree in runtime. We have to verify all of the leaves and root anyway, as they can be created offchain by anyone in our use case. I could see why someone would use it though, especially when working with large trees or with generic leaves.

I do regret that many projects are probably going to inherit the implicitly reversed order here for their onchain merkle trees, but there isn't any fundamental technical issue with that.

@frangio
Copy link
Contributor

frangio commented Oct 7, 2023

If you want to reproduce the tree in runtime using this library you could do that by just shipping the leaf values and leaf encoding and loading this with StandardMerkleTree.of instead of load.

values: {
value: T;
treeIndex: number;
}[];
leafEncoding: string[];

I don't know if I understand the real problem with the "reversed" ordering. It seems to me that this order has no implication almost ever except if you use multiproofs (and well, if you want to reproduce the tree from the leaves without using the library).

@vsergeev
Copy link
Author

vsergeev commented Oct 7, 2023

It's just a semantic discrepancy that makes it a bit more difficult to reproduce the merkle tree with another implementation. It's actually more nuanced than I've described: while the leaves are stored sorted backwards, the node hash pairs are still sorted forwards, so in a sense the sorting in the library isn't consistent with itself. The readme comment "The leaves are sorted." isn't entirely accurate -- "reverse sorted" would be more precise. It wouldn't hurt to mention that the pairs are sorted too, as that's just as important.

@frangio
Copy link
Contributor

frangio commented Oct 8, 2023

I recall now that there was a technical reason why we did this, though I don't remember the exact details. When you construct the arguments for a multiproof in a smart contract you will need to sort them and provide them sorted. I believe when you do that you will need to provide them in "forward" order if the tree contains the leaves "backwards". The current design prioritizes making that aspect the more natural one.

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