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

fix: always check index out of bounds for arrays and slices #5676

Closed
wants to merge 10 commits into from

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Aug 2, 2024

Description

Problem

Resolves #5296

Summary

Always insert an index out of bounds check when accessing or modifying an array or a slice.

Additional Context

None.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite changed the title Ab/index out of bounds checks fix: always check index out of bounds for arrays and slices Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Changes to circuit sizes

Generated at commit: 819490a00e5cb4de47a754b86131456f26bdeeb9, compared to commit: ade69a9e5f1546249e9b43b40e9ff0da87c4632e

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
brillig_unitialised_arrays +8 ❌ +160.00% +2,746 ❌ +16152.94%
array_dynamic_main_output +4 ❌ +12.12% +2,742 ❌ +1603.51%
array_to_slice +8 ❌ +12.50% +2,746 ❌ +1044.11%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
brillig_unitialised_arrays 13 (+8) +160.00% 2,763 (+2,746) +16152.94%
array_dynamic_main_output 37 (+4) +12.12% 2,913 (+2,742) +1603.51%
array_to_slice 72 (+8) +12.50% 3,009 (+2,746) +1044.11%
regression_5252 82,071 (+285) +0.35% 109,800 (+3,009) +2.82%
hashmap 209,877 (+3,943) +1.91% 370,935 (+3,904) +1.06%
array_dynamic 140 (+33) +30.84% 3,717 (+31) +0.84%
array_sort 79 (+24) +43.64% 2,882 (+22) +0.77%
nested_array_dynamic 4,193 (+34) +0.82% 13,789 (+33) +0.24%
regression_capacity_tracker 125 (+10) +8.70% 3,937 (+9) +0.23%
regression_mem_op_predicate 63 (+5) +8.62% 3,568 (+5) +0.14%
regression_struct_array_conditional 76 (+4) +5.56% 3,185 (+4) +0.13%
slice_dynamic_index 1,206 (+6) +0.50% 6,426 (+6) +0.09%
array_dynamic_blackbox_input 1,331 (+512) +62.52% 45,970 (0) 0.00%
array_dynamic_nested_blackbox_input 173 (+4) +2.37% 38,799 (0) 0.00%
conditional_1 7,865 (-680) -7.96% 38,799 (0) 0.00%
nested_array_dynamic_simple 1 (-15) -93.75% 6 (-31) -83.78%

@asterite
Copy link
Collaborator Author

asterite commented Aug 2, 2024

Circuit sizes went up, as expected. In the ones mostly affected I think that's expected. For example array_dynamic_main_output is setting an array value with an unknown index. Previously there was no bounds check and now there is.

But in some cases there are checks that could be removed. In array_sort I can find this:

    v22 = lt v4, u32 3
    jmpif v22 then: b2, else: b3
  b2():
    v24 = lt v4, u32 3
    constrain v24 == u1 1 '"Index out of bounds"'
    v25 = array_get v0, index v4

It's checking the < 3 condition twice. I don't know if that's not optimized away because that's inside an unconstrained function, or because that optimization doesn't exist yet.

@asterite asterite requested a review from a team August 2, 2024 17:33
@asterite
Copy link
Collaborator Author

asterite commented Aug 2, 2024

It's also interesting what happened with nested_array_dynamic_simple:

fn main(x: Field) {
    // x = 3
    let array: [[(Field, [Field; 1], [Field; 1]); 1]; 1] = [[(1, [2], [3])]];

    let fetched_value = array[x - 3];
    assert(fetched_value[0].0 == 1);
    assert(fetched_value[0].1[0] == 2);
    assert(fetched_value[0].2[0] == 3);
}

the compiler notices that the array only has one value, so the assert checks will always succeed as long as x - 3 is zero. The resulting program (in this PR) is this one:

acir(inline) fn main f0 {
  b0(v0: Field):
    v47 = sub v0, Field 3
    v48 = cast v47 as u32
    constrain v48 == u32 0 '"Index out of bounds"'
    return
}

The old program was much longer... I don't know if constrain enables some further optimizations.

@asterite
Copy link
Collaborator Author

Closing because we chose #5691 instead.

@asterite asterite closed this Aug 26, 2024
@asterite asterite deleted the ab/index-out-of-bounds-checks branch August 26, 2024 12:39
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.

Dead Instruction Elimination pass removes out of bounds array sets
1 participant