Skip to content

Commit

Permalink
feat(avm): fix some Brillig problems (AztecProtocol/aztec-packages#5091)
Browse files Browse the repository at this point in the history
This PR enables the stack of 6 PRs on top.
\
While working on external calls, we came across several problems with Brillig. I made some changes to fix them. Some Brillig changes:
* **Truncation**: Brillig was using AND of Fields (actually, AND on Ints of 254 bits). This is not supported by the VM. Truncation was changed to be done without ANDing, and using `CAST` instead, which truncates to the required bit size.
* **Array.get**/**Array.set**: Calculation of the `arrayBase+index` was done using field arithmetic (or field sizes). Now it's done using ints.
* **Reference counting**: Checking `refCount==1` was done using field arithmetic (or field sizes). Now it's done with ints.

These changes seem to solve all the comparison w/different bit sizes, and unexpected uses of field arithmetic. (That we've found with the current test contract).

NOTE: I had to recreate the contract snapshots with `yarn workspace @aztec/protocol-contracts test -u`, then modify
* noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr
* noir-projects/noir-contracts/contracts/fpc_contract/src/main.nr

Relates to #4313, #4127.
  • Loading branch information
AztecBot committed Mar 11, 2024
2 parents 7807ce1 + 0c0c242 commit 5c502ef
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 32 deletions.
2 changes: 1 addition & 1 deletion .aztec-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
b32725421171f39d510619c8f78a39c182738725
07dd8215dffd2c3c6d22e0f430f5072b4ff7c763
Original file line number Diff line number Diff line change
Expand Up @@ -831,16 +831,10 @@ impl<'block> BrilligBlock<'block> {
_ => unreachable!("ICE: array set on non-array"),
};

// Here we want to compare the reference count against 1.
let one = self.brillig_context.make_usize_constant(1_usize.into());
let condition = self.brillig_context.allocate_register();

self.brillig_context.binary_instruction(
reference_count,
one,
condition,
BrilligBinaryOp::Field { op: BinaryFieldOp::Equals },
);

self.brillig_context.memory_op(reference_count, one, condition, BinaryIntOp::Equals);
self.brillig_context.branch_instruction(condition, |ctx, cond| {
if cond {
// Reference count is 1, we can mutate the array directly
Expand Down
36 changes: 13 additions & 23 deletions compiler/noirc_evaluator/src/brillig/brillig_ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use acvm::{
FieldElement,
};
use debug_show::DebugShow;
use num_bigint::BigUint;

/// The Brillig VM does not apply a limit to the memory address space,
/// As a convention, we take use 64 bits. This means that we assume that
Expand Down Expand Up @@ -213,13 +212,7 @@ impl BrilligContext {
self.debug_show.array_get(array_ptr, index, result);
// Computes array_ptr + index, ie array[index]
let index_of_element_in_memory = self.allocate_register();
self.binary_instruction(
array_ptr,
index,
index_of_element_in_memory,
BrilligBinaryOp::Field { op: BinaryFieldOp::Add },
);

self.memory_op(array_ptr, index, index_of_element_in_memory, BinaryIntOp::Add);
self.load_instruction(result, index_of_element_in_memory);
// Free up temporary register
self.deallocate_register(index_of_element_in_memory);
Expand All @@ -239,7 +232,10 @@ impl BrilligContext {
array_ptr,
index,
index_of_element_in_memory,
BrilligBinaryOp::Field { op: BinaryFieldOp::Add },
BrilligBinaryOp::Integer {
op: BinaryIntOp::Add,
bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
},
);

self.store_instruction(index_of_element_in_memory, value);
Expand Down Expand Up @@ -726,6 +722,8 @@ impl BrilligContext {
/// Instead truncation instructions are emitted as to when a
/// truncation should be done.
/// For Brillig, all integer operations will overflow as its cheap.
/// We currently use cast to truncate: we cast to the required bit size
/// and back to the original bit size.
pub(crate) fn truncate_instruction(
&mut self,
destination_of_truncated_value: SingleAddrVariable,
Expand All @@ -744,20 +742,12 @@ impl BrilligContext {
value_to_truncate.bit_size
);

let mask = BigUint::from(2_u32).pow(bit_size) - BigUint::from(1_u32);
let mask_constant = self.make_constant(
FieldElement::from_be_bytes_reduce(&mask.to_bytes_be()).into(),
value_to_truncate.bit_size,
);

self.binary_instruction(
value_to_truncate.address,
mask_constant,
destination_of_truncated_value.address,
BrilligBinaryOp::Integer { op: BinaryIntOp::And, bit_size: value_to_truncate.bit_size },
);

self.deallocate_register(mask_constant);
// We cast back and forth to ensure that the value is truncated.
let intermediate_register =
SingleAddrVariable { address: self.allocate_register(), bit_size };
self.cast_instruction(intermediate_register, value_to_truncate);
self.cast_instruction(destination_of_truncated_value, intermediate_register);
self.deallocate_register(intermediate_register.address);
}

/// Emits a stop instruction
Expand Down

0 comments on commit 5c502ef

Please sign in to comment.