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

feat(ssa): Implement missing brillig constraints SSA check #6658

Merged
merged 97 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
97 commits
Select commit Hold shift + click to select a range
d744b60
WIP on value id parents
rkarabut Oct 30, 2024
5fbf961
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Oct 31, 2024
8e711ec
Build the actual dependency graph, record constrains/brilligs
rkarabut Oct 31, 2024
1ac3e3f
Check for brillig call constrain coverage
rkarabut Nov 1, 2024
e848be7
cargo fmt, clippy
rkarabut Nov 1, 2024
d741525
Move tracing-test to dev-dependencies
rkarabut Nov 1, 2024
49b59c6
Collect SSA reports
rkarabut Nov 4, 2024
d33dc07
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Nov 4, 2024
7e6bf9b
Remove instruction catch-all
rkarabut Nov 5, 2024
b418169
Consider brillig call covered if _all_ the results are constrained
rkarabut Nov 5, 2024
ea84f59
Rework the 5425 reduced test case
rkarabut Nov 6, 2024
0a6a613
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Nov 6, 2024
835571b
Add 5425 test into test_programs
rkarabut Nov 7, 2024
f3e50ec
Add missing brillig constrain check into ssa.rs
rkarabut Nov 7, 2024
0b1bcf8
Add skip check compiler option
rkarabut Nov 8, 2024
93eb194
nargo fmt test program
rkarabut Nov 12, 2024
b70a4db
Add multiple results brillig check, fix ancestor walk
rkarabut Nov 12, 2024
63b9da9
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Nov 13, 2024
a796e2e
Clean up
rkarabut Nov 13, 2024
1dfb9b2
Fix possible parent loops, skip covered brillig calls
rkarabut Nov 13, 2024
a5b7aa8
Optimize
rkarabut Nov 14, 2024
c9635f5
Reverse order of checking constrains
rkarabut Nov 14, 2024
2c42c51
WIP
rkarabut Nov 15, 2024
5d7520f
Process more cases
rkarabut Nov 15, 2024
610007d
Resolve all value ids
rkarabut Nov 15, 2024
2578958
Restore proper 5425 main
rkarabut Nov 15, 2024
cf031bb
Don't check brillig arguments intersection if there are none
rkarabut Nov 15, 2024
632ae24
Clean up
rkarabut Nov 18, 2024
29db10a
Add constant brillig call test, fix comments
rkarabut Nov 18, 2024
b125bd5
Add range checked brillig call test
rkarabut Nov 18, 2024
94d4ba8
Get Cargo.lock from master
rkarabut Nov 18, 2024
492b7b8
Merge branch 'directed-graph-ssa-check', rework algorithm
rkarabut Nov 19, 2024
9c66a91
Avoid allocating extra vectors
rkarabut Nov 21, 2024
3d2fc2b
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Nov 25, 2024
caf8b0d
Update match
rkarabut Nov 25, 2024
d59db94
Merge remote-tracking branch 'origin/master' into directed-graph-ssa-…
rkarabut Nov 29, 2024
75e5cc0
Fix typos, improve comments
rkarabut Nov 29, 2024
2c5bccd
Update Cargo.lock
rkarabut Nov 29, 2024
241ed61
Update the check to comply with the check_unconstrained_regression test
rkarabut Nov 30, 2024
ca83b0f
Merge remote-tracking branch 'origin/master' into rk/directed-graph-s…
rkarabut Nov 30, 2024
d86cfa2
Capitalize Brillig in comments to match the style
rkarabut Nov 30, 2024
6b4b719
Capitalize Brillig
rkarabut Nov 30, 2024
5429962
Fix array get handling
rkarabut Nov 30, 2024
c2c8b9a
Fix/optimize partial constraint aggregation
rkarabut Nov 30, 2024
5fb7f5b
Optimize
rkarabut Nov 30, 2024
cef0f9a
Optimize, skip over instructions preceding Brillig calls
rkarabut Nov 30, 2024
0a75c0a
Skip the check if there are no Brillig functions
rkarabut Dec 1, 2024
a55b1bc
Fix typos
rkarabut Dec 1, 2024
73e6ce2
Attempt lowering recursion limit
rkarabut Dec 2, 2024
fecb7ac
Update test
rkarabut Dec 2, 2024
4c20131
Clear the taint record of Brillig results found constrained
rkarabut Dec 2, 2024
e85ca35
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 3, 2024
05d4499
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 3, 2024
bfc0e22
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 3, 2024
3c5a92c
Update compiler/noirc_evaluator/src/errors.rs
rkarabut Dec 4, 2024
78aac76
Update compiler/noirc_evaluator/src/ssa/checks/check_for_underconstra…
rkarabut Dec 4, 2024
e38f19d
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 4, 2024
ebf7dc2
Change debug! to panic
rkarabut Dec 4, 2024
acbb10a
Shorten the skip option name
rkarabut Dec 4, 2024
a44828b
Fix redundant function id conversions
rkarabut Dec 4, 2024
06e60b5
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 4, 2024
6e2cd9e
Update TODO
rkarabut Dec 4, 2024
5cf36a2
Rework tests to use the new SSA loader
rkarabut Dec 4, 2024
1b29283
Experiment: disable check by default to test unconditional_recursion …
rkarabut Dec 5, 2024
4dfcf8f
Experiment: dial RECURSION_LIMIT back to 1000 to see if
rkarabut Dec 5, 2024
5b4fd91
Revert experiment
rkarabut Dec 5, 2024
2a30386
Add nested type result brillig test case
rkarabut Dec 5, 2024
19b3c97
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 5, 2024
f798585
Update Cargo.lock
rkarabut Dec 5, 2024
d86891b
Update Cargo.lock
rkarabut Dec 5, 2024
ca1fc4f
Update comments, bring in unsafe{} for the 5425 test case
rkarabut Dec 5, 2024
5f1f8da
Enable the compile_success_with_bug test type
rkarabut Dec 5, 2024
b070c6e
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 5, 2024
55f991d
Upgrade pprof to fix advisories on CI
rkarabut Dec 5, 2024
4a63ba6
Rewrite ResultStatus as an enum
rkarabut Dec 5, 2024
4e6cc07
Update compiler/noirc_evaluator/src/ssa/checks/check_for_underconstra…
rkarabut Dec 5, 2024
61796c8
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 5, 2024
4c69ed6
Remove unneeded resolve
rkarabut Dec 5, 2024
dc12963
Move arguments/results allocation inside the loop
rkarabut Dec 5, 2024
e4c7bde
Fix Brillig encountered check
rkarabut Dec 5, 2024
4661ee5
Update compiler/noirc_evaluator/src/ssa/checks/check_for_underconstra…
rkarabut Dec 5, 2024
82e4431
Clear up partial constraint comments
rkarabut Dec 5, 2024
6b068c5
Remove pre-Brillig call skip (see
rkarabut Dec 5, 2024
8d3dd70
Switch the recursion issue solution
rkarabut Dec 5, 2024
c836758
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 5, 2024
29f8e0c
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 5, 2024
798947c
Rework update_children to remove the parent loop
rkarabut Dec 9, 2024
1451a03
Fix the false negative root result intersection check case
rkarabut Dec 9, 2024
bc440a3
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 9, 2024
7b6ba19
Update warning
rkarabut Dec 9, 2024
ea7fbc2
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 9, 2024
9e76cef
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 9, 2024
3c20db7
Merge branch 'master' into rk/directed-graph-ssa-check
TomAFrench Dec 9, 2024
cd37a3d
Update instruction set
rkarabut Dec 9, 2024
344342f
Fix build
rkarabut Dec 10, 2024
4034a1d
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 10, 2024
329a131
Merge branch 'master' into rk/directed-graph-ssa-check
rkarabut Dec 11, 2024
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
1 change: 1 addition & 0 deletions .github/workflows/test-rust-workspace-msrv.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ jobs:
name: nextest-archive
- name: Run tests
run: |
RUST_MIN_STACK=8388608 \
cargo nextest run --archive-file nextest-archive.tar.zst \
--partition count:${{ matrix.partition }}/4 \
--no-fail-fast
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/test-rust-workspace.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ jobs:
name: nextest-archive
- name: Run tests
run: |
RUST_MIN_STACK=8388608 \
cargo nextest run --archive-file nextest-archive.tar.zst \
--partition count:${{ matrix.partition }}/4 \
--no-fail-fast
Expand Down
24 changes: 23 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ pub struct CompileOptions {
#[arg(long)]
pub skip_underconstrained_check: bool,

/// Flag to turn off the compiler check for missing Brillig call constrains.
/// Warning: This can improve compilation speed but can also lead to correctness errors.
/// This check should always be run on production code.
#[arg(long)]
pub skip_brillig_constraints_check: bool,

/// Setting to decide on an inlining strategy for Brillig functions.
/// A more aggressive inliner should generate larger programs but more optimized
/// A less aggressive inliner should generate smaller programs
Expand Down Expand Up @@ -631,6 +637,7 @@ pub fn compile_no_check(
},
emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None },
skip_underconstrained_check: options.skip_underconstrained_check,
skip_brillig_constraints_check: options.skip_brillig_constraints_check,
inliner_aggressiveness: options.inliner_aggressiveness,
max_bytecode_increase_percent: options.max_bytecode_increase_percent,
};
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ cfg-if.workspace = true
[dev-dependencies]
proptest.workspace = true
similar-asserts.workspace = true
tracing-test = "0.2.5"
num-traits.workspace = true
test-case.workspace = true

Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,10 @@ impl From<SsaReport> for FileDiagnostic {
let message = bug.to_string();
let (secondary_message, call_stack) = match bug {
InternalBug::IndependentSubgraph { call_stack } => {
("There is no path from the output of this brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(),call_stack)
("There is no path from the output of this Brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(), call_stack)
}
InternalBug::UncheckedBrilligCall { call_stack } => {
("This Brillig call's inputs and its return values haven't been sufficiently constrained. This should be done to prevent potential soundness vulnerabilities".to_string(), call_stack)
}
InternalBug::AssertFailed { call_stack } => ("As a result, the compiled circuit is ensured to fail. Other assertions may also fail during execution".to_string(), call_stack)
};
Expand All @@ -117,8 +120,10 @@ pub enum InternalWarning {

#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize, Hash)]
pub enum InternalBug {
#[error("Input to brillig function is in a separate subgraph to output")]
#[error("Input to Brillig function is in a separate subgraph to output")]
IndependentSubgraph { call_stack: CallStack },
#[error("Brillig function call isn't properly covered by a manual constraint")]
UncheckedBrilligCall { call_stack: CallStack },
#[error("Assertion is always false")]
AssertFailed { call_stack: CallStack },
}
Expand Down
27 changes: 20 additions & 7 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@ pub struct SsaEvaluatorOptions {
/// Skip the check for under constrained values
pub skip_underconstrained_check: bool,

/// The higher the value, the more inlined brillig functions will be.
/// Skip the missing Brillig call constraints check
pub skip_brillig_constraints_check: bool,

/// The higher the value, the more inlined Brillig functions will be.
pub inliner_aggressiveness: i64,

/// Maximum accepted percentage increase in the Brillig bytecode size after unrolling loops.
Expand Down Expand Up @@ -104,12 +107,22 @@ pub(crate) fn optimize_into_acir(

let mut ssa = optimize_all(builder, options)?;

let ssa_level_warnings = if options.skip_underconstrained_check {
vec![]
} else {
time("After Check for Underconstrained Values", options.print_codegen_timings, || {
ssa.check_for_underconstrained_values()
})
let mut ssa_level_warnings = vec![];

if !options.skip_underconstrained_check {
ssa_level_warnings.extend(time(
"After Check for Underconstrained Values",
options.print_codegen_timings,
|| ssa.check_for_underconstrained_values(),
));
}

if !options.skip_brillig_constraints_check {
ssa_level_warnings.extend(time(
"After Check for Missing Brillig Call Constraints",
options.print_codegen_timings,
|| ssa.check_for_missing_brillig_constraints(),
));
};

drop(ssa_gen_span_guard);
Expand Down
Loading
Loading