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 merkle tree hashing for poseidon #277

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Fix merkle tree hashing for poseidon #277

merged 2 commits into from
Feb 20, 2024

Conversation

Leonard-Pat
Copy link
Contributor

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

The current Poseidon hashing implementation for the merkle tree is incorrect. It uses

            let mut state = PoseidonTrait::new();
            state = state.update(data1);
            state = state.update(data2);
            state.finalize()

Under the hood this actually hashes the elements twice (once from the state update and once from finalization, see corelib), the intended behaviours is that only one round of hashing is done (when hashing two elements). This is noted in https://docs.starknet.io/documentation/architecture_and_concepts/Cryptography/hash-functions/#poseidon_hash where by the hash of two elements is simply
Screenshot 2024-02-19 at 11 53 04.
Also please see POC https://github.com/Leonard-Pat/merkle-tree-failing

  • Fixes Poseidon hashing for merkle tree poseidon impl

Does this introduce a breaking change?

  • Yes
  • No

Other information

Updated tests to reflect this change - note the gas limit was exceeded and therefore needed to be udpated as well

@Leonard-Pat Leonard-Pat changed the title Fix merkle tree hashes for poseidon Fix merkle tree hashing for poseidon Feb 19, 2024
Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

can you also fix pedersen plz

@Leonard-Pat
Copy link
Contributor Author

Leonard-Pat commented Feb 19, 2024

can you also fix pedersen plz

I believe Pedersen is correct as the finalization step does not add another round of hashing

@0xLucqs
Copy link
Collaborator

0xLucqs commented Feb 20, 2024

ah indeed but i think that using directly the pedersen function would reduce a bit the step cost

@Leonard-Pat
Copy link
Contributor Author

ah indeed but i think that using directly the pedersen function would reduce a bit the step cost

okay done :)

@0xLucqs 0xLucqs merged commit e14c6b0 into keep-starknet-strange:main Feb 20, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants