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

feat: verifier returns all outputs rather than their hash #57

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Dec 19, 2024

The fact that the verifier returns the hash of the output rather than the full outputs prevents a lot of use cases.
Eg. I want to accept a proof in my application if and only if the value written in the inputs is the pubkey that belong to a set I maintain. With only the hash I won't be able to check if this pubkey belong to my set.

This is a breaking change.
People who rely on the old behavior (hash of all outputs) of this lib will have to compute the hash themselves.

Sidenote:

  • There is room for code factorization between the different layouts.
  • It would theoretically be possible to return an iterator over &Felt instead of a Vec. This would avoid both collecting and cloning the returned values but at the cost of more complexity in the trait.

@Okm165 Okm165 self-assigned this Dec 20, 2024
@Okm165 Okm165 added the enhancement New feature or request label Dec 20, 2024
@Okm165
Copy link
Contributor

Okm165 commented Dec 20, 2024

We can export program hash and output hash util functions in the project for people to use (in couple flavors: pedersen, poseidon and keccak for example). Also wondering if raw program byte-code could be returned for sake of symmetry so user could just pass it to util function in his project lmk wdyt?

The byte-code is there in raw form we should have a way to extract it raw imo as we do with output.

@tdelabro
Copy link
Contributor Author

#59 provides it for the program hash
I can also factorize and export the output_hash in this one.

About the byte-code I'm not sure.
From my perspective, as a user of the verifier, I just want to provide the proof, get a bool and the output, and that is all.
But maybe I'm missing a use-case here

@tdelabro
Copy link
Contributor Author

So after checking it appears that the method to compute the hash of the output (once returned as a vec) already exists in the ecosystem:

pedersen:
https://github.com/xJonathanLEI/starknet-rs/blob/8755b68675f103ee011f1d964fcec55f1902fa09/starknet-core/src/crypto.rs#L66
poseidon:
https://github.com/xJonathanLEI/starknet-rs/blob/8755b68675f103ee011f1d964fcec55f1902fa09/starknet-crypto/src/poseidon_hash.rs#L72

Whatever method I can expose will just be a wrapper over those. Maybe it's not useful to reexpose those in swiftness.

@Okm165
Copy link
Contributor

Okm165 commented Dec 20, 2024

You are right lets have just logic for extraction of program and output to Vec<Felt> in swiftness then in examples we can present hashing these with https://github.com/xJonathanLEI/starknet-rs utils so we don't redefine anything

@tdelabro
Copy link
Contributor Author

tdelabro commented Jan 7, 2025

@Okm165 do you want to merge this, or has there been a change in plan?

@Okm165
Copy link
Contributor

Okm165 commented Jan 7, 2025

@tdelabro i think good idea would be to add example of calculating hash with starknet-rs tools so users before this breaking change are not lost when this changes apply
I guess good place is here https://github.com/iosis-tech/swiftness/blob/main/cli/src/main.rs

@tdelabro
Copy link
Contributor Author

tdelabro commented Jan 7, 2025

I can add an --hash-output argument to the program to that if it is passed, you will get the hash instead of the array. Wdyt?

@Okm165
Copy link
Contributor

Okm165 commented Jan 7, 2025

That is great solution

@tdelabro tdelabro force-pushed the verify-return-all-output branch from 7d40c23 to f151e66 Compare January 9, 2025 18:09
@tdelabro
Copy link
Contributor Author

tdelabro commented Jan 9, 2025

@Okm165 I pushed the CLI new arg

Copy link
Contributor

@Okm165 Okm165 left a comment

Choose a reason for hiding this comment

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

LGTM

@Okm165 Okm165 merged commit f7c539f into iosis-tech:main Jan 9, 2025
18 checks passed
@tdelabro tdelabro deleted the verify-return-all-output branch January 10, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants