From 6f9efe707a8cd6380e57ebbd10af0a6672c84908 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Mon, 25 Sep 2023 13:30:40 +0000 Subject: [PATCH 1/8] 1090 - new routine to validate arrays and unit tests --- circuits/cpp/src/aztec3/utils/array.hpp | 37 +++++ circuits/cpp/src/aztec3/utils/array.test.cpp | 130 ++++++++++++++++++ .../cpp/src/aztec3/utils/circuit_errors.hpp | 1 + 3 files changed, 168 insertions(+) diff --git a/circuits/cpp/src/aztec3/utils/array.hpp b/circuits/cpp/src/aztec3/utils/array.hpp index 0134e7e30ad..00cfd6a77f5 100644 --- a/circuits/cpp/src/aztec3/utils/array.hpp +++ b/circuits/cpp/src/aztec3/utils/array.hpp @@ -5,6 +5,8 @@ #include +#include + /** * NOTE: see bberg's stdlib/primitives/field/array.hpp for the corresponding circuit implementations of these functions. */ @@ -60,6 +62,41 @@ template size_t array_length(std::array const return length; }; +/** + * @brief Routine which validates that all zero values of an array form a contiguous region at the end, i.e., + of the form: [*,*,*...,0,0,0,0] where any * is non-zero. Note that a full array of non-zero values is + valid. + * @tparam The type of the values stored in the array. + * @tparam The builder type. + * @tparam The size of the array. + * @param builder The builder which will assert on an array being malformed. + * @param arr The array to validate. + * @param error_message The prefix to the error message set by the caller + */ +template +void validate_array(Builder& builder, std::array const& arr, std::string const& error_message) +{ + size_t first_zero_pos = SIZE; + size_t last_non_zero_pos = 0; + for (size_t i = 0; i < SIZE; i++) { + if (!is_empty(arr[i])) { + last_non_zero_pos = i; + } else if (is_empty(arr[i]) && first_zero_pos == SIZE) { + first_zero_pos = i; + } + } + + builder.do_assert( + last_non_zero_pos <= first_zero_pos, // Only case when equality holds is for a full zeros array. + format( + error_message, + " - array is not well-formed. A non-zero value occured after a zero. Last position of a non-zero value: ", + last_non_zero_pos, + "; First position of a zero-value: ", + first_zero_pos), + CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +}; + /** * @brief Helper method to return the last non-empty item in an array * @tparam The type of the value stored in the array diff --git a/circuits/cpp/src/aztec3/utils/array.test.cpp b/circuits/cpp/src/aztec3/utils/array.test.cpp index 76b418f839a..c33ab72ed11 100644 --- a/circuits/cpp/src/aztec3/utils/array.test.cpp +++ b/circuits/cpp/src/aztec3/utils/array.test.cpp @@ -1,5 +1,7 @@ #include "array.hpp" +#include "aztec3/utils/dummy_circuit_builder.hpp" + #include #include @@ -127,4 +129,132 @@ TEST(utils_array_tests, rearrange_test_vector_long_zeros_left) rearrange_and_check(test_vec, test_vec_rearranged, "long zeros left"); } +TEST(utils_array_validation, test_vector_all_zeros) +{ + const size_t SIZE = 64; + std::array test_vec{}; + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector all zeros"); + + EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); +} + +TEST(utils_array_validation, test_vector_all_non_zeros) +{ + const size_t SIZE = 64; + std::array test_vec; + unsigned int gen = 4127; + for (size_t i = 0; i < SIZE; i++) { + test_vec[i] = fr(gen); + gen = 761 * gen % 5619; + } + + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector all non zeros"); + + EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); +} + +TEST(utils_array_validation, test_vector_valid_one_zero) +{ + const size_t SIZE = 110; + std::array test_vec{}; + unsigned int gen = 4159; + for (size_t i = 0; i < SIZE - 1; i++) { + test_vec[i] = fr(gen); + gen = 71 * gen % 2613; + } + + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector a single zero at the end"); + + EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); +} + +TEST(utils_array_validation, test_vector_valid_one_non_zero) +{ + const size_t SIZE = 110; + std::array test_vec{}; + test_vec[0] = fr(124); + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector a single non-zero at the beginning"); + + EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); +} + +TEST(utils_array_validation, test_vector_invalid_one_zero_middle) +{ + const size_t SIZE = 128; + std::array test_vec{}; + unsigned int gen = 354; + for (size_t i = 0; i < SIZE; i++) { + test_vec[i] = fr(gen); + gen = 319 * gen % 2213; + } + test_vec[67] = fr(0); + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector a single zero in the middle"); + + EXPECT_TRUE(dummyBuilder.failed()); +} + +TEST(utils_array_validation, test_vector_invalid_one_zero_beginning) +{ + const size_t SIZE = 128; + std::array test_vec{}; + unsigned int gen = 447; + for (size_t i = 0; i < SIZE; i++) { + test_vec[i] = fr(gen); + gen = 39 * gen % 12313; + } + test_vec[0] = fr(0); + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector a single zero at the beginning"); + + EXPECT_TRUE(dummyBuilder.failed()); +} + +TEST(utils_array_validation, test_vector_invalid_zero_both_ends) +{ + const size_t SIZE = 128; + std::array test_vec{}; + unsigned int gen = 47; + for (size_t i = 0; i < SIZE; i++) { + test_vec[i] = fr(gen); + gen = 6439 * gen % 82313; + } + test_vec[0] = fr(0); + test_vec[SIZE - 1] = fr(0); + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector with a zero at each end"); + + EXPECT_TRUE(dummyBuilder.failed()); +} + +TEST(utils_array_validation, test_vector_invalid_non_zero_last) +{ + const size_t SIZE = 203; + std::array test_vec{}; + test_vec[SIZE - 1] = fr(785); + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector with a non-zero at the end"); + + EXPECT_TRUE(dummyBuilder.failed()); +} + +TEST(utils_array_validation, test_vector_invalid_alternate) +{ + const size_t SIZE = 203; + std::array test_vec{}; + unsigned int gen = 83; + for (size_t i = 0; i < SIZE; i += 2) { + test_vec[i] = fr(gen); + gen = 2437 * gen % 2314; + } + DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); + validate_array(dummyBuilder, test_vec, "Test vector consisting in alternating zero and non-zero values."); + + EXPECT_TRUE(dummyBuilder.failed()); +} + } // namespace aztec3::utils \ No newline at end of file diff --git a/circuits/cpp/src/aztec3/utils/circuit_errors.hpp b/circuits/cpp/src/aztec3/utils/circuit_errors.hpp index f3a46976397..e2c49485b9c 100644 --- a/circuits/cpp/src/aztec3/utils/circuit_errors.hpp +++ b/circuits/cpp/src/aztec3/utils/circuit_errors.hpp @@ -89,6 +89,7 @@ enum CircuitErrorCode : uint16_t { PUBLIC_DATA_TREE_ROOT_MISMATCH = 7007, MEMBERSHIP_CHECK_FAILED = 7008, ARRAY_OVERFLOW = 7009, + ARRAY_NOT_ZERO_RIGHT_PADDED = 7010, ROOT_CIRCUIT_FAILED = 8000, From a64a292f7b42fba931e6c297eb3f1c80b029f40d Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 26 Sep 2023 08:47:43 +0000 Subject: [PATCH 2/8] 1090 - activate the array validation in init and inner private kernel circuit --- .../aztec3/circuits/kernel/private/common.cpp | 31 ++++++++++++++++--- .../aztec3/circuits/kernel/private/common.hpp | 3 ++ .../native_private_kernel_circuit_init.cpp | 2 ++ .../native_private_kernel_circuit_inner.cpp | 2 ++ 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp index adf1d70353a..557a20f93bd 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp @@ -7,6 +7,7 @@ #include "aztec3/circuits/abis/function_data.hpp" #include "aztec3/circuits/abis/kernel_circuit_public_inputs.hpp" #include "aztec3/circuits/abis/new_contract_data.hpp" +#include "aztec3/circuits/abis/private_circuit_public_inputs.hpp" #include "aztec3/circuits/abis/private_kernel/private_call_data.hpp" #include "aztec3/circuits/abis/read_request_membership_witness.hpp" #include "aztec3/circuits/hash.hpp" @@ -16,6 +17,7 @@ using DummyBuilder = aztec3::utils::DummyCircuitBuilder; +using aztec3::circuits::abis::CompleteAddress; using aztec3::circuits::abis::ContractDeploymentData; using aztec3::circuits::abis::ContractLeafPreimage; using aztec3::circuits::abis::FunctionData; @@ -26,6 +28,7 @@ using aztec3::circuits::abis::ReadRequestMembershipWitness; using aztec3::utils::array_push; using aztec3::utils::is_array_empty; using aztec3::utils::push_array_to_array; +using aztec3::utils::validate_array; using DummyBuilder = aztec3::utils::DummyCircuitBuilder; using CircuitErrorCode = aztec3::utils::CircuitErrorCode; using aztec3::circuits::abis::private_kernel::PrivateCallData; @@ -114,6 +117,26 @@ void common_validate_read_requests(DummyBuilder& builder, } } +/** + * @brief We validate that relevant arrays assumed to be zero-padded on the right comply to this format. + * + * @param builder + * @param app_public_inputs Reference to the private_circuit_public_inputs of the current kernel iteration. + */ +void common_validate_arrays(DummyBuilder& builder, PrivateCircuitPublicInputs const& app_public_inputs) +{ + validate_array(builder, app_public_inputs.new_commitments, "New commitments"); + validate_array(builder, app_public_inputs.new_nullifiers, "New nullifiers"); + validate_array(builder, app_public_inputs.nullified_commitments, "Nullified commitments"); + validate_array(builder, app_public_inputs.private_call_stack, "Private Call Stack"); + validate_array(builder, app_public_inputs.public_call_stack, "Public Call Stack"); + validate_array(builder, app_public_inputs.new_l2_to_l1_msgs, "New L2 to L1 messages"); + // return_values are not used in private kernel circuit and therefore not validated. + // encrypted_logs_hash and unencrypted_logs_hash have their own integrity checks. + // read_requests are processed through a loop to the whole array and therefore does not + // rely on the array to be right-zero padded. +} + void common_validate_0th_nullifier(DummyBuilder& builder, CombinedAccumulatedData const& end) { builder.do_assert(end.new_nullifiers[0] != 0, @@ -296,10 +319,10 @@ void common_contract_logic(DummyBuilder& builder, auto constructor_hash = compute_constructor_hash(function_data, private_call_public_inputs.args_hash, private_call_vk_hash); - auto const new_contract_address = abis::CompleteAddress::compute(contract_dep_data.deployer_public_key, - contract_dep_data.contract_address_salt, - contract_dep_data.function_tree_root, - constructor_hash) + auto const new_contract_address = CompleteAddress::compute(contract_dep_data.deployer_public_key, + contract_dep_data.contract_address_salt, + contract_dep_data.function_tree_root, + constructor_hash) .address; // Add new contract data if its a contract deployment function diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp index 32f231262cc..97f3de0843c 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp @@ -7,6 +7,7 @@ #include "aztec3/circuits/abis/function_data.hpp" #include "aztec3/circuits/abis/kernel_circuit_public_inputs.hpp" #include "aztec3/circuits/abis/previous_kernel_data.hpp" +#include "aztec3/circuits/abis/private_circuit_public_inputs.hpp" #include "aztec3/circuits/abis/private_kernel/private_call_data.hpp" #include "aztec3/circuits/abis/read_request_membership_witness.hpp" #include "aztec3/utils/dummy_circuit_builder.hpp" @@ -19,6 +20,7 @@ using aztec3::circuits::abis::ContractDeploymentData; using aztec3::circuits::abis::FunctionData; using aztec3::circuits::abis::KernelCircuitPublicInputs; using aztec3::circuits::abis::PreviousKernelData; +using aztec3::circuits::abis::PrivateCircuitPublicInputs; using aztec3::circuits::abis::ReadRequestMembershipWitness; using aztec3::circuits::abis::private_kernel::PrivateCallData; @@ -34,6 +36,7 @@ void common_validate_read_requests(DummyBuilder& builder, std::array, MAX_READ_REQUESTS_PER_CALL> const& read_request_membership_witnesses); +void common_validate_arrays(DummyBuilder& builder, PrivateCircuitPublicInputs const& app_public_inputs); void common_validate_0th_nullifier(DummyBuilder& builder, CombinedAccumulatedData const& end); void common_update_end_values(DummyBuilder& builder, diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.cpp index 9e70e541a0a..da7d27f74e8 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.cpp @@ -160,6 +160,8 @@ KernelCircuitPublicInputs native_private_kernel_circuit_initial(DummyCircuit validate_inputs(builder, private_inputs); + common_validate_arrays(builder, private_inputs.private_call.call_stack_item.public_inputs); + validate_this_private_call_against_tx_request(builder, private_inputs); common_validate_call_stack(builder, private_inputs.private_call); diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.cpp index 4312d82c04d..aac28a78a1c 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.cpp @@ -106,6 +106,8 @@ KernelCircuitPublicInputs native_private_kernel_circuit_inner(DummyCircuitBu validate_inputs(builder, private_inputs); + common_validate_arrays(builder, private_inputs.private_call.call_stack_item.public_inputs); + pop_and_validate_this_private_call_hash(builder, private_inputs.private_call, public_inputs.end.private_call_stack); common_validate_call_stack(builder, private_inputs.private_call); From 659eeca824c14062d161dbe2b35105c065dd854a Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 26 Sep 2023 10:23:13 +0000 Subject: [PATCH 3/8] 1090 - comments --- circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp | 4 +++- .../kernel/private/native_private_kernel_circuit_ordering.cpp | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp index 557a20f93bd..d53965a61ff 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp @@ -125,6 +125,8 @@ void common_validate_read_requests(DummyBuilder& builder, */ void common_validate_arrays(DummyBuilder& builder, PrivateCircuitPublicInputs const& app_public_inputs) { + // The following arrays (new_commitments, etc...) are passed to push_array_to_array() + // routines which rely on the passed arrays to be well-formed. validate_array(builder, app_public_inputs.new_commitments, "New commitments"); validate_array(builder, app_public_inputs.new_nullifiers, "New nullifiers"); validate_array(builder, app_public_inputs.nullified_commitments, "Nullified commitments"); @@ -133,7 +135,7 @@ void common_validate_arrays(DummyBuilder& builder, PrivateCircuitPublicInputs Date: Wed, 27 Sep 2023 08:53:13 +0000 Subject: [PATCH 4/8] 1090 - noir code should never push a zero nullified_commitment --- yarn-project/aztec-nr/aztec/src/auth.nr | 5 +++-- .../noir-contracts/src/contracts/test_contract/src/main.nr | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/yarn-project/aztec-nr/aztec/src/auth.nr b/yarn-project/aztec-nr/aztec/src/auth.nr index 238f633f218..3d7b68d9241 100644 --- a/yarn-project/aztec-nr/aztec/src/auth.nr +++ b/yarn-project/aztec-nr/aztec/src/auth.nr @@ -1,17 +1,18 @@ use crate::context::{PrivateContext, PublicContext}; use crate::oracle::compute_selector::compute_selector; +use crate::constants_gen::EMPTY_NULLIFIED_COMMITMENT; global IS_VALID_SELECTOR = 0xe86ab4ff; global IS_VALID_PUBLIC_SELECTOR = 0xf3661153; fn assert_valid_message_for(context: &mut PrivateContext, whom: Field, message_hash: Field) { let result = context.call_private_function(whom, IS_VALID_SELECTOR, [message_hash])[0]; - context.push_new_nullifier(message_hash, 0); + context.push_new_nullifier(message_hash, EMPTY_NULLIFIED_COMMITMENT); assert(result == IS_VALID_SELECTOR, "Message not authorized by account"); } fn assert_valid_public_message_for(context: &mut PublicContext, whom: Field, message_hash: Field) { let result = context.call_public_function(whom, IS_VALID_PUBLIC_SELECTOR, [message_hash])[0]; - context.push_new_nullifier(message_hash, 0); + context.push_new_nullifier(message_hash, EMPTY_NULLIFIED_COMMITMENT); assert(result == IS_VALID_SELECTOR, "Message not authorized by account"); } \ No newline at end of file diff --git a/yarn-project/noir-contracts/src/contracts/test_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/test_contract/src/main.nr index f467af53656..02b14706bea 100644 --- a/yarn-project/noir-contracts/src/contracts/test_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/test_contract/src/main.nr @@ -103,7 +103,7 @@ contract Test { let note = DummyNote::new(amount, secretHash); // Public oracle call to emit new commitment. - context.push_new_nullifier(note.get_commitment(), 0); + context.push_new_nullifier(note.get_commitment(), EMPTY_NULLIFIED_COMMITMENT); } // Forcefully emits a nullifier (for testing purposes) From 333809de02f7880141151ad0691c53f79544f2f4 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 27 Sep 2023 12:20:06 +0000 Subject: [PATCH 5/8] 1090 - replaces a zero nullified_commitment by EMPTY placeholder while pushing a new nullifier --- yarn-project/aztec-nr/aztec/src/context.nr | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/yarn-project/aztec-nr/aztec/src/context.nr b/yarn-project/aztec-nr/aztec/src/context.nr index 022319561cb..eadbe9c20da 100644 --- a/yarn-project/aztec-nr/aztec/src/context.nr +++ b/yarn-project/aztec-nr/aztec/src/context.nr @@ -159,9 +159,16 @@ impl PrivateContext { self.new_commitments.push(note_hash); } + // We never push a zero nullified_commitment as zero is used to indicate the end + // of a field array in private kernel. This routine transparently replaces a + // zero value into the special placeholder: EMPTY_NULLIFIED_COMMITMENT. fn push_new_nullifier(&mut self, nullifier: Field, nullified_commitment: Field) { self.new_nullifiers.push(nullifier); - self.nullified_commitments.push(nullified_commitment); + let mut non_zero_nullified = nullified_commitment; + if (non_zero_nullified == 0) { + non_zero_nullified = EMPTY_NULLIFIED_COMMITMENT; + } + self.nullified_commitments.push(non_zero_nullified); } // docs:start:context_message_portal From 2405093896f853fd53b6032d8e564dc80dc8c2e5 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 28 Sep 2023 06:42:39 +0000 Subject: [PATCH 6/8] 1090 - add unit tests for private kernel circuit on malformed array validation --- ...ative_private_kernel_circuit_init.test.cpp | 89 +++++++++++++++++++ ...tive_private_kernel_circuit_inner.test.cpp | 88 ++++++++++++++++++ 2 files changed, 177 insertions(+) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp index 5c66b6fc7b4..05286799ed3 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp @@ -117,6 +117,95 @@ TEST_F(native_private_kernel_init_tests, basic_contract_deployment) EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::NO_ERROR); } +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_commitments) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_commitments{ fr(0), fr(9123) }; + private_inputs.private_call.call_stack_item.public_inputs.new_commitments = malformed_commitments; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_commitments"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_nullifiers) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_nullifiers{}; + malformed_nullifiers[MAX_NEW_NULLIFIERS_PER_CALL - 1] = fr(12); + private_inputs.private_call.call_stack_item.public_inputs.new_nullifiers = malformed_nullifiers; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_nullifiers"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_nullified_commitments) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_nullified_commitments{ fr(0), + fr(0), + EMPTY_NULLIFIED_COMMITMENT }; + private_inputs.private_call.call_stack_item.public_inputs.nullified_commitments = malformed_nullified_commitments; + + DummyBuilder builder = + DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_nullified_commitments"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_private_call_stack) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_private_call_stack{ fr(0), fr(888) }; + private_inputs.private_call.call_stack_item.public_inputs.private_call_stack = malformed_private_call_stack; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_private_call_stack"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_public_call_stack) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_public_call_stack{ fr(0), fr(888) }; + private_inputs.private_call.call_stack_item.public_inputs.public_call_stack = malformed_public_call_stack; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_public_call_stack"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_new_l2_to_l1_msgs) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_new_l2_to_l1_msgs{}; + malformed_new_l2_to_l1_msgs[MAX_NEW_L2_TO_L1_MSGS_PER_CALL - 1] = fr(1); + private_inputs.private_call.call_stack_item.public_inputs.new_l2_to_l1_msgs = malformed_new_l2_to_l1_msgs; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_new_l2_to_l1_msgs"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + TEST_F(native_private_kernel_init_tests, contract_deployment_call_stack_item_hash_mismatch_fails) { auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp index 77a8c7cfa65..f1da6df8223 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp @@ -206,6 +206,94 @@ TEST_F(native_private_kernel_inner_tests, private_function_incorrect_call_stack_ CircuitErrorCode::PRIVATE_KERNEL__CALCULATED_PRIVATE_CALL_HASH_AND_PROVIDED_PRIVATE_CALL_HASH_MISMATCH); } +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_commitments) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_commitments{ fr(0), fr(9123) }; + private_inputs.private_call.call_stack_item.public_inputs.new_commitments = malformed_commitments; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_commitments"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_nullifiers) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_nullifiers{}; + malformed_nullifiers[MAX_NEW_NULLIFIERS_PER_CALL - 1] = fr(12); + private_inputs.private_call.call_stack_item.public_inputs.new_nullifiers = malformed_nullifiers; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_nullifiers"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_nullified_commitments) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_nullified_commitments{ fr(0), + fr(0), + EMPTY_NULLIFIED_COMMITMENT }; + private_inputs.private_call.call_stack_item.public_inputs.nullified_commitments = malformed_nullified_commitments; + + DummyBuilder builder = + DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_nullified_commitments"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_private_call_stack) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_private_call_stack{ fr(0), fr(888) }; + private_inputs.private_call.call_stack_item.public_inputs.private_call_stack = malformed_private_call_stack; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_private_call_stack"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_public_call_stack) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_public_call_stack{ fr(0), fr(888) }; + private_inputs.private_call.call_stack_item.public_inputs.public_call_stack = malformed_public_call_stack; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_public_call_stack"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_new_l2_to_l1_msgs) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_new_l2_to_l1_msgs{}; + malformed_new_l2_to_l1_msgs[MAX_NEW_L2_TO_L1_MSGS_PER_CALL - 1] = fr(1); + private_inputs.private_call.call_stack_item.public_inputs.new_l2_to_l1_msgs = malformed_new_l2_to_l1_msgs; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_new_l2_to_l1_msgs"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + TEST_F(native_private_kernel_inner_tests, private_kernel_should_fail_if_aggregating_too_many_commitments) { // Negative test to check if push_array_to_array fails if two many commitments are merged together From 4a2726e6d1f3474731fea9087a404e11b00190c1 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 28 Sep 2023 07:06:49 +0000 Subject: [PATCH 7/8] 1090 - rewording comments in unit tests --- circuits/cpp/src/aztec3/utils/array.test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/circuits/cpp/src/aztec3/utils/array.test.cpp b/circuits/cpp/src/aztec3/utils/array.test.cpp index c33ab72ed11..9fd4140d343 100644 --- a/circuits/cpp/src/aztec3/utils/array.test.cpp +++ b/circuits/cpp/src/aztec3/utils/array.test.cpp @@ -134,7 +134,7 @@ TEST(utils_array_validation, test_vector_all_zeros) const size_t SIZE = 64; std::array test_vec{}; DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector all zeros"); + validate_array(dummyBuilder, test_vec, "Test vector with all zeros"); EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); } @@ -150,7 +150,7 @@ TEST(utils_array_validation, test_vector_all_non_zeros) } DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector all non zeros"); + validate_array(dummyBuilder, test_vec, "Test vector with all non zeros"); EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); } @@ -166,7 +166,7 @@ TEST(utils_array_validation, test_vector_valid_one_zero) } DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector a single zero at the end"); + validate_array(dummyBuilder, test_vec, "Test vector with a single zero at the end"); EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); } @@ -177,7 +177,7 @@ TEST(utils_array_validation, test_vector_valid_one_non_zero) std::array test_vec{}; test_vec[0] = fr(124); DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector a single non-zero at the beginning"); + validate_array(dummyBuilder, test_vec, "Test vector with a single non-zero at the beginning"); EXPECT_FALSE(dummyBuilder.failed()) << dummyBuilder.get_first_failure(); } @@ -193,7 +193,7 @@ TEST(utils_array_validation, test_vector_invalid_one_zero_middle) } test_vec[67] = fr(0); DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector a single zero in the middle"); + validate_array(dummyBuilder, test_vec, "Test vector with a single zero in the middle"); EXPECT_TRUE(dummyBuilder.failed()); } @@ -209,7 +209,7 @@ TEST(utils_array_validation, test_vector_invalid_one_zero_beginning) } test_vec[0] = fr(0); DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector a single zero at the beginning"); + validate_array(dummyBuilder, test_vec, "Test vector with a single zero at the beginning"); EXPECT_TRUE(dummyBuilder.failed()); } @@ -252,7 +252,7 @@ TEST(utils_array_validation, test_vector_invalid_alternate) gen = 2437 * gen % 2314; } DummyCircuitBuilder dummyBuilder("Builder for array validation test vectors"); - validate_array(dummyBuilder, test_vec, "Test vector consisting in alternating zero and non-zero values."); + validate_array(dummyBuilder, test_vec, "Test vector with alternating zero and non-zero values."); EXPECT_TRUE(dummyBuilder.failed()); } From 19ed6224245426fd00b9ba654ff8e7264edd3079 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Fri, 29 Sep 2023 06:21:47 +0000 Subject: [PATCH 8/8] 1090 - address review's feedback --- .../aztec3/circuits/kernel/private/common.cpp | 10 +++---- ...ative_private_kernel_circuit_init.test.cpp | 28 ++++++++++++++++++ ...tive_private_kernel_circuit_inner.test.cpp | 29 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp index d53965a61ff..ea3ee298746 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp @@ -125,18 +125,18 @@ void common_validate_read_requests(DummyBuilder& builder, */ void common_validate_arrays(DummyBuilder& builder, PrivateCircuitPublicInputs const& app_public_inputs) { - // The following arrays (new_commitments, etc...) are passed to push_array_to_array() - // routines which rely on the passed arrays to be well-formed. + // Each of the following arrays is expected to be zero-padded. + // In addition, some of the following arrays (new_commitments, etc...) are passed + // to push_array_to_array() routines which rely on the passed arrays to be well-formed. + validate_array(builder, app_public_inputs.return_values, "Return values"); + validate_array(builder, app_public_inputs.read_requests, "Read requests"); validate_array(builder, app_public_inputs.new_commitments, "New commitments"); validate_array(builder, app_public_inputs.new_nullifiers, "New nullifiers"); validate_array(builder, app_public_inputs.nullified_commitments, "Nullified commitments"); validate_array(builder, app_public_inputs.private_call_stack, "Private Call Stack"); validate_array(builder, app_public_inputs.public_call_stack, "Public Call Stack"); validate_array(builder, app_public_inputs.new_l2_to_l1_msgs, "New L2 to L1 messages"); - // return_values are not used in private kernel circuit and therefore not validated. // encrypted_logs_hash and unencrypted_logs_hash have their own integrity checks. - // read_requests are processed through a loop over the whole array and therefore do not - // rely on the array to be right-zero padded. } void common_validate_0th_nullifier(DummyBuilder& builder, CombinedAccumulatedData const& end) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp index 05286799ed3..8457a6d7d76 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.test.cpp @@ -117,6 +117,34 @@ TEST_F(native_private_kernel_init_tests, basic_contract_deployment) EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::NO_ERROR); } +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_return_values) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_return_values{ fr(0), fr(0), fr(553) }; + private_inputs.private_call.call_stack_item.public_inputs.return_values = malformed_return_values; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_return_values"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_read_requests) +{ + auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); + + std::array malformed_read_requests{ fr(0), fr(9123), fr(0), fr(12) }; + private_inputs.private_call.call_stack_item.public_inputs.read_requests = malformed_read_requests; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_read_requests"); + native_private_kernel_circuit_initial(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + TEST_F(native_private_kernel_init_tests, input_validation_malformed_arrays_commitments) { auto private_inputs = do_private_call_get_kernel_inputs_init(true, constructor, standard_test_args()); diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp index f1da6df8223..b27ad2ebe69 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_inner.test.cpp @@ -206,6 +206,34 @@ TEST_F(native_private_kernel_inner_tests, private_function_incorrect_call_stack_ CircuitErrorCode::PRIVATE_KERNEL__CALCULATED_PRIVATE_CALL_HASH_AND_PROVIDED_PRIVATE_CALL_HASH_MISMATCH); } +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_return_values) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_return_values{ fr(0), fr(0), fr(553) }; + private_inputs.private_call.call_stack_item.public_inputs.return_values = malformed_return_values; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_return_values"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + +TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_read_requests) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array malformed_read_requests{ fr(0), fr(9123), fr(0), fr(12) }; + private_inputs.private_call.call_stack_item.public_inputs.read_requests = malformed_read_requests; + + DummyBuilder builder = DummyBuilder("private_kernel_tests__input_validation_malformed_arrays_read_requests"); + native_private_kernel_circuit_inner(builder, private_inputs); + + EXPECT_EQ(builder.failed(), true); + EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); +} + TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_commitments) { auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); @@ -219,6 +247,7 @@ TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_comm EXPECT_EQ(builder.failed(), true); EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::ARRAY_NOT_ZERO_RIGHT_PADDED); } + TEST_F(native_private_kernel_inner_tests, input_validation_malformed_arrays_nullifiers) { auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args());