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

chore: Experimenting with stepwise ACVM solver [DO NOT MERGE] #1729

Closed
wants to merge 17 commits into from

Conversation

TomAFrench
Copy link
Member

Description

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench
Copy link
Member Author

@kevaundray Heads up that the new SSA code is still generating opcodes out of order.

Base automatically changed from acvm-0.16.0 to master July 6, 2023 13:18
@TomAFrench TomAFrench changed the title chore: Experimenting with new ACVM solver [DO NOT MERGE] chore: Experimenting with stepwise ACVM solver [DO NOT MERGE] Jul 10, 2023
@TomAFrench TomAFrench force-pushed the acvm-struct branch 2 times, most recently from dad6fac to 2fdd5fe Compare July 10, 2023 05:15
@TomAFrench
Copy link
Member Author

TomAFrench commented Jul 10, 2023

I'm getting failures on the program

use dep::std;

fn main(x: Field, y: Field, salt: Field, out_x: Field, out_y: Field ) {
    // let res = std::hash::pedersen([x, y]);
    // assert(res[0] == out_x);
    // assert(res[1] == out_y);

    let raw_data = [x,y];
    let mut state = 0;
    for i in 0..2 {
        state = state * 8 + raw_data[i];
    }
    state += salt;
    let hash = std::hash::pedersen([state]);
    // assert(std::hash::pedersen([43])[0] == hash[0]);
}   

Which results in the ACIR (pre/post optimization)

Unoptimised ACIR for main:
current witness index : 8
public parameters indices : []
return value indices : []
EXPR [ (8, _1) (1, _2) (1, _3) (-1, _6) 0 ]
BLACKBOX::PEDERSEN [(_6, num_bits: 254)] [ _7, _8] domain_separator: 0

Compiled ACIR for main:
current witness index : 11
public parameters indices : []
return value indices : []
EXPR [ (1, _3) (-1, _6) (-1, _10) 0 ]
EXPR [ (8, _1) (1, _2) (1, _10) 0 ]
BLACKBOX::PEDERSEN [(_6, num_bits: 254)] [ _7, _8] domain_separator: 0

Looks like it's running into the issue of not knowing how to sort the arithmetic opcodes. I purposefully removed the existing sorting here as it results in the bad outcome as described in noir-lang/acvm#419 but it seems like the underlying logic can't handle all cases properly either (which makes sense as it has no information on which witness was the "output" of the original expression and so is now included in an expression with two unknowns.

@TomAFrench
Copy link
Member Author

As a rough heuristic we should prefer to attach intermediate variables to the lowest value witnesses in the expression.

@kevaundray
Copy link
Contributor

@TomAFrench Is this still a [DO NOT MERGE] PR or will this be used for introducing stepwise execution?

@TomAFrench
Copy link
Member Author

I'll probably repurpose this PR but for now it's a scratch branch while I get dynamic arrays working.

@TomAFrench
Copy link
Member Author

Closing in favour of #2051

@TomAFrench TomAFrench closed this Jul 26, 2023
@TomAFrench TomAFrench deleted the acvm-struct branch July 26, 2023 13:36
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