Skip to content

Commit

Permalink
feat(avm): fix some Brillig problems (#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
fcarreiro authored Mar 11, 2024
1 parent 8b10600 commit 07dd821
Show file tree
Hide file tree
Showing 6 changed files with 176 additions and 184 deletions.
8 changes: 8 additions & 0 deletions avm-transpiler/src/transpile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec<u8> {
lhs,
rhs,
} => {
assert!(is_integral_bit_size(*bit_size), "BinaryIntOp::{:?} bit_size must be integral, got {:?}", op, bit_size);
let avm_opcode = match op {
BinaryIntOp::Add => AvmOpcode::ADD,
BinaryIntOp::Sub => AvmOpcode::SUB,
Expand Down Expand Up @@ -913,6 +914,13 @@ fn map_brillig_pcs_to_avm_pcs(initial_offset: usize, brillig: &Brillig) -> Vec<u
pc_map
}

fn is_integral_bit_size(bit_size: u32) -> bool {
match bit_size {
1 | 8 | 16 | 32 | 64 | 128 => true,
_ => false,
}
}

fn tag_from_bit_size(bit_size: u32) -> AvmTypeTag {
match bit_size {
1 => AvmTypeTag::UINT8, // temp workaround
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ contract AppSubscription {

global SUBSCRIPTION_DURATION_IN_BLOCKS = 5;
global SUBSCRIPTION_TXS = 5;
global CANONICAL_GAS_TOKEN_ADDRESS = AztecAddress::from_field(0x22a29a9544c0cbdbc35bb21caa78b6f08903be4b390a65319e33afd7443f3499);
global CANONICAL_GAS_TOKEN_ADDRESS = AztecAddress::from_field(10518537853519909204957666322334442672584410385979059968848104222965977783517);

#[aztec(private)]
fn entrypoint(payload: pub DAppPayload, user_address: pub AztecAddress) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ contract FPC {
gas_token_address: SharedImmutable<AztecAddress>,
}

global CANONICAL_GAS_TOKEN_ADDRESS = AztecAddress::from_field(0x22a29a9544c0cbdbc35bb21caa78b6f08903be4b390a65319e33afd7443f3499);
global CANONICAL_GAS_TOKEN_ADDRESS = AztecAddress::from_field(10518537853519909204957666322334442672584410385979059968848104222965977783517);

#[aztec(public)]
#[aztec(initializer)]
Expand Down
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 noir/noir-repo/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
Loading

0 comments on commit 07dd821

Please sign in to comment.