Corrupted bounded vec after branch not taken #4600
Labels
bug
Something isn't working
Comments
|
I'm trying to get a minimal reproduction, but it's quite a bit complicated |
|
Note: rewriting it in this form: #[aztec(private)]
fn request_attestation(
from: AztecAddress,
attestor: AztecAddress,
blacklist_root: Field,
proofs: [Field; DEPTH * BOUNDED_VEC_LEN],
nonce: Field
) {
if (!from.eq(context.msg_sender())) {
assert_current_call_valid_authwit(&mut context, from);
} else {
assert(nonce == 0, "invalid nonce");
}
let maybe_note = storage.balances.last_unattested_note(from, attestor);
let mut attested = false;
if maybe_note.is_some() {
let note = maybe_note.unwrap_unchecked();
attested = Attestor::at(attestor).request_attestation(&mut context, note.partition_table, blacklist_root, proofs)[0] as bool;
}
if attested {
storage.balances.add_attestation(from, maybe_note.unwrap(), attestor);
}
}Makes the corruption no longer happen |
|
Unfortunately I didn't manage to simplify it to a point where the SSA isn't huge :/ |
|
I'm thinking that we should make the --show-ssa more selective. Oftentimes we are interested in a single pass but get a wall of text where the majority is irrelevant. |
|
This is the simplest I could make it while still reproducing the corruption: use crate::types::token_note::OwnedNote;
use dep::aztec::protocol_types::{
constants::{MAX_NOTES_PER_PAGE, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL},
abis::side_effect::{SideEffect, SideEffectLinkedToNoteHash}
};
use dep::aztec::context::{PrivateContext, PublicContext, Context};
use dep::aztec::note::{
lifecycle::{create_note, create_note_hash_from_public, destroy_note},
note_getter::{get_notes, view_notes}, note_interface::NoteInterface,
note_viewer_options::NoteViewerOptions, utils::compute_note_hash_for_consumption
};
use dep::aztec::oracle::notes::{notify_created_note, notify_nullified_note};
use dep::aztec::{hash::pedersen_hash};
#[contract_library_method]
fn compute_nullifier(hash: Field, account: AztecAddress, context: &mut PrivateContext) -> Field {
let secret = context.request_nullifier_secret_key(account);
pedersen_hash(
[
hash,
secret.low,
secret.high
],
0
)
}
// TODO: Recursively call `request_attestation` until all attestations are received?
#[aztec(private)]
fn request_attestation(
from: AztecAddress,
attestor: AztecAddress,
blacklist_root: Field,
proofs: [Field; DEPTH * BOUNDED_VEC_LEN],
nonce: Field
) {
dep::aztec::oracle::debug_log::debug_log_array_with_prefix(
"Start request_attestation",
context.private_call_stack_hashes.storage
);
if (!from.eq(context.msg_sender())) {
assert_current_call_valid_authwit(&mut context, from);
} else {
assert(nonce == 0, "invalid nonce");
}
let maybe_note = storage.balances.last_unattested_note(from, attestor);
let note = maybe_note.unwrap();
let attested = Attestor::at(attestor).request_attestation(&mut context, note.partition_table, blacklist_root, proofs)[0] as bool;
dep::aztec::oracle::debug_log::debug_log_array_with_prefix("Attested", [attested]);
dep::aztec::oracle::debug_log::debug_log_array_with_prefix("Before if", context.private_call_stack_hashes.storage);
if attested {
dep::aztec::oracle::debug_log::debug_log_array_with_prefix(
"Should not appear",
context.private_call_stack_hashes.storage
);
let mut nullifier = 0;
let mut consumed_note_hash: Field = 0;
nullifier = compute_nullifier(27, from, &mut context);
context.push_new_nullifier(nullifier, consumed_note_hash);
}
dep::aztec::oracle::debug_log::debug_log_array_with_prefix("After if", context.private_call_stack_hashes.storage);
}I've attached the SSA I think the corrupt private_call_stack_hashes are The error still reproduces with this simpler (in SSA terms) code: |
sirasistant
changed the title
|
We seem to have hit a similar instance of this issue in the branch In this case, the function causing issues is this one: fn execute_calls(self, context: &mut PrivateContext) {
dep::aztec::oracle::debug_log::debug_log("Starting execute calls");
context.print_note_hash_read_requests();
for call in self.function_calls {
if !call.target_address.is_zero() {
dep::aztec::oracle::debug_log::debug_log_format(
"Executing function call to={0} args={1}",
[call.target_address.to_field(), call.args_hash]
);
if call.is_public {
context.call_public_function_with_packed_args(
call.target_address,
call.function_selector,
call.args_hash,
false,
false
);
} else {
let _result = context.call_private_function_with_packed_args(
call.target_address,
call.function_selector,
call.args_hash,
false,
false
);
}
}
}
dep::aztec::oracle::debug_log::debug_log("Ended execute calls");
context.print_note_hash_read_requests();
}The logs that correspond to a run where We have also tried to reproduce this in a noir circuit outside aztec-packages, in |
|
I wonder if this is related to the constant folding bug I found in #4716. These cases both seem similar in that branches not taken in an if statement are affecting the result of the program. If you make this change: https://github.com/noir-lang/noir/pull/4716/files#diff-1d0f0549b989551a60174308917ce4e7cbd5615ef86813e1729ac60e903f3702R281-R285 does it fix the issue? |
|
@jfecher that worked, thanks so much!! Does it make sense to submit a PR with just that change, or is it best to wait for 4716 to be ready? |
|
@spalladino good to hear! I'll make a separate PR with that change now. #4716 may not be ready for a while since there is 1 remaining failing test in it that is very tricky to fix. |
github-merge-queue bot
pushed a commit
that referenced
this issue
Apr 11, 2024
# Description ## Problem\* Resolves #4600 ## Summary\* Array sets and gets are not pure. Listing them as such can cause constant folding to fold away enabled array gets for ones in disabled contexts (enable_side_effects_if u1 0). When used in disabled contexts they will only ever retrieve from index zero. ## Additional Context No reproduceable test case unfortunately. The ones in #4600 all require aztec as a dependency and the ones in #4717 only trigger with the remove_if_else pass added there. I'm going to add a SSA unit test instead where two array get instructions should survive constant folding. This PR is a draft until I add that. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Aim
Running in the aztec sandbox this test that runs this function
Expected Behavior
The function should run normally
Bug
The simulator complains that the private_call_stack_hashes exposed by the function do not match the oracle calls emitted to call other functions. Digging into the code with debug logs:
We can see that the bounded vec that stores the private_call_stack_hashes gets corrupted after the
if attested {}that is not taken in runtime:Where the initial item gets duplicated to all other items.
The call_stack_item_hashes is only written in
assert_current_call_valid_authwit(which isn't executed, so no item is pushed) andrequest_attestation(which is executed, and that where the initial item that gets duplicated comes from)To Reproduce
yarn compile && yarn codegen && yarn test -t "Request attestation after blacklisting"Project Impact
None
Impact Context
No response
Workaround
None
Workaround Description
No response
Additional Context
No response
Installation Method
None
Nargo Version
No response
NoirJS Version
No response
Would you like to submit a PR for this Issue?
None
Support Needs
No response
The text was updated successfully, but these errors were encountered: