DO NOT MERGE minimal example of bugs

[iAmMichaelConnor]

See main.nr for the word BUG where there are considerably more constraints being generated than I'd expect.

The most interesting bug is:

for i in FIELDS_PER_BLOB..NOIR_FIELDS_PER_BLOB {
        // the top_bits are assumed to be big-endian bit arrays:
        let mut reconstituted_field = top_bits[0];
        for j in 1..254 {
            let k = i * 254 + j;
            reconstituted_field *= 2;
            reconstituted_field += top_bits[k]; // BUG: k is known at compile-time, so this shouldn't be contributing 24k constraints?
            std::as_witness(reconstituted_field); // BUG: this is costing 4048 gates
        }
        blob_as_fields[i] = reconstituted_field;
    }

Notice, k = i * 254 + j should be known at compile time, but top_bits[k] is being accessed like a ROM read, as shown in this flamegraph screenshot:

I've stripped-away most of the blob stuff, to hone in on the problematic functions.

---

You can view a flamegraph of this stuff! The svg file is pushed, so you won't need to recompile.

cd noir-circuits/blob

python3 -m http.server --directory "./flamegraph" 3000

Open localhost:3000 (make sure port 3000 is forwarded from the mainframe)

---

If you make any changes, here's how to regenerate the flamegraph:

nargo compile

To view a pretty flamegraph:

PATH/TO/YOUR/MONOREPO/VERSION/OF/noir/noir-repo/target/release/noir-profiler gates-flamegraph --artifact-path ./target/blob.json --backend-path ~/.bb/bb --output ./flamegraph --backend-gates-command "gates_mega_honk" -- -h && python3 -m http.server --directory "./flamegraph" 3000

Open localhost:3000 (make sure port 3000 is forwarded from the mainframe)

[TomAFrench]

You likely mean let k = (i - FIELDS_PER_BLOB) * 254 + j; here as otherwise you're going to be reading way past the end of top_bits.

The reason you're getting extra array_gets is that in the case where you're reading past the end of the array we're unable to simplify the instruction despite having a constant index.

[iAmMichaelConnor]

Whoops, thanks for spotting that!
So Noir doesn't throw an error when you try to read an array out of bounds? I'd have expected a compile-time or run-time error being thrown? We know the size of the array at compile-time, and we know the (incorrectly computed) value of k? (Just wanting to understanding this, because I must be thinking about compilers incorrectly).
I'd have at least expected the tests (on the main branch) to throw, whenever they reach this point. "Let's access this array of length 4064 at an index larger than 4063", and then for it to go "AAAH PANIC, CAN'T DO THAT MATE!"

[TomAFrench]

The tests should throw an error during exexution, if that's not happening then something is wrong.

re compile-time error: we removed most of these as compile time errors for cases such as this result in issues for tests where performing an OOB array read would prevent the test from executing which interferes with debugging.

[TomAFrench]

It's not clear to me why this should be free. You're returning a value from an unconstrained function (which can be anything) and asserting that it is equal to a value in the circuit.

We can't trust that the prover will return the limbs from x as specified in the unconstrained function so we cannot optimise out these constraints.

[TomAFrench]

In this situation it would be best to not return an F from __unsafe_extract_top_bit but just the top bit and top limb. You can then construct the new F in a constrained function more cheaply than doing it an unconstrained function and making assertions about it.

[TomAFrench]

unconstrained fn __unsafe_extract_top_bit(x: F) -> (Field, Field) {
    let top_limb: Field = x.limbs[2];
    // The top_limb is at most 2 bytes (16 bits).
    // 0x8000 = 2^15 = 32768
    let top_bit: Field = (top_limb as u16 / 0x8000) as Field;
    let top_limb_with_top_bit_removed = top_limb - top_bit * 0x8000;
    (top_bit, top_limb_with_top_bit_removed)
}

// DANGER: it's named as "unsafe" because the caller MUST already have checked that 
// each blob Field is formatted as (u1, Field). I.e. the "rhs" 254-bits should already
// fit within a Field. If the "rhs" 254 bits is larger than the field modulus,
// there will be an uncaught overflow of the 254-bits in the Field, resulting in 
// an unintended tiny value.
//
// For efficiency, the top_bit is kept as a Field throughout.
fn unsafe_extract_top_bit(x: F) -> (Field, F) {
    let (top_bit, top_limb_with_top_bit_removed) = __unsafe_extract_top_bit(x);
    assert_eq(top_bit * 0x8000 + top_limb_with_top_bit_removed, x.limbs[2]);

    (top_bit, BigNum { limbs: [x.limbs[0], x.limbs[1], top_limb_with_top_bit_removed] })
}

[iAmMichaelConnor]

I thought asserting two witnesses to be equal can be done with a copy-constraint, which just leverages the permutation polynomial stuff of plonk, instead of creating a gate?
I think that's what bberg does. It does a weird thing where it actually just sets one of the arguments of assert_equal to have the same witness index as the other, iirc.

Maybe I'm misremembering because I've been out of the weeds for 2 years.

Thanks for taking the time to suggest an improvement here :)

[TomAFrench]

This is something that we can maybe do in the DSL folder but in Noir this is just an AssertZero opcode like any other.

[TomAFrench]

Generally though if you want continuity it's best to do that entirely in constrained-land.

[TomAFrench]

@vezenovm This is falling afoul of the restricted codegen width, right?

[iAmMichaelConnor]

In the main branch, I'm throwing std::as_witness() around like throwing darts blindfolded.

I haven't fully grasped when it's actually needed.

I seem to have adopted a rule (without reason) that I should include it for any variable which is declared outside of a for loop, and mutated inside the for loop. 🤷‍♂️

[vezenovm]

Essentially it should be used when you know an expression can fit inside of the width of the backend you are using and you don't want Noir to create extra intermediate gates. It is a bit hard to determine exactly when as_witness is needed and it usually requires a bit of experimentation to determine if Noir is creating an unnecessary intermediate gates. We have made efforts to enable ACIR gen to aware of the backend expression width and the --bounded-codegen flag can assist in determining if as_witness would be helpful. Although I did run nargo info --force --bounded-codegen --expression-width=4 on this program and I am in fact getting a lower gate count with just nargo info 🤔 . I would just not use as_witness for now as if it is not used correctly it will add more gates, which looks to be in line with what we are seeing.

[vezenovm]

I'm also seeing an increase of ~2k gates not 4k gates. 30481 (w/o as_witness) -> 32513 (w/ as_witness)

And if you are curious I got 31153 w/ the --bounded-codegen flag.

[TomAFrench]

Field overflows silently

[iAmMichaelConnor]

Oh yeah this is an artifact of late-night stupidity on my part. Thankfully it's just dangling here and not used anywhere.