o1-labs / snarky

OCaml DSL for verifiable computation
MIT License
492 stars 73 forks source link

[DO NOT MERGE] removing references in Run_state.t #800

Closed mimoo closed 1 year ago

mimoo commented 1 year ago

References in Run_state.t

There are two fields in the Run_state that are references (int ref and bool ref). It seems like a very dangerous pattern to me. There's two scenarios in the current logic that I'm worried of (as we transition to a Rust struct):

Let's check to make sure! There are two reference fields: as_prover and next_auxiliary.

As_prover

This one is easy, it always gets initialized to false, and you can't retrieve the reference (only its value). So we can safely remove the reference.

You can see it in this code change that was not merged but only created to showcase the change: https://github.com/o1-labs/snarky/pull/800/commits/55ae8dab6a5d38a7690170bcfaad0f571a509d3b

Next_auxiliary

The reference can't be retrieved either, as this is the only way to get the value out of it:

let next_auxiliary { next_auxiliary; _ } = !next_auxiliary

Note that the make function also modifies next_auxiliary due to the R1CS trick:

next_auxiliary := 1 + num_inputs

So it is actually never used, it is just a way to return the correct value for next_auxiliary to the caller.

The only real danger here is then if the caller starts mutating this after calling make.

This commit shows that there's no danger there as well: https://github.com/o1-labs/snarky/pull/800/commits/7549ca9e5151bca214e5823dae513298828ea468