chore: turn on elaborator

[TomAFrench]

This PR turns off usage of the legacy name resolution/typechecking code to instead use the new elaborator.

I'm seeing panics when performing keccak hashes however.

[nventuro]

We get some build errors in a couple contracts, this seems to be related to the macros that deal with enqueuing calls.

[Thunkar]

I think I found out the issue: it's not the enqueue calls, but the way we're injecting the calculated function signatures after the HIR pass.

Since we don't have the typedchecked AST when generating the contract interfaces, we leave a placeholder to be replaced during the HIR pass (example taken from crowdfunding_contract.

 pub fn _check_deadline(self) -> dep::aztec::context::PublicStaticVoidCallInterface<15,()> {
            let mut args_acc: [Field] = &[];

            let selector = dep::aztec::protocol_types::abis::function_selector::FunctionSelector::from_signature("SELECTOR_PLACEHOLDER");
            dep::aztec::context::PublicStaticVoidCallInterface {
                target_contract: self.target_contract,
                selector,
                name: "_check_deadline",
                args: args_acc,
                original: | inputs: dep::aztec::context::inputs::PublicContextInputs | -> () {
                    _check_deadline(inputs)
                },
                is_static: true,
                gas_opts: dep::aztec::context::gas::GasOpts::default()
            }
        }

In this case, "SELECTOR_PLACEHOLDER" would be replaced by "_check_deadline()". The compiler is crashing here because "SELECTOR_PLACEHOLDER" is 20 characters long and "_check_deadline()" is 17, which is generating an incorrect range check in the SSA:

I've changed the placeholder to be an empty string and it seems to work, but would love @jfecher's input on this.

[AztecBot]

Docs Preview

Hey there! 👋 You can check your preview at https://66967a58fb4c60ad2ffc6551--aztec-docs-dev.netlify.app

[Thunkar]

While at it, added a better error message for noir-lang/noir#5384

@TomAFrench I don't think it's worth it investing in a better solution, since that generic is going away first and then the whole macros folder (hopefully soon) after.

[jfecher]

I've changed the placeholder to be an empty string and it seems to work, but would love @jfecher's input on this.

The panic you're getting looks to be a result of simplifying a constant call to Keccak256. When simplifying that call in the compiler it looks like we have no checks at all that the second argument (message_size) is actually in bounds of the first argument (the array). This is surely good code.

For now, maybe check your calls to keccak256?

The compiler is crashing here because "SELECTOR_PLACEHOLDER" is 20 characters long and "_check_deadline()" is 17

I'm assuming this was just speculation? Since the panic location in SSA in the code doesn't deal with names at all.

No idea how this would be triggered or affected by the SELECTOR_PLACEHOLDER / _check_deadline check though.

Also, surprise code review:

original: | inputs: dep::aztec::context::inputs::PublicContextInputs | -> () {
                    _check_deadline(inputs)
                },

This should be able to be simplified to:

original: _check_deadline,

[jfecher]

Another thing to note is that in the elaborator the aztec_macros call on the typed AST occurs after type checking where as in the legacy code it occurred before type checking. I'm hoping this won't be an issue but am assuming it will be.

[Thunkar]

I clearly didn't do a good job of explaining the issue and the fix 😅

Those keccak256 calls are done when generating function selectors from signatures (which are strings). We are essentially "tricking" the compiler, giving it a particular string and then replacing its value with another one after the HIR pass, which seems to confuse it (the bytes.len() call is somehow not "updated" with the new string length) which permeates all the way to the SSA generating invalid range checks. This seems to be in line with what you said:

Another thing to note is that in the elaborator the aztec_macros call on the typed AST occurs after type checking where as in the legacy code it occurred before type checking. I'm hoping this won't be an issue but am assuming it will be.

Could it be possible that the as_bytes string builtin gets executed before we perform the swap and therefore the length of the string passed to keccak256 is not updated?

Might be completely off base though.

This should be able to be simplified to:

original: _check_deadline,

Unfortunately this is a very simple case of autogenerated code with no args in public. More complex ones required the verbose version 😢

[jfecher]

Could it be possible that the as_bytes string builtin gets executed before we perform the swap and therefore the length of the string passed to keccak256 is not updated?

I don't think so. The macros are executed before monomorphization which is before SSA where any calls to keccak256 are possibly optimized. It's possible the change isn't being picked up for some reason.

[Thunkar]

Sorry @TomAFrench to be hijacking your PR, but we really need this ASAP so we don't fall behind. Currently investigating some hash mismatches in the e2e tests, really hoping for CI issue, but doesn't look like it.

[TomAFrench]

Hey no worries, this isn't hijacking at all. I opened this just to track what errors were being emitted so they can be fixed so I'm expecting other people to jump in.

[Thunkar]

After looking into the issue, it seems like one of the interner APIs we use in the macros (context.def_interner.update_expression) was generating problems during monomorphization and the wrong version of the FunctionSelector::from_signature function was being selected. I have no idea why and it looks very non trivial to fix...so I didn't, and looked for a workaround instead (we're gonna kill this code soon™️ anyways)

Since updating expressions doesn't seem to generate any problems in cases where we don't have to deal with generics at all, I decided to use the same approach as in note_interface.nr and generate the selector "offline". This is actually better in terms of constraints and really the way we will hopefully do it when we migrate to metaprogramming, since those values are known at compile time and doesn't make sense to recompute and constrain them.

[Thunkar]

These changes are due to this bug: noir-lang/noir#5523

It's the only difference in behavior (besides macro fuckery) we've found

[sklppy88]

nice !