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.

Sign up for GitHub

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

Jump to bottom

Sync temp fix 2: crash inserting acir-only instruction into brillig runtime #7247

Open
TomAFrench opened this issue Jan 31, 2025 · 1 comment · May be fixed by #7279
Open

Sync temp fix 2: crash inserting acir-only instruction into brillig runtime #7247

TomAFrench opened this issue Jan 31, 2025 · 1 comment · May be fixed by #7279

Comments

@TomAFrench
Copy link
Member

#7241 (comment)
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jan 31, 2025
@aakoshh aakoshh changed the title Sync temp fix 2 Sync temp fix 2: crash inserting air-only instruction into brillig runtime Feb 4, 2025
@aakoshh
Copy link
Contributor

aakoshh commented Feb 4, 2025

See the original comment with how to reproduce at AztecProtocol/aztec-packages#11294 (comment)

I spent some time debugging this today. I checked out master on aztec-packages, uncommented the panic!, but it doesn't occur any more with the following:

cargo run -q -p nargo_cli -- --program-dir ../../noir-projects/noir-contracts compile --package token_contract --silence-warnings --inliner-aggressiveness 0 --force

I checked out the original branch (available as af/sync-noir-11249 on aztec-packages), where I am still able to reproduce the issue. I used the following command to limit the SSA to the contract function where the panic occurs:

cargo run -q -p nargo_cli -- --program-dir ../../noir-projects/noir-contracts compile --package token_contract --silence-warnings --inliner-aggressiveness 0 --show-ssa --show-contract-fn setup_refund --force

I noticed by accident that if I use --show-ssa --show-contract-fn setup_refund on master I get no output at all, made me realise that the reason it doesn't fail on master is because the setup_refund method has been removed. So this bug is still relevant, probably.

Looking at the SSA I think the problem is with the defunctionalization. We start off having these methods:

After Initial SSA:
...
acir(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_sha256 f77 {
  b0(v45: Field, v46: Field, v47: u1):
    v50, v51 = call f109(v45, v46, v47, f110) -> ([u8; 16], [u8; 16])
    return v50, v51
}
acir(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret f109 {
  b0(v45: Field, v46: Field, v47: u1, v48: function):
...
brillig(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_sha256 f182 {
  b0(v45: Field, v46: Field, v47: u1):
    v50, v51 = call f196(v45, v46, v47, f197) -> ([u8; 16], [u8; 16])
    return v50, v51
}
brillig(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret f196 {
  b0(v45: Field, v46: Field, v47: u1, v48: function):
...
brillig(inline) fn extract_close_to_uniformly_random_256_bits_from_ecdh_shared_secret_using_sha256 f197 {
  b0(v45: Field, v46: Field, v47: u1):
...
acir(inline) fn extract_close_to_uniformly_random_256_bits_from_ecdh_shared_secret_using_sha256 f110 {
  b0(v45: Field, v46: Field, v47: u1):
...

See the originals here. So what we have derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_sha256, which calls derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret, passing it extract_close_to_uniformly_random_256_bits_from_ecdh_shared_secret_using_sha256 as a lambda, and we have an acir and brillig version of all the functions because of monomorphisation.

During defunctionalization we have a bug where the two extract_close_to_uniformly_random_256_bits_from_ecdh_shared_secret_using_sha256 variants end up being called by both the acir and the brillig versions of the apply function:

After Defunctionalization:
...
acir(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret f109 {
  b0(v45: Field, v46: Field, v47: u1, v48: Field):
    v51 = call f476(v48, v45, v46, v47) -> [u8; 32]
...
brillig(inline) fn derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret f196 {
  b0(v45: Field, v46: Field, v47: u1, v48: Field):
    v51 = call f477(v48, v45, v46, v47) -> [u8; 32]
...
acir(inline_always) fn apply f476 {
  b0(v45: Field, v46: Field, v47: Field, v48: u1):
    v51 = eq v45, Field 110
    jmpif v51 then: b2, else: b1
  b1():
    constrain v45 == Field 197
    v54 = call f197(v46, v47, v48) -> [u8; 32]
    jmp b3(v54)
  b2():
    v56 = call f110(v46, v47, v48) -> [u8; 32]
    jmp b3(v56)
  b3(v49: [u8; 32]):
    return v49
}
brillig(inline_always) fn apply f477 {
  b0(v45: Field, v46: Field, v47: Field, v48: u1):
    v51 = eq v45, Field 110
    jmpif v51 then: b2, else: b1
  b1():
    constrain v45 == Field 197
    v54 = call f197(v46, v47, v48) -> [u8; 32]
    jmp b3(v54)
  b2():
    v56 = call f110(v46, v47, v48) -> [u8; 32]
    jmp b3(v56)
  b3(v49: [u8; 32]):
    return v49
}

Both f476 and f477 call f110 and f197, which are the same function apart from having different runtimes. Whatever it is that is collecting the candidates of lambdas that can be passed to a function such as f196 is not taking into account the caller runtime. By now it will only be passed functions that match its runtime. At least that's what I think, although I'm not sure if an ACIR function can call a Brillig function passing a lambda that isn't yet monomorphised.

@aakoshh aakoshh linked a pull request Feb 4, 2025 that will close this issue
Draft
5 tasks
@michaeljklein michaeljklein mentioned this issue Feb 5, 2025
Open
@aakoshh aakoshh changed the title Sync temp fix 2: crash inserting air-only instruction into brillig runtime Sync temp fix 2: crash inserting acir-only instruction into brillig runtime Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Noir
Status: 📋 Backlog
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Try to turn acir lambdas into brillig noir-lang/noir
2 participants
@aakoshh @TomAFrench