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 'verifyWithIndex' function #2851

Closed
wants to merge 1 commit into from

Conversation

Arachnid
Copy link

@Arachnid Arachnid commented Sep 9, 2021

I was going to open an issue to discuss this, but figured it was just as quick to give an example PR for feedback.

It's trivial for the merkle proof verification function to compute and return an index when it verifies the proof. This is particularly useful for the common case of wanting to track when airdrops have been claimed using a bitmap; the returned index value can be used to specify the bit in the bitmap to set.

@Amxx
Copy link
Collaborator

Amxx commented Sep 9, 2021

Hello @Arachnid, and thank you for your PR

The changes proposed add a verifyWithIndex(bytes32[],bytes32,bytes32) returns (bool,uint256), which is nice, but renames the existing function. I think it is essential to keep the existing signature verify(bytes32[],bytes32,bytes32) returns (bool) for backward compatibility.

Note that there is already a PR affecting MerkleProof.sol #2841. If you are ok with it, I think we should merge the two.

@Amxx
Copy link
Collaborator

Amxx commented Sep 9, 2021

Also, I don't really think your indexes are the ones I would expect

  • The first leaf from the left is 0,
  • The second one is 2*(n-1) with n the depth of the tree

Edit: with all proof in the tree not having the same length, I'm not sure if we can do better then the indexes you propose :/

Edit2: let me know what you think of #2841

@Arachnid
Copy link
Author

#2841 looks like it's pretty much a superset of this, so I'm happy to close it in favor of that!

@Arachnid Arachnid closed this Sep 10, 2021
@frangio
Copy link
Contributor

frangio commented Oct 15, 2021

@Arachnid We were going to merge this feature in #2841 but rolled it back. We were uncomfortable with the fact that we either make verify revert when the index overflows (a breaking change), or else the index stops being unique.

Curious if you have any thoughts about these issues.

@Arachnid
Copy link
Author

Personally I would've had it revert on overflow, and document that it only supports trees that are at most 256 levels deep, and that indexes will be unique but not necessarily sequential. I think this is still a useful primitive under those contraints.

The proposed workaround of supplying the index as part of the leaf is quite inelegant, as it requires providing redundant information.

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.

None yet

3 participants