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

Why does StarkProof.verify return two felts? #48

Open
tdelabro opened this issue Dec 9, 2024 · 9 comments
Open

Why does StarkProof.verify return two felts? #48

tdelabro opened this issue Dec 9, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@tdelabro
Copy link

tdelabro commented Dec 9, 2024

https://docs.rs/swiftness/latest/swiftness/types/struct.StarkProof.html#method.verify

I understand the first one is the program hash, which make sense, and that the second is the hash of the public output. Which doesn't totally make sense to me. Why this choice rather than returning the full output?

@tdelabro
Copy link
Author

tdelabro commented Dec 9, 2024

Like what if my use case is:

  • the proof needs to be valid
    AND
  • the first returned value must be equal to this specific nonce
    ?

With the current impl, I cannot run checks upon the public output myself.

@Okm165
Copy link
Contributor

Okm165 commented Dec 11, 2024

Hi I understand your use-case, the way that hashing output makes sense is to assert that output is not growing indefinitely in recursion schemes that leverage verification of previous layers.
Verifier always outputs 2 felts for consistency.

@Okm165
Copy link
Contributor

Okm165 commented Dec 11, 2024

@tdelabro we could add optional return of raw output, let me know if we should support it, do u wanna contribute?

@tdelabro
Copy link
Author

I would make it a separate method rather than an optional argument, but yeah it would be nice.

I'm not sure I will find time for it, don't rely on me sorry :)

@tdelabro
Copy link
Author

tdelabro commented Dec 17, 2024

        let output_len: usize = (output_stop - output_start).to_bigint().try_into()?;
        let output = &memory[memory.len() - output_len * 2..];
        let hash = output.iter().skip(1).step_by(2).fold(FELT_0, |acc, e| pedersen_hash(&acc, e));
        let output_hash = pedersen_hash(&hash, &Felt::from(output_len));

Also, this code is very obscure.
We need some comments to understand why this is the implementation. Is it dictated by some starkware specification or just something you decided by yourself?

Why .skip(1)? We don't want to use the very first output value. Why that?
Why output_len * 2? I don't see any reason to use that rather than output_len. We need it explained.

@tdelabro
Copy link
Author

tdelabro commented Dec 17, 2024

If you don't add the method that returns the full output, you should at least give me a function that allows me to pass my expected output as an argument and return the hash computed the way you are doing it.
Otherwise, I cannot check that the returned output_hash is the one that matches the values I expected.

The same probably applies to computing the program hash.
I believe I can compute mine using cairo-pie/metadata.json.
It contains {"program":{"data":[5191102242953854976,5191102247248822272, ...], ...}}
I believe this is what you are using here:

        let hash = program.iter().fold(FELT_0, |acc, &e| pedersen_hash(&acc, e));
        let program_hash = pedersen_hash(&hash, &Felt::from(program.len()));

@Okm165
Copy link
Contributor

Okm165 commented Dec 17, 2024

output_start and output_stop are the addresses of the memory
memory is serialized as [address1, value1, address2, value2, ...]

so we calculate output_len and mul it by 2 to traverse the correct amount of elems in serialized memory
later we want to extract the values: so we skip address1 and step by 2 to extract value1, value2 ...

@tdelabro
Copy link
Author

tdelabro commented Dec 17, 2024

Okay, thanks a lot for the explanation.
I really think this should be written somewhere (both the specification used (hashchain of outputs + outputs len) and the details of this impl (I had no idea how memory was serialized while reading this function))

Now I'm able to write my hashing of the outputs to match yours, but I still think it would be better if you provide it yourselves in some utils module.
And of course the function that return all outputs too

@Okm165 Okm165 added enhancement New feature or request good first issue Good for newcomers labels Dec 17, 2024
@Okm165 Okm165 self-assigned this Dec 17, 2024
@tdelabro
Copy link
Author

@Okm165 I can take this one now if you want. I have to code it locally anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants