Skip to content

Updated ret opcode test.#52

Open
Alon-Ti wants to merge 1 commit intomainfrom
alont/ret-test
Open

Updated ret opcode test.#52
Alon-Ti wants to merge 1 commit intomainfrom
alont/ret-test

Conversation

@Alon-Ti
Copy link
Contributor

@Alon-Ti Alon-Ti commented Feb 19, 2025

This change is Reviewable

@Alon-Ti Alon-Ti marked this pull request as ready for review February 19, 2025 14:18
@Alon-Ti Alon-Ti requested a review from giladchase February 19, 2025 14:19
Copy link
Contributor Author

Alon-Ti commented Feb 19, 2025

Copy link

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion


crates/prover/src/components/ret_opcode/mod.rs line 70 at r1 (raw file):

            };
            256
        ]);

Why cant we add another state here to verify the ret switched to the correct state, like in the addmul test? something like:

chain!(
            VmState {
                pc: 0,
                ap: 3,
                fp: 3,
            };
            128,
              vec![
                    VmState {
                        pc: 5678, // probably need smaller numbers here and in fp to make it easier.
                        ap: 7,
                        fp: 1234,
                    };
                    128
                ]
).collect_vec()

I tried adding this, and it passed, but also putting incorrect values in PC/fp worked 😅

Code quote:

        let claim_generator = ClaimGenerator::new(vec![
            VmState {
                pc: 0,
                ap: 3,
                fp: 3,
            };
            256
        ]);

@Alon-Ti Alon-Ti force-pushed the alont/par-addmul branch 2 times, most recently from 878d5d5 to 3c402f1 Compare March 1, 2025 13:59
@Alon-Ti Alon-Ti changed the base branch from alont/par-addmul to graphite-base/52 March 1, 2025 14:02
@Alon-Ti Alon-Ti changed the base branch from graphite-base/52 to main March 1, 2025 14:03
Copy link
Contributor Author

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @giladchase)


crates/prover/src/components/ret_opcode/mod.rs line 70 at r1 (raw file):

Previously, giladchase wrote…

Why cant we add another state here to verify the ret switched to the correct state, like in the addmul test? something like:

chain!(
            VmState {
                pc: 0,
                ap: 3,
                fp: 3,
            };
            128,
              vec![
                    VmState {
                        pc: 5678, // probably need smaller numbers here and in fp to make it easier.
                        ap: 7,
                        fp: 1234,
                    };
                    128
                ]
).collect_vec()

I tried adding this, and it passed, but also putting incorrect values in PC/fp worked 😅

The execution is just a set of states, you can't assume that two consecutive ones feed into each other...

@graphite-app
Copy link

graphite-app bot commented Mar 2, 2025

Merge activity

  • Mar 2, 4:50 AM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

Copy link

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

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.

2 participants