search

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

Fix: set up builtins initial stack based on program builtins #1632

Closed odesenfans closed 4 months ago

odesenfans commented 6 months ago

Problem: when using allow-missing-builtins for a program, some builtins requested by the program may not be part of the layout. This results in an incorrect configuration of the initial stack of the program as the program will expect pointers for all builtins, even though it may not access them later down the line. An example of this is any program that invokes another program (ex: the bootloader).

Solution: copy the Python VM implementation. We now iterate over the program builtins and set the initial stack of the builtin if it is set up in the VM and a default value of (0, 0) otherwise.

Fixes #1631.

Checklist

odesenfans commented 6 months ago

A few points of attention for the review:

  1. get_builtins_final_stack appears to be used in tests only, I wonder if that function is still relevant. I duplicated the change made in read_return_values as it seemed pertinent, but I may be missing some context.
  2. The get_output_unordered_builtins test does not seem to make sense so I disabled it. If I understand this correctly the test checks if a program can specify its builtins out of order, but that seems like a silly idea. Might have misunderstood though, I didn't want to disassemble the program to check.
odesenfans commented 6 months ago

I pushed a fix. The borrow checker was being a bit zealous for some reason.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 98.26087% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 96.56%. Comparing base (ca3b8ab) to head (0bcc83c).

:exclamation: Current head 0bcc83c differs from pull request most recent head ed23efc. Consider uploading reports for the commit ed23efc to get more accurate results

Files Patch % Lines
vm/src/vm/runners/cairo_runner.rs 98.26% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1632 +/- ## ========================================== + Coverage 94.96% 96.56% +1.59% ========================================== Files 99 95 -4 Lines 38532 38464 -68 ========================================== + Hits 36592 37142 +550 + Misses 1940 1322 -618 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

juanbono commented 5 months ago

@odesenfans can you check the comments by @fmoletta ?

fmoletta commented 4 months ago

This has been fixed by #1703