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

Cross-block deduplication in constant folding has correctness bugs. #6588

Closed
TomAFrench opened this issue Nov 21, 2024 · 6 comments · Fixed by #6627
Closed

Cross-block deduplication in constant folding has correctness bugs. #6588

TomAFrench opened this issue Nov 21, 2024 · 6 comments · Fixed by #6627
Assignees

Comments

@TomAFrench
Copy link
Member

One of the components which has been causing issues on Noir syncs has been the changes made in #6499 so it's had to be pulled out of the syncs.
I opened this PR to test adding it in isolation and there's a number of test failures.

Sadly all of these tests rely on foreign calls so it's going to be a bit messy to get a nice reproduction case but we should find it and bring it back into this repo to fix the underlying issue.

@asterite
Copy link
Collaborator

So far what I found is that the value of value_change_hint here:

https://github.com/AztecProtocol/aztec-packages/blob/a9f3b5f6e7e5bc9d4bc9c0600b492a5e0cd2c1d9/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr#L205

changes from:

ScheduledValueChange { pre: 0x00, post: 0x11, block_of_change: 33 }

to

ScheduledValueChange { pre: 0x00, post: 0x00, block_of_change: 0 }

after the entire if hash != 0 { has passed.

If I comment out the else of that hash, it works.

Then what I found is that if I change the value we compare here:

https://github.com/AztecProtocol/aztec-packages/blob/a9f3b5f6e7e5bc9d4bc9c0600b492a5e0cd2c1d9/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr#L221-L225

to something else, for example:

            assert_eq(
                value_change_hint,
                ScheduledValueChange::deserialize([2, 4, 8]),
                "Non-zero value change for zero hash",
            );

then after the if the value changes to this:

ScheduledValueChange { pre: 0x02, post: 0x04, block_of_change: 8 }

So it's clear that that assert_eq is somehow mutating the value of value_change_hint, though I don't understand why yet.

I'll try to reproduce the above scenario with smaller code and without oracles.

@asterite
Copy link
Collaborator

asterite commented Nov 25, 2024

I forgot to mention that the else block of that if isn't actually executed, though the value changes after the entire if, which makes things even stranger.

@TomAFrench
Copy link
Member Author

That's interesting, I remember we had a issue with with if else blocks behaving in a similar fashion in the value merger PR it might be worth having a look at that

@TomAFrench
Copy link
Member Author

TomAFrench commented Nov 25, 2024

See #6073 Ah ignore this, this test failure is unrelated.

@TomAFrench
Copy link
Member Author

I was misremembering, it was this PR which removes negations from jmpif conditions: #5891

@asterite
Copy link
Collaborator

We have a couple of PRs that fix this: #6627 and #6628 (they are similar but we'll probably go with #6627 because the code is more readable).

That said, I tried those fixes in the Aztec-Packages PRs AztecProtocol/aztec-packages#9972 and AztecProtocol/aztec-packages#10152 and they still fail CI... but maybe more fixes have been pushed to Noir master that, together with the fixes here, might make things work?

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
2 participants