Arrays of structs passed as parameter are not serialized correctly

[sirasistant]

Aim

Validating this program

struct Bar {
    valid: bool,
}
struct Point {
    x: Field,
    y: Field,
    bar: Bar,
}

fn main(
    points: [Point; 2]
) -> pub bool {
    validate_all(points)
}

fn validate(p: Point) -> bool {
    (p.x != 0) & (p.y != 0)
}

fn validate_all(points: [Point; 2]) -> bool {
    validate(points[0]) & validate(points[1])
}

Expected Behavior

Should verify with this Prover.toml

[[points]]
x = 1
y = 2
bar = {valid = 1}
[[points]]
x = 3
y = 4
bar = {valid = 1}

Bug

nargo prove t --experimental-ssa --show-ssa --print-acir
Error: could not satisfy all constraints

The initial witness is not serialized correctly for the format that ACIR expects:

Initial witness: WitnessMap({Witness(1): 1, Witness(2): 2, Witness(3): 1, Witness(4): 3, Witness(5): 4, Witness(6): 1})
acir fn main f3 {
  b0(v0: [Field; 2], v1: [Field; 2], v2: [u1; 2]):

current witness index : 21
public parameters indices : []
return value indices : [21]
BLACKBOX::RANGE [(_5, num_bits: 1)] [ ]
BLACKBOX::RANGE [(_6, num_bits: 1)] [ ]

To Reproduce

1. 2. 3. 4.

Installation Method

None

Nargo Version

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

[sirasistant]

Seems to be resolved with https://github.com/noir-lang/noir/pull/1917

[TomAFrench]

Took at look at the output from ABI encoding vs what the circuit is actually doing and these seem to be the two witness layouts involved.

// Output from ABI encoding
WitnessMap({
    Witness(1): 1, // points[0].x
    Witness(2): 2, // points[0].y
    Witness(3): 1, // points[0].bar.valid
    Witness(4): 3, // points[1].x
    Witness(5): 4, // points[1].y
    Witness(6): 1  // points[1].bar.valid
})

// Expected format from looking at ACIR
WitnessMap({
    Witness(1): 1, // points[0].x
    Witness(2): 2, // points[1].x
    Witness(3): 1, // points[0].y
    Witness(4): 3, // points[1].y
    Witness(5): 4, // points[0].bar.valid
    Witness(6): 1  // points[1].bar.valid
})

I'm not sure why ACIR gen is laying out the witnesses in this order. It seems really odd compared to what the ABI is doing.

[TomAFrench]

@jfecher why do we interlace all of the structs' fields in an array of structs in ACIR gen?

[sirasistant]

I think it's because the SSA/ACIR expects array of structs to struct of arrays conversion but it's no longer done when generating the ABI

[sirasistant]

And I think the PR I mentioned fixes it because it removes that conversion from the SSA

[jfecher]

@TomAFrench as @sirasistant mentioned, I believe it is to match the Array of structs -> struct of arrays conversion that is performed during monomorphization. I'd like to remove this but can't until we remove the old ssa code which doesn't support arrays of structs.

@sirasistant do you know if #1917 still exhibits the issue if you don't pass the --experimental-ssa flag? I believe that PR only conditionally disables the AOS2SOA conversion if you're using the ssa refactor. So if you're not passing the flag, the issue will still be present.

[sirasistant]

@jfecher old ssa has a catch for this apparently:

The application panicked (crashed).
Message:  internal error: entered unreachable code: array of structs are not supported for now
Location: crates/noirc_evaluator/src/ssa/ssa_gen.rs:87

[jfecher]

Ah, right. Since the old SSA crashes either way I think we can consider this fixed once #1917 is merged then.

[kevaundray]

Closing as #1917 has been merged