lambdaclass / cairo-vm

cairo-vm is a Rust implementation of the Cairo VM. Cairo (CPU Algebraic Intermediate Representation) is a programming language for writing provable programs, where one party can prove to another that a certain computation was executed correctly without the need for this party to re-execute the same program.
https://lambdaclass.github.io/cairo-vm
Apache License 2.0
504 stars 138 forks source link

Builtin pointers not allocated when running a program with `allow-missing-builtins` #1631

Closed odesenfans closed 4 months ago

odesenfans commented 6 months ago

Describe the bug For an experiment, I tried to execute the Starknet bootloader with layout starknet and allowing missing builtins to suppress the warning about the absence of the keccak builtin in the layout. This bypasses the builtin check successfully but yields the following error:

$ ./target/debug/stone-prover-cli prove --with-bootloader dependencies/cairo-programs/cairo0/fibonacci/fibonacci.json --verifier=l1 --layout=starknet --allow-missing-builtins --output-file /tmp/proof.json
info  execution in progress...
error failed to run Cairo program: starkware/cairo/bootloaders/bootloader/bootloader.cairo:60:5: While trying to retrieve the implicit argument 'poseidon_ptr' in:
/home/olivier/git/moonsong-labs/starkware/cairo-lang/src/starkware/cairo/bootloaders/simple_bootloader/run_simple_bootloader.cairo:21:5: While expanding the reference 'poseidon_ptr' in:
    poseidon_ptr,
    ^**********^
starkware/cairo/bootloaders/bootloader/bootloader.cairo:37:5: Error at pc=0:553:
Couldn't compute operand op1. Unknown value for memory cell 1:9
Cairo traceback (most recent call last):
<start>:3:1: (pc=0:2)

This is somewhat surprising because the missing builtin is keccak, not poseidon. Upon further inspection, I realized that the Cairo program expects pointers to the builtins in the main function:

func main{
    output_ptr: felt*,
    pedersen_ptr: HashBuiltin*,
    range_check_ptr,
    ecdsa_ptr,
    bitwise_ptr,
    ec_op_ptr,
    keccak_ptr,
    poseidon_ptr,
}() {

My best guess so far is that the issue happens because the builtin pointers are generated from the list of builtins in the layout, and not the list of builtins required by the program.

Expected behavior The pointers for each builtin should be initialized based on what the program requires, not on the builtins available in the layout.

What version/commit are you on? 59254f4c53594217e0182e04008e56df5c187889

pefontana commented 6 months ago

Hi @odesenfans ! Is there a way we can replicate the error?

odesenfans commented 6 months ago

Damn you're fast @pefontana ! I'll add one based on the new CLI tool we just released, let me create an example branch!

odesenfans commented 6 months ago

This should allow you to reproduce the issue:

git clone --recurse-submodules https://github.com/Moonsong-Labs/stone-prover-cli.git --branch od/reproduce-cairo-vm-allow-missing-builtin-issue
cd stone-prover-cli/
cargo build
./target/debug/stone-prover-cli prove --with-bootloader dependencies/cairo-programs/cairo0/fibonacci/fibonacci.json --verifier=l1 --layout=starknet --allow-missing-builtins
odesenfans commented 6 months ago

I think the problem comes from this function and causes the issue here.

As keccak is not in the layout, vm.builtin_runners only has 7 entries. The program entrypoint expects all 8 values to be present on the stack, hence the failure. Instead, initialize_main_entrypoint should add values on the stack for each builtin specified in the program. I'll check what the Python VM does for this, I've been told it's compatible so it might just be a matter of reproducing the same behaviour.

odesenfans commented 6 months ago

Yep, the Python VM does this:

        for builtin_name in self.program.builtins:
            builtin_runner = self.builtin_runners.get(f"{builtin_name}_builtin")
            if builtin_runner is None:
                assert self.allow_missing_builtins, "Missing builtin."
                stack += [0]
            else:
                stack += builtin_runner.initial_stack()

I'll try to implement this.

odesenfans commented 6 months ago

1632 fixes the issue, but I end up with another error:

Invalid stop pointer for ec_op_builtin: Stop pointer has value 0:0 but builtin segment is 7
odesenfans commented 6 months ago

Pushed a fix on #1632. There were issues when finalizing the stack for builtins.

pefontana commented 6 months ago

Amaizing @odesenfans !! Thanks for the fix, we will review it

fmoletta commented 4 months ago

Fixed by #1703