search

lurk-lab / arecibo

An advanced fork of Nova
https://lurk-lang.org/
MIT License
63 stars 27 forks source link

Supernova: better slices handling #360

Open tchataigner opened 3 months ago

tchataigner commented 3 months ago

While trying to further my understanding of how to handle the program counter I have encountered a case where we have a panic in supernova::snark::CompressedSNARK::verify.

It happened a conversation I had with @adr1anh about what value should the program counter of the last StepCircuit of a NIVC computation. After the discussion, I tried to set the pc to -F::ONE and I ended up with a panic as I was trying to access an index out of bounds.

This is due to the fact that in the supernova codebase there are a few places (1 , 2, ...) where we try to access indexes without ensuring they exist.

I suggest to use a .get method to return a proper error.

wwared commented 3 months ago

Worth mentioning: returning a dummy out-of-bounds PC value might turn out to be an anti-pattern that we might not want to necessarily support (depending on the nature of the panics we're getting); a possible alternative would be including an additional "terminal" circuit that exists but is always unsatisfiable, and return the PC value as the index for this terminal circuit

porcuquine commented 3 months ago

I fully support panicking in the case of a programmer error that should never survive into a finished application. I think it's reasonable to give a more instructive message when so panicking, though.

The point is: that it is categorically wrong for a circuit to supply an invalid next program counter, so the only question is how gently we want to let the programmer down. I don't think it needs to be gentle at all, but there's no harm in being instructive.