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

Add some Rust smoketesting CI for barretenberg_wrapper #14

Merged
merged 6 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
name: Rust

on: [push, pull_request]
Copy link
Member

@TomAFrench TomAFrench Jan 11, 2023

Choose a reason for hiding this comment

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

Suggested change
on: [push, pull_request]
on:
push:
branches: 'kw/noir-dsl'
pull_request:
branches: '*'

This currently runs CI twice on PRs.

Copy link
Author

@phated phated Jan 11, 2023

Choose a reason for hiding this comment

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

Running the PR twice is actually good because a push runs with the branch as it's own base and pull_request runs with the upstream as the base.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow on this. When you say "with the upstream as the base" do you mean "as if the PR were rebased on top of the branch being merged into"? I haven't heard anything about push and pull_request treating the same commit differently.

Copy link
Author

Choose a reason for hiding this comment

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

As per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

Thus, the checkout action running on something triggered by pull_request will checkout a merge between the upstream and the PR and test it. And push just checks out to the commit that is pushed.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting. I didn't know that GH did that temporary merge for pull_request.

Personally I generally try to avoid merging branches into a base branch which is ahead of it (e.g. through first merging back into the PR branch) to check this explicitly rather than relying on GH.


env:
CARGO_TERM_COLOR: always

jobs:
check_n_test:
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os:
- ubuntu-latest
- macos-latest
toolchain:
- stable
- nightly
steps:
- name: Checkout sources
uses: actions/checkout@v3

- name: Install toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: ${{ matrix.toolchain }}
override: true

- name: Install dependencies
if: ${{ matrix.os == 'ubuntu-latest' }}
run: sudo apt update && sudo apt install libomp-dev

- name: Install dependencies
if: ${{ matrix.os == 'macos-latest' }}
run: brew install llvm libomp cmake

- name: Run cargo check
working-directory: "./barretenberg_wrapper"
run: |
cargo +${{ matrix.toolchain }} check --all-targets --verbose

- name: Run cargo test
working-directory: "./barretenberg_wrapper"
run: |
cargo +${{ matrix.toolchain }} test

clippy:
name: cargo clippy
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v3

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
components: rustfmt, clippy

- name: Install dependencies
run: sudo apt update && sudo apt install libomp-dev

- name: Run cargo clippy
working-directory: "./barretenberg_wrapper"
run: |
cargo clippy -- -D warnings

format:
name: cargo fmt
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v3

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
components: rustfmt, clippy

- name: Install dependencies
run: sudo apt update && sudo apt install libomp-dev

- name: Run cargo fmt
working-directory: "./barretenberg_wrapper"
run: |
cargo fmt --all -- --check
6 changes: 6 additions & 0 deletions barretenberg_wrapper/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This ensures Clang (instead of GCC) is used as a linker on Linux.
# See https://github.com/rust-lang/rust/issues/71515#issuecomment-935020057
# These should apply when used as a library, as per
# https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure
[target.x86_64-unknown-linux-gnu]
Copy link
Author

@phated phated Jan 11, 2023

Choose a reason for hiding this comment

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

I believe we'll want to add something similar for aarch64-unknown-linux-gnu in PR #8

rustflags = ["-C", "linker=clang", "-C", "link-arg=-fuse-ld=lld"]
6 changes: 3 additions & 3 deletions barretenberg_wrapper/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ fn link_lib_omp(toolchain: &'static str) {
INTEL_APPLE => {
let brew_prefix = find_brew_prefix();
println!("cargo:rustc-link-search={}/opt/libomp/lib", brew_prefix)
},
}
ARM_APPLE => {
let brew_prefix = find_brew_prefix();
println!("cargo:rustc-link-search={}/opt/libomp/lib", brew_prefix)
Expand All @@ -229,12 +229,12 @@ fn link_lib_omp(toolchain: &'static str) {
match toolchain {
ARM_LINUX => {
// only arm linux uses gcc
println!("cargo:rustc-link-lib=gomp")
println!("cargo:rustc-link-lib=gomp")
}
INTEL_APPLE | ARM_APPLE => {
println!("cargo:rustc-link-lib=omp")
}
&_ => println!("cargo:rustc-link-lib=omp5")
&_ => println!("cargo:rustc-link-lib=omp5"),
}
}

Expand Down
4 changes: 3 additions & 1 deletion barretenberg_wrapper/src/composer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ pub unsafe fn get_circuit_size(cs_prt: *const u8) -> u32 {
// TODO test with a circuit of size 2^19 cf: https://github.com/noir-lang/noir/issues/12
}

/// # Safety
/// cs_prt must point to a valid constraints system structure of type standard_format
phated marked this conversation as resolved.
Show resolved Hide resolved
pub unsafe fn get_exact_circuit_size(cs_prt: *const u8) -> u32 {
standard_example__get_exact_circuit_size(cs_prt)
}
Expand Down Expand Up @@ -55,7 +57,7 @@ pub unsafe fn verify(
g2_ptr: &[u8],
) -> bool {
let proof_ptr = proof.as_ptr() as *const u8;

composer__verify_proof(
pippenger,
g2_ptr.as_ptr() as *const u8,
Expand Down
14 changes: 7 additions & 7 deletions barretenberg_wrapper/src/pedersen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub fn compress_many(inputs: &[[u8; 32]]) -> [u8; 32] {
let mut buffer = Vec::new();
let witness_len = inputs.len() as u32;
buffer.extend_from_slice(&witness_len.to_be_bytes());
for assignment in &*inputs {
for assignment in inputs {
phated marked this conversation as resolved.
Show resolved Hide resolved
buffer.extend_from_slice(assignment);
}

Expand All @@ -40,7 +40,7 @@ pub fn encrypt(inputs_buffer: &[[u8; 32]]) -> ([u8; 32], [u8; 32]) {
let buffer_len = inputs_buffer.len() as u32;
let mut result = [0_u8; 64];
buffer.extend_from_slice(&buffer_len.to_be_bytes());
for e in &*inputs_buffer {
for e in inputs_buffer {
phated marked this conversation as resolved.
Show resolved Hide resolved
buffer.extend_from_slice(e);
}

Expand Down Expand Up @@ -72,17 +72,17 @@ mod tests {
Test {
input_left: f_zero,
input_right: f_one,
expected_hex: "108800e84e0f1dafb9fdf2e4b5b311fd59b8b08eaf899634c59cc985b490234b",
expected_hex: "229fb88be21cec523e9223a21324f2e305aea8bff9cdbcb3d0c6bba384666ea1",
},
Test {
input_left: f_one,
input_right: f_one,
expected_hex: "00f1c7ea35a4cf7ea5e678fcc2a5fac5351a563a3ff021f0c4a4126462aa081f",
expected_hex: "26425ddf29b4af6ee91008e8dbcbee975653170eee849efd75abf8301dee114e",
},
Test {
input_left: f_one,
input_right: f_zero,
expected_hex: "2619a3512420b4d3c72e43fdadff5f5a3ec1b0e7d75cd1482159a7e21f6c6d6a",
expected_hex: "08f3cb4f0fdd7a9ef130c6d4590af6750b1475161020a198a56eced45078ccf2",
},
];

Expand All @@ -107,8 +107,8 @@ mod tests {
inputs.push(f_one);

let (x, y) = encrypt(&inputs);
let expected_x = "108800e84e0f1dafb9fdf2e4b5b311fd59b8b08eaf899634c59cc985b490234b";
let expected_y = "2d43ef68df82e0adf74fed92b1bc950670b9806afcfbcda08bb5baa6497bdf14";
let expected_x = "229fb88be21cec523e9223a21324f2e305aea8bff9cdbcb3d0c6bba384666ea1";
let expected_y = "296b4b4605e586a91caa3202baad557628a8c56d0a1d6dff1a7ca35aed3029d5";
assert_eq!(expected_x, hex::encode(x));
assert_eq!(expected_y, hex::encode(y));
}
Expand Down