TritonVM / tasm-lib

A collection of functions written in Triton VM assembly (tasm)
Apache License 2.0
11 stars 2 forks source link

test and benchmark infrastructure should panic on runs where stack is not initialized. #120

Open Sword-Smith opened 1 month ago

Sword-Smith commented 1 month ago

I recently wrote this implementation of Closure

    impl Closure for NoUnrolling {
        fn rust_shadow(&self, _stack: &mut Vec<BFieldElement>) {
            todo!()
        }

        fn pseudorandom_initial_state(
            &self,
            _seed: [u8; 32],
            _bench_case: Option<BenchmarkCase>,
        ) -> Vec<BFieldElement> {
            vec![]
        }
    }

I got a nasty panic deep inside of Triton VM, attempt to subtract with overflow. The reason being that I did not initialize the stack with 16 words as the testing framework requires you to do since it gives you full control over the initial state of the VM.

So pseudorandom_initial_state should have been implemented like so:

    fn pseudorandom_initial_state(
            &self,
            _seed: [u8; 32],
            _bench_case: Option<BenchmarkCase>,
        ) -> Vec<BFieldElement> {
            self.init_stack_for_isolated_run()
        }

The benchmarking and testing infrastructure should have caught this error. An assert that you initialize the stack with at least NUM_OP_STACK_REGISTERS (16) should be in place before the VM's stack is set. This should be done for both benchmarks and for tests.

There's no reason to implement the fix for any function that contains the word "deprecated" though, as this is test/benchmark infrastructure we are slowly phasing out.