Skip to content

feat: Noir sync rework#12574

Merged
aakoshh merged 59 commits intofeat/noir-one-way-syncfrom
noir-sync-rework
Mar 14, 2025
Merged

feat: Noir sync rework#12574
aakoshh merged 59 commits intofeat/noir-one-way-syncfrom
noir-sync-rework

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Mar 7, 2025

Check out Noir during build

Changes the way we sync the code from Noir away from having a periodically synced copy of the codebase, to checking out the code during the build based on a tag/branch/commit indicated by a file or env var.

The following new files were added:

  • noir/noir-repo-ref contains the reference in Noir that should be checked out
  • noir/scripts/sync.sh performs the checkouts; the init command does the initial checkout and the update command switches to new branches
  • noir/bootstrap.sh performs an init and update before every command except clean

TODO:

  • Sync during the build
  • Detect the need to switch branches
  • Create a patch file from local commits
  • Apply the patch file during checkouts
  • Fix patch creation to only include non-empty commits because git am stops at them, and with git am --allow-empty it just fails
  • Add to the git hooks something to prevent the user from switching branches without creating and committing a patch file, if there are commits in noir-repo. (Currently an error is returned by sync.sh but if we created a patch file, it should be committed on the aztec-packages branch we switched away from).\
  • Update the daily workflow to update noir-repo-ref to the next release.
  • Test the pull-noir workflow:

Rationale

Currently there is a two-way sync between Noir and aztec-packages, with automated workflows running daily to push changes between the master branches by opening (or appending to already open) sync PRs. The two-way sync was required because for example frequent changes in the bb interface were easier to update in this repo and pushed back to Noir, than to have to wait for a release cycle.

The problem we frequently encounter with the sync from Noir is that some of the integration tests in aztec-packages break due to subtle changes in the compiler, which we don't know about until we open the sync PR. At that point we're looking at a potentially long list of changes (every commit on master since the last sync), and it's not clear which one is the culprit. We bisect the build to find a failure, fix it in the PR, but this often just reveals the next bug to be fixed.

This process can take days, and during that time the sync process has to be paused - if it runs it just makes the PR bigger, with potentially more bugs. Regardless of whether we pause it or let it append to the PR, no new features or fixes get merged into aztec-packages until all the bugs are squashed.

Instead what we wish to achieve here is that we can:

  1. Point aztec-packages at an integration/release branch, or nightly tags to be synced from, and if there is a problem then we can cherry pick changes from master onto the release branch so that the good stuff can go in.
  2. Initiate CI runs against arbitrary branches on Noir, so that we can test smaller change sets before they are synced to aztec-packages, to see their effect in isolation.

Example

The following example shows:

  1. Initialising noir-repo to a tag
  2. Creating a file and commit
  3. Trying to switch branch is rejected because the commit is not in the patch
  4. Creating a patch
  5. Switching to a branch
  6. Creating another commit and adding it to the patch
  7. Switching to a commit
  8. See both commits are re-applied
% rm -rf noir-repo                                                                                                                                                                                 
% cat noir-repo-ref                                                                                                                                                                                
nightly-2025-03-07
% scripts/sync.sh init                                                                                                                                                                             
Cloning into 'noir-repo'...
...
Note: switching to '37be49fd081f33dc7256d23cee8ccc0719c50a82'.
...
HEAD is now at 37be49fd08 chore!: convert `TraitMethodNotInScope` to error (#7427)
➤ YN0000: ┌ Resolution step
➤ YN0002: │ @algolia/autocomplete-core@npm:1.9.3 doesn't provide @algolia/client-search (pbc410), requested by @algolia/autocomplete-shared
...
➤ YN0000: Done with warnings in 20s 227ms
[detached HEAD 6cee7cd4d6] Noir local fixup commit.
 4 files changed, 8 insertions(+), 9 deletions(-)
[detached HEAD 94cffa4cb1] Noir local patch commit.
% ./scripts/sync.sh info                                                         
Fixup commit:             30d08b921c71edb1c75491e459df6d54917c5e51
Checkout commit:          37be49fd081f33dc7256d23cee8ccc0719c50a82
Wanted:                   nightly-2025-03-07
Needs switch:             no
Needs patch:              no
Detached:                 yes
On branch:                no
Branch name:              n/a
Has wanted tag:           yes
Has tag commit:           yes
Has patch commit:         yes
Last commit is patch:     yes
Has fixup and patch:      yes
% cd noir-repo                                                                                                                                                                                  
% git status                                                                                                                                                                                    
Not currently on any branch.
nothing to commit, working tree clean
% echo "Hello" > hello.txt && git add . && git commit -m "Hello"                                                                                                                                
[detached HEAD 6053b5e04c] Hello
 1 file changed, 1 insertion(+)
 create mode 100644 hello.txt                                                                                                                                                                           
% cd ..                                                                                                                                                                                         
% NOIR_REPO_REF=master scripts/sync.sh update                                                                                                                                                      
noir-repo is on a detached HEAD and the last commit is not the patch marker commit;
switching from nightly-2025-03-07 to master could meand losing those commits.
Please use the 'make-patch' command to create a noir-repo.patch file and commit it in aztec-packages, 
so that it is applied after each checkout; make sure to commit the patch on the branch where it should be.
% scripts/sync.sh make-patch                                                                                                                                                                       
../patches/0001-Noir-local-patch-commit.patch
../patches/0002-Hello.patch
[detached HEAD 28f53e097b] Noir local patch commit.
% NOIR_REPO_REF=master scripts/sync.sh update                                                                                                                                                      
...
From https://github.com/noir-lang/noir
 * branch                  master     -> FETCH_HEAD
Warning: you are leaving 4 commits behind, not connected to
any of your branches:

  28f53e097b Noir local patch commit.
  6053b5e04c Hello
  94cffa4cb1 Noir local patch commit.
  6cee7cd4d6 Noir local fixup commit.

If you want to keep them by creating a new branch, this may be a good time
to do so with:

 git branch <new-branch-name> 28f53e097b

branch 'master' set up to track 'origin/master'.
Switched to a new branch 'master'
Already up to date.
➤ YN0000: ┌ Resolution step
➤ YN0002: │ @algolia/autocomplete-core@npm:1.9.3 doesn't provide @algolia/client-search (pbc410), requested by @algolia/autocomplete-shared
...
➤ YN0000: Done with warnings in 2s 686ms
[master 7cee0c05bb] Noir local fixup commit.
 4 files changed, 8 insertions(+), 9 deletions(-)
Applying: Hello
[master ad6812fae7] Noir local patch commit.
% cd noir-repo                                                                                                                                                                                  
% echo "World" > world.txt && git add . && git commit -m "World"                                                                                                                                    
[master 16f0054dc9] World
 1 file changed, 1 insertion(+)
 create mode 100644 world.txt
% cd ..                                                                                                                                                                                                                                                                                                                                                                  
% cat .noir-repo-last-ref                                                                                                                                                                          
master                                                                                                                                                          
% scripts/sync.sh make-patch                                                                                                                                                                       
../patches/0001-Hello.patch
../patches/0002-Noir-local-patch-commit.patch
../patches/0003-World.patch
[master 2cc18fd434] Noir local patch commit.
% cat noir-repo.patch                                                                                                                                                                              
From 6a8c19cd6fd70e4b982ec8ca10b4b70363636f97 Mon Sep 17 00:00:00 2001
From: aakoshh <akosh@aztecprotocol.com>
Date: Fri, 7 Mar 2025 23:46:36 +0000
Subject: [PATCH 1/3] Hello

---
 hello.txt | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 hello.txt

diff --git a/hello.txt b/hello.txt
new file mode 100644
index 0000000000..e965047ad7
--- /dev/null
+++ b/hello.txt
@@ -0,0 +1 @@
+Hello
-- 
9.43.0

From 16f0054dc961105cdb264d6ace10ff9061b7b96d Mon Sep 17 00:00:00 2001
From: aakoshh <akosh@aztecprotocol.com>
Date: Fri, 7 Mar 2025 23:49:19 +0000
Subject: [PATCH 3/3] World

---
 world.txt | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 world.txt

diff --git a/world.txt b/world.txt
new file mode 100644
index 0000000000..216e97ce08
--- /dev/null
+++ b/world.txt
@@ -0,0 +1 @@
+World
-- 
2.43.0

% export NOIR_REPO_REF=a9de769f3743733ac754a5d6c4e147ba8283a336                                                                                                                                    
% scripts/sync.sh update                                                                                                                                                                           
...
From https://github.com/noir-lang/noir
 * branch                  a9de769f3743733ac754a5d6c4e147ba8283a336 -> FETCH_HEAD
Note: switching to 'a9de769f3743733ac754a5d6c4e147ba8283a336'.
...

HEAD is now at a9de769f37 feat: add optional oracle resolver url in `acvm_cli` (#7630)
➤ YN0000: ┌ Resolution step
...
➤ YN0000: └ Completed in 1s 219ms
➤ YN0000: Done with warnings in 2s 643ms
[detached HEAD 53cbe7bae4] Noir local fixup commit.
 4 files changed, 8 insertions(+), 9 deletions(-)
Applying: Hello
Applying: World
[detached HEAD c4921bc4a8] Noir local patch commit.
% ls noir-repo/*.txt                                                                                                                                                                           16s 
noir-repo/hello.txt  noir-repo/world.txt
%                                                                                                                                                                                                  

@aakoshh aakoshh changed the title Noir sync rework feat(DRAFT): Noir sync rework Mar 7, 2025
@TomAFrench
Copy link
Member

TomAFrench commented Mar 7, 2025

We were pulling the noir version to use from the noir repository but now we don't have the noir repo downloaded during ./bootstrap.sh check

https://github.com/AztecProtocol/aztec-packages/actions/runs/13723701533/job/38384985929?pr=12574#step:6:42

Edit: link isn't working nicely so I'm referring to the line WARN: Rust is not installed. Performance will be degraded.

We can fix this by pulling from the avm-transpiler directory instead.

@TomAFrench
Copy link
Member

Create a patch file from local commits

I think we need a patch which zeroes out the version reported by nargo --version. We keep getting people reporting bugs which are due to using nargo from this repository and reporting issues which are just down to them having busted installs.

@aakoshh
Copy link
Contributor Author

aakoshh commented Mar 7, 2025

I think we need a patch which zeroes out the version reported by nargo --version.

You mean that nargo in this repo should be distinguished from nargo in noir, and to do that we'd do something in scripts/sync-in-fixup.sh to edit the the source, like it does for some files with sed.

Perhaps instead of zeroing it out, it could change const NARGO_VERSION: &str = env!("CARGO_PKG_VERSION"); to something like const NARGO_VERSION: &str = concat!(env!("CARGO_PKG_VERSION"), "-aztec");, so we still know what it is.

Although, if we cloned the code and they ran it, I'd be curious why the results are different than if they build it over there.

Create a patch file from local commits

On a different note, the patch file would contain non-automated changes made by aztec devs, stuff we want to actually migrate back eventually. I'm trying to figure out how to distinguish them from the fixup so they don't get mixed up.

@aakoshh
Copy link
Contributor Author

aakoshh commented Mar 7, 2025

Although, if we cloned the code and they ran it, I'd be curious why the results are different than if they build it over there.

Ah, obviously the patch file itself could do any number of changes 🤦

@aakoshh
Copy link
Contributor Author

aakoshh commented Mar 7, 2025

It looks like CI doesn't like my script echo-ing messages:

16:26:07 FAILED (http://ci.aztec-labs.com/07d7e9739057a2d0): already on nightly-2025-03-07 (0s) (code: 127)
16:26:07 Command: noir-repo already on nightly-2025-03-07
16:26:07 Commit: https://github.com/AztecProtocol/aztec-packages/commit/04c40f1b9d50b96a2740881737af6e1998f5bafb
16:26:07 Env: REF_NAME=noir-sync-rework CURRENT_VERSION=0.78.1 CI_FULL=0
16:26:07 Date: Fri Mar  7 16:22:16 UTC 2025
16:26:07 Sys: ARCH=amd64 CPUS=128 MEM=493Gi HOSTNAME=noir-sync-rework_amd64
16:26:07 
16:26:07 16:22:16 bash: line 1: already: command not found

Ah, now I see what test_cmds is for 💡

@aakoshh aakoshh requested review from TomAFrench and ludamad and removed request for TomAFrench March 8, 2025 00:04
@aakoshh aakoshh changed the title feat(DRAFT): Noir sync rework feat: Noir sync rework Mar 8, 2025
@ludamad ludamad changed the base branch from master to feat/noir-one-way-sync March 8, 2025 00:45
@aakoshh aakoshh force-pushed the noir-sync-rework branch from d288f0c to fe83b1a Compare March 8, 2025 11:26
@aakoshh aakoshh changed the base branch from feat/noir-one-way-sync to af/remove-noir-repo March 8, 2025 11:27
@aakoshh aakoshh changed the base branch from af/remove-noir-repo to feat/noir-one-way-sync March 8, 2025 11:31
@aakoshh aakoshh requested a review from ludamad March 10, 2025 16:04
@ludamad
Copy link
Collaborator

ludamad commented Mar 10, 2025

Good with me. @charlielye thoughts on this PR? (though I'd hope not questioning the whole concept, at this point :))

@TomAFrench
Copy link
Member

I tried this out and ran into an issue with how it handles switching branches. This can be reproed by the steps.

  1. start on noir-sync-rework and bootstrap.
  2. git checkout master
  3. At this point noir/noir-repo is cleared except for all the build directories and node_modules folders.
  4. git checkout noir-sync-rework
  5. ./bootstrap.sh fast

This causes bootstrap.sh to skip checking out the noir repo and so the build fails with the error message below.

--- noir build ---
Executing: ./noir-repo/.github/scripts/wasm-bindgen-install.sh (http://ci.aztec-labs.com/e04111002cf3b49d)
   0 .
Command exited with status 127. Dumping output:
bash: line 1: ./noir-repo/.github/scripts/wasm-bindgen-install.sh: No such file or directory
. failed (0s) (http://ci.aztec-labs.com/e04111002cf3b49d)

@aakoshh
Copy link
Contributor Author

aakoshh commented Mar 11, 2025

@TomAFrench I can replicate what you got by doing the following:

  1. Switch to master restores all the files in noir-repo, and on my end kicks in the Rust Analyzer as well
  2. Switch to noir-sync-rework removes the files which were deleted as part of the future, but doesn't touch the target directory which was always ignored
  3. Now the init command thinks it has nothing to do and then the build fails because the actual code isn't there

I changed scripts/sync.sh init to re-initialise the git repository if noir-repo/.git doesn't exist and switch to the desired reference. This should preserve the target and node_modules and whatever other uncommitted files the user might have in noir-repo.

If there would be conflicts then a full ./bootstrap.sh clean can clear up everything.

aakoshh and others added 3 commits March 11, 2025 10:53
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@TomAFrench
Copy link
Member

TomAFrench commented Mar 11, 2025

Ah sorry, fake news on my part. We should revert those changes. I misread the suggestion where -c was applied to git switch.


# Update the noir-repo before we hash its content, unless the command is exempt.
no_update=(clean make-patch bump-noir-repo-ref)
if [[ -z "$cmd" || ! ${no_update[*]} =~ "$cmd" ]]; then
Copy link
Collaborator

@ludamad ludamad Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just hash the ref and the patch file? (tho we should still disable the cache if there's any local changes to noir repo, hmm)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to fundamentally change what the hashes work with: have we compiled and cached this code or not, including pending changes and local commits that may not be in the hash file.

@@ -1,4 +1,4 @@
# Create a pull request from current Noir master.
# Create a pull request from current Noir nightly.
Copy link
Collaborator

@ludamad ludamad Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know if you guys still want a workflow to make PRs into noir with the patch, but otherwise we can delete the other side of the sync noir workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought opening a PR could involve some mechanical steps, such as rebasing on the remote branch without the fixup and patch marker commit, which could benefit from automation, even if it's just manually triggered, but we should definitely disable any automation for now. I thought we can return to this question once we actually have a situation and understand better what's a nice way to do it. @TomAFrench ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with doing this manually in the short term and automating it later.

echo "./yarn-project/constants/precommit.sh" >>$hooks_dir/pre-commit
chmod +x $hooks_dir/pre-commit
echo "(cd noir && ./postcheckout.sh $@)" >$hooks_dir/post-checkout
chmod +x $hooks_dir/post-checkout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do this right after checking out submodules? As this is basically "getting a submodule" our own way.
Also would mean going to the transpiler for the version above not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually check out the noir-repo, it does not initialise it, it just checks whether it has commits that should be patched. (I just pushed a fix to make sure it works correctly if the repo doesn't exist yet).

The other difference I think is that if I put it after the update of submodules, it would print this message whenever the developer runs a build and there are commits in the noir-repo. I don't want that, because they might just be running builds on the same branch over and over; what I want is to warn them iff they switch away to a different aztec-packages branch, before they initiate a build on the new branch, which could end up checking out a different branch/tag/commit of noir-repo for them. Before they do so they should think about switching back to where they came from, create a patch there and commit it. If it's okay, then until they switch branches again they can work in peace.

@aakoshh aakoshh merged commit 2b555ce into feat/noir-one-way-sync Mar 14, 2025
4 checks passed
@aakoshh aakoshh deleted the noir-sync-rework branch March 14, 2025 08:50
TomAFrench added a commit that referenced this pull request Mar 14, 2025
Contains the changes from
#12574 to replace
the copy of the noir-repo with a `noir-repo-ref` file to point at the
nightly version which we pull during the build.

Updated `noir-repo-ref` in the PR to match the last merge sync PR
#12624
Alternatively it could be set to include the commits from the open
#12669 by setting it
to
[nightly-2025-03-14](https://github.com/noir-lang/noir/commits/nightly-2025-03-14)

Added a patch to replicate the changes to honk test programs in
#12266
Opened noir-lang/noir#7706 to migrate the patch
back to Noir

---------

Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: aakoshh <akosh@aztecprotocol.com>
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Mar 15, 2025
Contains the changes from
AztecProtocol/aztec-packages#12574 to replace
the copy of the noir-repo with a `noir-repo-ref` file to point at the
nightly version which we pull during the build.

Updated `noir-repo-ref` in the PR to match the last merge sync PR
AztecProtocol/aztec-packages#12624
Alternatively it could be set to include the commits from the open
AztecProtocol/aztec-packages#12669 by setting it
to
[nightly-2025-03-14](https://github.com/noir-lang/noir/commits/nightly-2025-03-14)

Added a patch to replicate the changes to honk test programs in
AztecProtocol/aztec-packages#12266
Opened noir-lang/noir#7706 to migrate the patch
back to Noir

---------

Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: aakoshh <akosh@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants