Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable fill_builtin_segment when it is not run in proof_mode #436

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ohad-nir-starkware
Copy link
Collaborator

@ohad-nir-starkware ohad-nir-starkware commented Feb 2, 2025

This change is Reviewable

Copy link

@DavidLevitGurevich DavidLevitGurevich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remind me why we need this change?

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @ohad-starkware and @shaharsamocha7)

Copy link
Collaborator Author

@ohad-nir-starkware ohad-nir-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes the adapter is run not in proof_mode and in such instances the runner doesn't leave space for padding and fill_builtin_segment gets an error when checking for available space for padding.

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @ohad-starkware and @shaharsamocha7)

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.There is no point in running the adapter in that case
2. proof mode does not propagate from input, so you don't know it, so it's hardcoded "yes", which is bad because I can run non-proof-mode stuff with it and it will think it was in proof mode

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ohad-nir-starkware and @shaharsamocha7)


-- commits line 2 at r1:
we don't need this pr
plus it's incorrect :(

Copy link
Collaborator Author

@ohad-nir-starkware ohad-nir-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It does get there, ask Meidar.
  2. I made it propagate there from the different possible flows.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.I don't understand how is it possible to prove anything when the segments are not prepared to be padded
2. adapted prover is a hard coded yes, which is an implicit way of asserting that you only give it proof mode stuff. You can still give it non proof mode stuff.

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ohad-nir-starkware and @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 126 at r1 (raw file):

        StateTransitions::from_iter(trace_iter, &mut memory, dev_mode);
    let mut builtins_segments = BuiltinSegments::from_memory_segments(memory_segments);
    if proof_mode {

document

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants