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

Fix potential collisions with merkle map indicies #1694

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Jun 21, 2024

Summary

This PR fixes an issue where computing the key of a MerkleMap can lead to a collision between two different field elements. This is because the key is computed with bit reversal, so the key 1 is bit reversed to be 10..000, i.e. 2^254. This can lead to potential collisions since the pasta field is smaller than 2^255 and multiple distinct indices may be associated to multiple keys.

Changes

  • Modified the MerkleMap._keyToIndex function to check the size of the key computed and throw an error if it's greater than 2^254
  • Modified the property testing unit test for MerkleMaps to expect failure if the input field is greater than 2^254
  • Added @example tags to the MerkleMap class

The change ensures that the key fits in 254 bits. This prevents potential collisions due to the Pasta field modulus being smaller than 2^255.
@MartinMinkov MartinMinkov self-assigned this Jun 24, 2024
@MartinMinkov MartinMinkov marked this pull request as ready for review June 25, 2024 01:34
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

This change will prevent honest users from using keys bigger than 2^254, but we also have to protect against attackers which could go in and modify o1js e.g. remove the assertion you added @MartinMinkov.

So we also have to put the restriction on key size into constraints. The provable part of Merkle maps is MerkleMapWitness with computeRootAndKey().

The easiest option to adapt those is probably to add a new computeRootAndKeyV2() (and deprecate the old version) which contains an additional assertion:

isLefts[0].assertTrue()

All the leaves are "left" nodes because we know the bit vector computed from the key starts with a zero.

To make it more obvious that this forces the computed key to be < 2^254, you could also hard-code what happens in the 0th iteration outside the loop (i.e. the key is still 0), and only start the loop at i=1.

Btw, your change so far is not breaking IMO because keys were always random and < 2^254 in practice and because circuits aren't changed so far.

@mitschabaude
Copy link
Member

The eventual fix IMO is to just make MerkleMap a tree of height 255 (instead of 256), and change the witness length from 255 to 254. But that would require us to create complete V2 versions of MerkleMap and MerkleMapWitness, so maybe let's do that at the next version upgrade, when this breaking change is ok and we don't have to maintain the old height-256 version.

… computeRootAndKeyV2 to avoid key collisions
…AndKey()` due to potential hash collision issue, recommend `MerkleMap.computeRootAndKeyV2()` as replacement
@MartinMinkov MartinMinkov merged commit ed198f3 into main Jun 25, 2024
14 checks passed
@MartinMinkov MartinMinkov deleted the feat/fix-merkle-map branch June 25, 2024 17:29
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

2 participants