diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp index b6a936d7d8b..0b637b960de 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp @@ -61,24 +61,25 @@ void common_validate_call_stack(DummyBuilder& builder, PrivateCallData const * - https://discourse.aztec.network/t/spending-notes-which-havent-yet-been-inserted/180 * * @param builder - * @param read_requests the commitments being read by this private call - * @param read_request_membership_witnesses used to compute the private data root - * for a given request which is essentially a membership check. + * @param storage_contract_address Contract address to use when siloing read requests * @param historic_private_data_tree_root This is a reference to the historic root which all * read requests are checked against here. + * @param read_requests the commitments being read by this private call + * @param read_request_membership_witnesses used to compute the private data root + * for a given request which is essentially a membership check */ void common_validate_read_requests(DummyBuilder& builder, NT::fr const& storage_contract_address, + NT::fr const& historic_private_data_tree_root, std::array const& read_requests, std::array, - MAX_READ_REQUESTS_PER_CALL> const& read_request_membership_witnesses, - NT::fr const& historic_private_data_tree_root) + MAX_READ_REQUESTS_PER_CALL> const& read_request_membership_witnesses) { // Arrays read_request and read_request_membership_witnesses must be of the same length. Otherwise, // we might get into trouble when accumulating them in public_inputs.end - builder.do_assert(array_length(read_requests) == array_length(read_request_membership_witnesses), - format("mismatch array length between read_requests and witnesses - read_requests length: ", + format("[private kernel circuit] mismatch array length between read_requests and witnesses - " + "read_requests length: ", array_length(read_requests), " witnesses length: ", array_length(read_request_membership_witnesses)), @@ -101,26 +102,65 @@ void common_validate_read_requests(DummyBuilder& builder, if (read_request != 0 && !witness.is_transient) { const auto& root_for_read_request = root_from_sibling_path(leaf, witness.leaf_index, witness.sibling_path); - builder.do_assert( - root_for_read_request == historic_private_data_tree_root, - format("private data tree root mismatch at read_request[", - rr_idx, - "]", - "\n\texpected root: ", - historic_private_data_tree_root, - "\n\tbut got root*: ", - root_for_read_request, - "\n\tread_request: ", - read_request, - "\n\tsiloed-rr (leaf): ", - leaf, - "\n\t* got root by siloing read_request (compressing with storage_contract_address to get leaf) " - "and merkle-hashing to a root using membership witness"), - CircuitErrorCode::PRIVATE_KERNEL__READ_REQUEST_PRIVATE_DATA_ROOT_MISMATCH); + builder.do_assert(root_for_read_request == historic_private_data_tree_root, + format("private data tree root mismatch at read_request[", + rr_idx, + "]", + "\n\texpected root: ", + historic_private_data_tree_root, + "\n\tbut got root*: ", + root_for_read_request, + "\n\tread_request: ", + read_request, + "\n\tsiloed-rr (leaf): ", + leaf, + "\n\tleaf_index: ", + witness.leaf_index, + "\n\tis_transient: ", + witness.is_transient, + "\n\thint_to_commitment: ", + witness.hint_to_commitment, + "\n\t* got root by siloing read_request (compressing with " + "storage_contract_address to get leaf) " + "and merkle-hashing to a root using membership witness"), + CircuitErrorCode::PRIVATE_KERNEL__READ_REQUEST_PRIVATE_DATA_ROOT_MISMATCH); } } } + +/** + * @brief Ensure that all read requests from previous kernel are transient. + * + * @param builder + * @param read_requests from previous kernel's public inputs + * @param read_request_membership_witnesses from previous kernel's public inputs + */ +void common_validate_previous_kernel_read_requests( + DummyBuilder& builder, + std::array const& read_requests, + std::array, MAX_READ_REQUESTS_PER_TX> const& + read_request_membership_witnesses) +{ + for (size_t rr_idx = 0; rr_idx < MAX_READ_REQUESTS_PER_TX; rr_idx++) { + const auto& read_request = read_requests[rr_idx]; + const auto& witness = read_request_membership_witnesses[rr_idx]; + builder.do_assert(read_request == 0 || witness.is_transient, // rr == 0 means empty + format("Previous kernel's read request[", + rr_idx, + "] is not transient, but kernel should only forward transient reads.", + "\n\tread_request: ", + read_request, + "\n\tleaf_index: ", + witness.leaf_index, + "\n\tis_transient: ", + witness.is_transient, + "\n\thint_to_commitment: ", + witness.hint_to_commitment), + CircuitErrorCode::PRIVATE_KERNEL__UNRESOLVED_NON_TRANSIENT_READ_REQUEST); + } +} + void common_update_end_values(DummyBuilder& builder, PrivateCallData const& private_call, KernelCircuitPublicInputs& public_inputs) @@ -147,17 +187,19 @@ void common_update_end_values(DummyBuilder& builder, const auto& storage_contract_address = private_call_public_inputs.call_context.storage_contract_address; - // Read requests and witnessess to be accumulated in public_inputs.end + // Transient read requests and witnessess are accumulated in public_inputs.end // We silo the read requests (domain separation per contract address) { - std::array siloed_read_requests; for (size_t i = 0; i < read_requests.size(); ++i) { - siloed_read_requests[i] = - read_requests[i] == 0 ? 0 : silo_commitment(storage_contract_address, read_requests[i]); + const auto& read_request = read_requests[i]; + const auto& witness = read_request_membership_witnesses[i]; + if (witness.is_transient) { // only forward transient to public inputs + const auto siloed_read_request = + read_request == 0 ? 0 : silo_commitment(storage_contract_address, read_request); + array_push(builder, public_inputs.end.read_requests, siloed_read_request); + array_push(builder, public_inputs.end.read_request_membership_witnesses, witness); + } } - push_array_to_array(builder, siloed_read_requests, public_inputs.end.read_requests); - push_array_to_array( - builder, read_request_membership_witnesses, public_inputs.end.read_request_membership_witnesses); } // Enhance commitments and nullifiers with domain separation whereby domain is the contract. @@ -315,19 +357,16 @@ void common_contract_logic(DummyBuilder& builder, } } -void common_initialise_end_values(PreviousKernelData const& previous_kernel, - KernelCircuitPublicInputs& public_inputs) +void common_inner_ordering_initialise_end_values(PreviousKernelData const& previous_kernel, + KernelCircuitPublicInputs& public_inputs) { public_inputs.constants = previous_kernel.public_inputs.constants; - // Ensure the arrays are the same as previously, before we start pushing more data onto them in other functions - // within this circuit: + // Ensure the arrays are the same as previously, before we start pushing more data onto them in other + // functions within this circuit: auto& end = public_inputs.end; const auto& start = previous_kernel.public_inputs.end; - end.read_requests = start.read_requests; - end.read_request_membership_witnesses = start.read_request_membership_witnesses; - end.new_commitments = start.new_commitments; end.new_nullifiers = start.new_nullifiers; diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp b/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp index 0a7c80bbedc..57256896e19 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp @@ -27,10 +27,16 @@ void common_validate_call_stack(DummyBuilder& builder, PrivateCallData const void common_validate_read_requests(DummyBuilder& builder, NT::fr const& storage_contract_address, + NT::fr const& historic_private_data_tree_root, std::array const& read_requests, std::array, - MAX_READ_REQUESTS_PER_CALL> const& read_request_membership_witnesses, - NT::fr const& historic_private_data_tree_root); + MAX_READ_REQUESTS_PER_CALL> const& read_request_membership_witnesses); + +void common_validate_previous_kernel_read_requests( + DummyBuilder& builder, + std::array const& read_requests, + std::array, MAX_READ_REQUESTS_PER_TX> const& + read_request_membership_witnesses); void common_update_end_values(DummyBuilder& builder, PrivateCallData const& private_call, @@ -42,6 +48,7 @@ void common_contract_logic(DummyBuilder& builder, ContractDeploymentData const& contract_dep_data, FunctionData const& function_data); -void common_initialise_end_values(PreviousKernelData const& previous_kernel, - KernelCircuitPublicInputs& public_inputs); +void common_inner_ordering_initialise_end_values(PreviousKernelData const& previous_kernel, + KernelCircuitPublicInputs& public_inputs); + } // namespace aztec3::circuits::kernel::private_kernel \ No newline at end of file diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit.test.cpp index 93bc67c2aaa..8e851bc7328 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit.test.cpp @@ -39,8 +39,8 @@ class native_private_kernel_tests : public ::testing::Test { // 1. We send transient read request on value 23 and pending commitment 12 // 2. We send transient read request on value 12 and pending commitment 23 -// We expect both read requests and commitments to be completely chopped by the ordering circuit. -TEST_F(native_private_kernel_tests, native_accumulate_transient_read_requests_and_choping_works) +// We expect both read requests and commitments to be successfully matched in ordering circuit. +TEST_F(native_private_kernel_tests, native_accumulate_transient_read_requests) { auto private_inputs_init = do_private_call_get_kernel_inputs_init(false, deposit, standard_test_args()); @@ -48,8 +48,7 @@ TEST_F(native_private_kernel_tests, native_accumulate_transient_read_requests_an private_inputs_init.private_call.call_stack_item.public_inputs.read_requests[0] = fr(23); private_inputs_init.private_call.read_request_membership_witnesses[0].is_transient = true; - DummyBuilder builder = - DummyBuilder("native_private_kernel_tests__native_accumulate_transient_read_requests_and_choping_works"); + DummyBuilder builder = DummyBuilder("native_private_kernel_tests__native_accumulate_transient_read_requests"); auto public_inputs = native_private_kernel_circuit_initial(builder, private_inputs_init); ASSERT_FALSE(builder.failed()) << "failure: " << builder.get_first_failure() @@ -83,9 +82,11 @@ TEST_F(native_private_kernel_tests, native_accumulate_transient_read_requests_an ASSERT_FALSE(builder.failed()) << "failure: " << builder.get_first_failure() << " with code: " << builder.get_first_failure().code; - ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 0); - ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 2); - ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 2); + ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 2); // no commitments squashed + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1074): read_request*s + // can be removed from final public inputs + ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 0); + ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 0); } // 1. We send transient read request on value 23 and pending commitment 10 @@ -134,9 +135,11 @@ TEST_F(native_private_kernel_tests, native_transient_read_requests_no_match) ASSERT_TRUE(builder.failed()); ASSERT_TRUE(builder.get_first_failure().code == CircuitErrorCode::PRIVATE_KERNEL__TRANSIENT_READ_REQUEST_NO_MATCH); - ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 1); - ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 2); - ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 2); + ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 2); // no commitments squashed + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1074): read_request*s + // can be removed from final public inputs + ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 0); + ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 0); } } // namespace aztec3::circuits::kernel::private_kernel 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 2601be67849..c055654342e 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 @@ -186,9 +186,9 @@ KernelCircuitPublicInputs native_private_kernel_circuit_initial(DummyBuilder common_validate_read_requests( builder, private_inputs.private_call.call_stack_item.public_inputs.call_context.storage_contract_address, - private_inputs.private_call.call_stack_item.public_inputs.read_requests, - private_inputs.private_call.read_request_membership_witnesses, - public_inputs.constants.historic_tree_roots.private_historic_tree_roots.private_data_tree_root); + public_inputs.constants.historic_tree_roots.private_historic_tree_roots.private_data_tree_root, + private_inputs.private_call.call_stack_item.public_inputs.read_requests, // read requests from private call + private_inputs.private_call.read_request_membership_witnesses); // TODO(dbanks12): feels like update_end_values should happen after contract logic update_end_values(builder, private_inputs, public_inputs); 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 1732df9c264..f20be341316 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 @@ -23,6 +23,8 @@ using aztec3::circuits::kernel::private_kernel::testing_harness::do_private_call using aztec3::circuits::kernel::private_kernel::testing_harness::get_random_reads; using aztec3::circuits::kernel::private_kernel::testing_harness::validate_deployed_contract_address; using aztec3::circuits::kernel::private_kernel::testing_harness::validate_no_new_deployed_contract; + +using aztec3::utils::array_length; using aztec3::utils::CircuitErrorCode; @@ -447,6 +449,9 @@ TEST_F(native_private_kernel_init_tests, native_no_read_requests_works) // Check the first nullifier is hash of the signed tx request ASSERT_EQ(public_inputs.end.new_nullifiers[0], private_inputs.tx_request.hash()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_init_tests, native_one_read_requests_works) @@ -476,6 +481,9 @@ TEST_F(native_private_kernel_init_tests, native_one_read_requests_works) // Check the first nullifier is hash of the signed tx request ASSERT_EQ(public_inputs.end.new_nullifiers[0], private_inputs.tx_request.hash()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_init_tests, native_two_read_requests_works) @@ -505,6 +513,9 @@ TEST_F(native_private_kernel_init_tests, native_two_read_requests_works) // Check the first nullifier is hash of the signed tx request ASSERT_EQ(public_inputs.end.new_nullifiers[0], private_inputs.tx_request.hash()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_init_tests, native_max_read_requests_works) @@ -535,6 +546,9 @@ TEST_F(native_private_kernel_init_tests, native_max_read_requests_works) // Check the first nullifier is hash of the signed tx request ASSERT_EQ(public_inputs.end.new_nullifiers[0], private_inputs.tx_request.hash()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } // TODO(dbanks12): more tests of read_requests for multiple iterations. @@ -618,6 +632,8 @@ TEST_F(native_private_kernel_init_tests, native_one_transient_read_requests_work info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + ASSERT_EQ(array_length(public_inputs.end.read_requests), 1); // transient read request gets forwarded } // TODO(https://github.com/AztecProtocol/aztec-packages/issues/906): re-enable once kernel supports forwarding/matching @@ -625,7 +641,6 @@ TEST_F(native_private_kernel_init_tests, native_one_transient_read_requests_work TEST_F(native_private_kernel_init_tests, native_max_read_requests_one_transient_works) { // max read requests with one transient should work - auto private_inputs = do_private_call_get_kernel_inputs_init(false, deposit, standard_test_args()); auto const& contract_address = @@ -653,6 +668,47 @@ TEST_F(native_private_kernel_init_tests, native_max_read_requests_one_transient_ info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // transient read request gets forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 1); +} + +TEST_F(native_private_kernel_init_tests, native_max_read_requests_all_transient_works) +{ + // max read requests with all transient should work + auto private_inputs = do_private_call_get_kernel_inputs_init(false, deposit, standard_test_args()); + + auto const& contract_address = + private_inputs.private_call.call_stack_item.public_inputs.call_context.storage_contract_address; + + auto [read_requests, read_request_membership_witnesses, root] = + get_random_reads(contract_address, MAX_READ_REQUESTS_PER_CALL); + private_inputs.private_call.call_stack_item.public_inputs.historic_private_data_tree_root = root; + private_inputs.private_call.call_stack_item.public_inputs.read_requests = read_requests; + + + // Make the read request at position 1 transient + for (size_t rr_idx = 0; rr_idx < MAX_READ_REQUESTS_PER_CALL; ++rr_idx) { + read_request_membership_witnesses[rr_idx].leaf_index = NT::fr(0); + read_request_membership_witnesses[rr_idx].sibling_path = std::array{}; + read_request_membership_witnesses[rr_idx].is_transient = true; + } + private_inputs.private_call.read_request_membership_witnesses = read_request_membership_witnesses; + + DummyBuilder builder = + DummyBuilder("native_private_kernel_init_tests__native_max_read_requests_all_transient_works"); + auto const& public_inputs = native_private_kernel_circuit_initial(builder, private_inputs); + + validate_no_new_deployed_contract(public_inputs); + + auto failure = builder.get_first_failure(); + if (failure.code != CircuitErrorCode::NO_ERROR) { + info("failure: ", failure); + } + ASSERT_FALSE(builder.failed()); + + // transient read request all get forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), MAX_READ_REQUESTS_PER_CALL); } } // namespace aztec3::circuits::kernel::private_kernel 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 776f6139807..b999c561a4a 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 @@ -41,6 +41,19 @@ using CircuitErrorCode = aztec3::utils::CircuitErrorCode; // return aggregation_object; // } +void initialise_end_values(PreviousKernelData const& previous_kernel, KernelCircuitPublicInputs& public_inputs) +{ + common_inner_ordering_initialise_end_values(previous_kernel, public_inputs); + + // Ensure the arrays are the same as previously, before we start pushing more data onto them in other + // functions within this circuit: + auto& end = public_inputs.end; + const auto& start = previous_kernel.public_inputs.end; + + end.read_requests = start.read_requests; + end.read_request_membership_witnesses = start.read_request_membership_witnesses; +} + void validate_this_private_call_hash(DummyBuilder& builder, PrivateKernelInputsInner const& private_inputs, @@ -94,6 +107,9 @@ void validate_inputs(DummyBuilder& builder, PrivateKernelInputsInner const& builder.do_assert(start_private_call_stack_length != 0, "Cannot execute private kernel circuit with an empty private call stack", CircuitErrorCode::PRIVATE_KERNEL__PRIVATE_CALL_STACK_EMPTY); + + common_validate_previous_kernel_read_requests( + builder, start.read_requests, start.read_request_membership_witnesses); } // NOTE: THIS IS A VERY UNFINISHED WORK IN PROGRESS. @@ -106,7 +122,7 @@ KernelCircuitPublicInputs native_private_kernel_circuit_inner(DummyBuilder& KernelCircuitPublicInputs public_inputs{}; // Do this before any functions can modify the inputs. - common_initialise_end_values(private_inputs.previous_kernel, public_inputs); + initialise_end_values(private_inputs.previous_kernel, public_inputs); validate_inputs(builder, private_inputs); @@ -122,9 +138,9 @@ KernelCircuitPublicInputs native_private_kernel_circuit_inner(DummyBuilder& common_validate_read_requests( builder, private_inputs.private_call.call_stack_item.public_inputs.call_context.storage_contract_address, - private_inputs.private_call.call_stack_item.public_inputs.read_requests, - private_inputs.private_call.read_request_membership_witnesses, - public_inputs.constants.historic_tree_roots.private_historic_tree_roots.private_data_tree_root); + public_inputs.constants.historic_tree_roots.private_historic_tree_roots.private_data_tree_root, + private_inputs.private_call.call_stack_item.public_inputs.read_requests, // read requests from private call + private_inputs.private_call.read_request_membership_witnesses); // TODO(dbanks12): feels like update_end_values should happen later 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 149db657ca9..c8f6dca12f1 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 @@ -5,6 +5,7 @@ #include "aztec3/circuits/apps/test_apps/escrow/deposit.hpp" #include "aztec3/circuits/kernel/private/common.hpp" #include "aztec3/constants.hpp" +#include "aztec3/utils/array.hpp" #include "aztec3/utils/circuit_errors.hpp" #include @@ -21,6 +22,8 @@ using aztec3::circuits::apps::test_apps::escrow::deposit; using aztec3::circuits::kernel::private_kernel::testing_harness::do_private_call_get_kernel_inputs_inner; using aztec3::circuits::kernel::private_kernel::testing_harness::get_random_reads; using aztec3::circuits::kernel::private_kernel::testing_harness::validate_no_new_deployed_contract; + +using aztec3::utils::array_length; using aztec3::utils::CircuitErrorCode; // NOTE: *DO NOT* call fr constructors in static initializers and assign them to constants. This will fail. Instead, use @@ -359,6 +362,9 @@ TEST_F(native_private_kernel_inner_tests, native_no_read_requests_works) info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_inner_tests, native_one_read_requests_works) @@ -387,6 +393,9 @@ TEST_F(native_private_kernel_inner_tests, native_one_read_requests_works) info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_inner_tests, native_two_read_requests_works) @@ -415,6 +424,9 @@ TEST_F(native_private_kernel_inner_tests, native_two_read_requests_works) info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_inner_tests, native_max_read_requests_works) @@ -444,6 +456,9 @@ TEST_F(native_private_kernel_inner_tests, native_max_read_requests_works) info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // non-transient read requests are NOT forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 0); } TEST_F(native_private_kernel_inner_tests, native_read_requests_less_than_witnesses) @@ -527,6 +542,8 @@ TEST_F(native_private_kernel_inner_tests, native_one_transient_read_requests_wor info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + ASSERT_EQ(array_length(public_inputs.end.read_requests), 1); // transient read request gets forwarded } TEST_F(native_private_kernel_inner_tests, native_max_read_requests_one_transient_works) @@ -562,6 +579,49 @@ TEST_F(native_private_kernel_inner_tests, native_max_read_requests_one_transient info("failure: ", failure); } ASSERT_FALSE(builder.failed()); + + // transient read request gets forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), 1); +} + +TEST_F(native_private_kernel_inner_tests, native_max_read_requests_all_transient_works) +{ + // max read requests with all transient should work + + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + auto const& contract_address = + private_inputs.private_call.call_stack_item.public_inputs.call_context.storage_contract_address; + + auto [read_requests, read_request_membership_witnesses, root] = + get_random_reads(contract_address, MAX_READ_REQUESTS_PER_CALL); + private_inputs.previous_kernel.public_inputs.constants.historic_tree_roots.private_historic_tree_roots + .private_data_tree_root = root; + private_inputs.private_call.call_stack_item.public_inputs.historic_private_data_tree_root = root; + private_inputs.private_call.call_stack_item.public_inputs.read_requests = read_requests; + + // Make the read request at position 1 transient + for (size_t rr_idx = 0; rr_idx < MAX_READ_REQUESTS_PER_CALL; ++rr_idx) { + read_request_membership_witnesses[rr_idx].leaf_index = NT::fr(0); + read_request_membership_witnesses[rr_idx].sibling_path = std::array{}; + read_request_membership_witnesses[rr_idx].is_transient = true; + } + private_inputs.private_call.read_request_membership_witnesses = read_request_membership_witnesses; + + DummyBuilder builder = + DummyBuilder("native_private_kernel_inner_tests__native_max_read_requests_one_transient_works"); + auto const& public_inputs = native_private_kernel_circuit_inner(builder, private_inputs); + + validate_no_new_deployed_contract(public_inputs); + + auto failure = builder.get_first_failure(); + if (failure.code != CircuitErrorCode::NO_ERROR) { + info("failure: ", failure); + } + ASSERT_FALSE(builder.failed()); + + // transient read request all get forwarded + ASSERT_EQ(array_length(public_inputs.end.read_requests), MAX_READ_REQUESTS_PER_CALL); } TEST_F(native_private_kernel_inner_tests, native_logs_are_hashed_as_expected) diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.cpp index e16ce0b429e..1e65a349b31 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.cpp @@ -4,6 +4,7 @@ #include "aztec3/circuits/abis/kernel_circuit_public_inputs.hpp" #include "aztec3/circuits/abis/previous_kernel_data.hpp" #include "aztec3/constants.hpp" +#include "aztec3/utils/array.hpp" #include "aztec3/utils/circuit_errors.hpp" #include "aztec3/utils/dummy_circuit_builder.hpp" @@ -14,47 +15,139 @@ namespace aztec3::circuits::kernel::private_kernel { using aztec3::circuits::abis::KernelCircuitPublicInputs; using aztec3::circuits::abis::PreviousKernelData; +using aztec3::utils::array_length; using DummyBuilder = aztec3::utils::DummyCircuitBuilder; using CircuitErrorCode = aztec3::utils::CircuitErrorCode; -// TODO(jeanmon): the following code will be optimized based on hints regarding matching -// a read request and commitment, i.e., we get pairs i,j such that read_requests[i] == new_commitments[j] -// Relevant task: https://github.com/AztecProtocol/aztec-packages/issues/892 -void chop_pending_commitments(DummyBuilder& builder, - std::array const& read_requests, - std::array, - MAX_READ_REQUESTS_PER_TX> const& read_request_membership_witnesses, - std::array& new_commitments) +// TODO(https://github.com/AztecProtocol/aztec-packages/issues/892): optimized based on hints +// regarding matching a read request to a commitment +// i.e., we get pairs i,j such that read_requests[i] == new_commitments[j] +void match_reads_to_commitments(DummyBuilder& builder, + std::array const& read_requests, + std::array, + MAX_READ_REQUESTS_PER_TX> const& read_request_membership_witnesses, + std::array& new_commitments) { - // chop commitments from the previous call(s) - for (size_t i = 0; i < MAX_READ_REQUESTS_PER_TX; i++) { - const auto& read_request = read_requests[i]; - const auto is_transient_read = read_request_membership_witnesses[i].is_transient; + // Arrays read_request and read_request_membership_witnesses must be of the same length. Otherwise, + // we might get into trouble when accumulating them in public_inputs.end + builder.do_assert(array_length(read_requests) == array_length(read_request_membership_witnesses), + format("[private ordering circuit] mismatch array length between read_requests and witnesses - " + "read_requests length: ", + array_length(read_requests), + " witnesses length: ", + array_length(read_request_membership_witnesses)), + CircuitErrorCode::PRIVATE_KERNEL__READ_REQUEST_WITNESSES_ARRAY_LENGTH_MISMATCH); + + // match reads to commitments from the previous call(s) + for (size_t rr_idx = 0; rr_idx < MAX_READ_REQUESTS_PER_TX; rr_idx++) { + const auto& read_request = read_requests[rr_idx]; + const auto& witness = read_request_membership_witnesses[rr_idx]; + const auto is_transient_read = witness.is_transient; + const auto& hint_to_commitment = witness.hint_to_commitment; + if (is_transient_read) { size_t match_pos = MAX_NEW_COMMITMENTS_PER_TX; - for (size_t j = 0; j < MAX_NEW_COMMITMENTS_PER_TX; j++) { - match_pos = (read_request == new_commitments[j]) ? j : match_pos; + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/892): inefficient + // O(n^2) inner loop will be optimized via matching hints + for (size_t c_idx = 0; c_idx < MAX_NEW_COMMITMENTS_PER_TX; c_idx++) { + match_pos = (read_request == new_commitments[c_idx]) ? c_idx : match_pos; } - // chop the pending commitment, i.e., replacing with 0. - // TODO(jeanmon): In addition, we will NOT chop if this is only a read request action without nullifiers. - if (match_pos != MAX_NEW_COMMITMENTS_PER_TX) { - new_commitments[match_pos] = fr(0); - } else { - builder.do_assert( - false, - format("transient read request at position [", i, "] does not match any new commitment"), - CircuitErrorCode::PRIVATE_KERNEL__TRANSIENT_READ_REQUEST_NO_MATCH); - } + // Transient reads MUST match a pending commitment + builder.do_assert( + match_pos != MAX_NEW_COMMITMENTS_PER_TX, + format("read_request at position [", + rr_idx, + "]* is transient but does not match any new commitment.", + "\n\tread_request: ", + read_request, + "\n\tis_transient: ", + is_transient_read, + "\n\thint_to_commitment: ", + hint_to_commitment, + "\n\t* the read_request position/index is not expected to match position in app-circuit " + "outputs because kernel iterations gradually remove non-transient read_requests as " + "membership checks are resolved."), + CircuitErrorCode::PRIVATE_KERNEL__TRANSIENT_READ_REQUEST_NO_MATCH); + } else { + // This if-condition means it is a non-empty read request and it is flagged as transient.... + // NON-transient reads MUST be membership-checked and removed during standard kernel iterations + // NONE should be here in (let alone output from) the ordering circuit. + builder.do_assert( + read_request == NT::fr(0), // basically: assert(is_transient_read || empty) + format("read_request at position [", + rr_idx, + "]* is NOT transient but is still unresolved in the final kernel stage! This implies invalid " + "inputs " + "to the final (ordering) stage of the kernel.", + "\n\tread_request: ", + read_request, + "\n\tleaf_index: ", + witness.leaf_index, + "\n\tis_transient: ", + is_transient_read, + "\n\thint_to_commitment: ", + hint_to_commitment, + "\n\t* the read_request position/index is not expected to match position in app-circuit " + "outputs because kernel iterations gradually remove non-transient read_requests as " + "membership checks are resolved."), + CircuitErrorCode::PRIVATE_KERNEL__UNRESOLVED_NON_TRANSIENT_READ_REQUEST); } } - - // Move all zero entries of this array to the end and preserve ordering of other entries - utils::array_rearrange(new_commitments); } +// TODO(https://github.com/AztecProtocol/aztec-packages/issues/836): match_nullifiers_to_commitments_and_squash +// TODO(https://github.com/AztecProtocol/aztec-packages/issues/837): optimized based on hints +// regarding matching a nullifier to a commitment +// i.e., we get pairs i,j such that new_nullifiers[i] == new_commitments[j] +// void match_nullifiers_to_commitments_and_squash( +// DummyBuilder& builder, +// std::array& new_nullifiers, +// // std::array const& nullifier_hints, // ??? +// std::array& new_commitments) +//{ +// // match reads to commitments from the previous call(s) +// for (size_t n_idx = 0; n_idx < MAX_NEW_NULLIFIERS_PER_TX; n_idx++) { +// const auto& nullifier = new_nullifiers[n_idx]; // new_nullifiers[n_idx].nullifier +// const auto& nullified_commitment = NT::fr(0); // new_nullifiers[n_idx].commitment +// const auto is_transient_nullifier = false; // nullifier_hints[n_idx].is_transient; +// // const auto& hint_to_commitment = nullifier_hints[n_idx].hint_to_commitment; +// +// if (is_transient_nullifier) { +// size_t match_pos = MAX_NEW_COMMITMENTS_PER_TX; +// // TODO(https://github.com/AztecProtocol/aztec-packages/issues/837): inefficient +// // O(n^2) inner loop will be optimized via matching hints +// for (size_t c_idx = 0; c_idx < MAX_NEW_COMMITMENTS_PER_TX; c_idx++) { +// match_pos = (nullified_commitment == new_commitments[c_idx]) ? c_idx : match_pos; +// } +// +// if (match_pos != MAX_NEW_COMMITMENTS_PER_TX) { +// new_commitments[match_pos] = fr(0); +// } else { +// // Transient nullifiers MUST match a pending commitment +// builder.do_assert(false, // match_pos != MAX_NEW_COMMITMENTS_PER_TX, +// format("new_nullifier at position [", +// n_idx, +// "]* is transient but does not match any new commitment.", +// "\n\tnullifier: ", +// nullifier, +// "\n\tnullified_commitment: ", +// nullified_commitment), +// //"\n\thint_to_commitment: ", +// // hint_to_commitment, +// CircuitErrorCode::PRIVATE_KERNEL__TRANSIENT_NEW_NULLIFIER_NO_MATCH); +// } +// } +// // non-transient nullifiers are just kept in new_nullifiers array and forwarded to +// // public inputs (used later by base rollup circuit) +// } +// // Move all zero-ed (removed) entries of these arrays to the end and preserve ordering of other entries +// array_rearrange(new_commitments); +// array_rearrange(new_nullifiers); +//} + KernelCircuitPublicInputs native_private_kernel_circuit_ordering(DummyBuilder& builder, PreviousKernelData const& previous_kernel) { @@ -62,15 +155,32 @@ KernelCircuitPublicInputs native_private_kernel_circuit_ordering(DummyBuilde KernelCircuitPublicInputs public_inputs{}; // Do this before any functions can modify the inputs. - common_initialise_end_values(previous_kernel, public_inputs); + common_inner_ordering_initialise_end_values(previous_kernel, public_inputs); + + common_validate_previous_kernel_read_requests(builder, + previous_kernel.public_inputs.end.read_requests, + previous_kernel.public_inputs.end.read_request_membership_witnesses); + + // Matching read requests to pending commitments requires the full list of new commitments accumulated over + // all iterations of the private kernel. Therefore, we match reads against new_commitments in + // previous_kernel.public_inputs.end, where "previous kernel" is the last "inner" kernel iteration. + // Remark: The commitments in public_inputs.end have already been SILOED! + match_reads_to_commitments(builder, + previous_kernel.public_inputs.end.read_requests, + previous_kernel.public_inputs.end.read_request_membership_witnesses, + public_inputs.end.new_commitments); + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1074): ideally final public_inputs + // shouldn't even include read_requests and read_request_membership_witnesses as they should be empty. - // Removing of nullified pending commitments have to happen on the list of commitments which have been accumulated - // over all iterations of the private kernel. Therefore, we have to target commitments in public_inputs.end + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/836): match_nullifiers_to_commitments_and_squash + // Matching nullifiers to pending commitments requires the full list of new commitments accumulated over + // all iterations of the private kernel. Therefore, we match reads against new_commitments in public_inputs.end + // which has been initialized to previous_kernel.public_inputs.end in common_initialise_*() above. // Remark: The commitments in public_inputs.end have already been SILOED! - chop_pending_commitments(builder, - public_inputs.end.read_requests, - public_inputs.end.read_request_membership_witnesses, - public_inputs.end.new_commitments); + // match_nullifiers_to_commitments_and_squash(builder, + // public_inputs.end.new_nullifiers, + // // public_inputs.end.nullifier_hints, + // public_inputs.end.new_commitments); return public_inputs; }; diff --git a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.test.cpp b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.test.cpp index 43bd5719460..651ce656994 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.test.cpp +++ b/circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.test.cpp @@ -29,7 +29,7 @@ class native_private_kernel_ordering_tests : public ::testing::Test { static void SetUpTestSuite() { barretenberg::srs::init_crs_factory("../barretenberg/cpp/srs_db/ignition"); } }; -TEST_F(native_private_kernel_ordering_tests, native_one_read_request_choping_commitment_works) +TEST_F(native_private_kernel_ordering_tests, native_matching_one_read_request_to_commitment_works) { auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); @@ -49,7 +49,7 @@ TEST_F(native_private_kernel_ordering_tests, native_one_read_request_choping_com DummyBuilder builder = - DummyBuilder("native_private_kernel_ordering_tests__native_one_read_request_choping_commitment_works"); + DummyBuilder("native_private_kernel_ordering_tests__native_matching_one_read_request_to_commitment_works"); auto const& public_inputs = native_private_kernel_circuit_ordering(builder, private_inputs.previous_kernel); auto failure = builder.get_first_failure(); @@ -57,10 +57,15 @@ TEST_F(native_private_kernel_ordering_tests, native_one_read_request_choping_com info("failure: ", failure); } ASSERT_FALSE(builder.failed()); - ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 0); + ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 1); + ASSERT_TRUE(public_inputs.end.new_commitments[0] == fr(1282)); + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1074): read_request*s + // can be removed from final public inputs + ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 0); + ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 0); } -TEST_F(native_private_kernel_ordering_tests, native_read_requests_choping_commitment_works) +TEST_F(native_private_kernel_ordering_tests, native_matching_some_read_requests_to_commitments_works) { auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); @@ -86,7 +91,7 @@ TEST_F(native_private_kernel_ordering_tests, native_read_requests_choping_commit read_request_membership_witnesses; DummyBuilder builder = - DummyBuilder("native_private_kernel_ordering_tests__native_read_requests_choping_commitment_works"); + DummyBuilder("native_private_kernel_ordering_tests__native_matching_some_read_requests_to_commitments_works"); auto const& public_inputs = native_private_kernel_circuit_ordering(builder, private_inputs.previous_kernel); auto failure = builder.get_first_failure(); @@ -94,9 +99,15 @@ TEST_F(native_private_kernel_ordering_tests, native_read_requests_choping_commit info("failure: ", failure); } ASSERT_FALSE(builder.failed()); - ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 2); + ASSERT_TRUE(array_length(public_inputs.end.new_commitments) == 4); ASSERT_TRUE(public_inputs.end.new_commitments[0] == fr(1285)); - ASSERT_TRUE(public_inputs.end.new_commitments[1] == fr(1282)); + ASSERT_TRUE(public_inputs.end.new_commitments[1] == fr(1283)); + ASSERT_TRUE(public_inputs.end.new_commitments[2] == fr(1282)); + ASSERT_TRUE(public_inputs.end.new_commitments[3] == fr(1284)); + // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1074): read_request*s + // can be removed from final public inputs + ASSERT_TRUE(array_length(public_inputs.end.read_requests) == 0); + ASSERT_TRUE(array_length(public_inputs.end.read_request_membership_witnesses) == 0); } TEST_F(native_private_kernel_ordering_tests, native_read_request_unknown_fails) @@ -136,4 +147,33 @@ TEST_F(native_private_kernel_ordering_tests, native_read_request_unknown_fails) ASSERT_EQ(failure.code, CircuitErrorCode::PRIVATE_KERNEL__TRANSIENT_READ_REQUEST_NO_MATCH); } +TEST_F(native_private_kernel_ordering_tests, native_unresolved_non_transient_read_fails) +{ + auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args()); + + std::array new_commitments{}; + std::array read_requests{}; + std::array, MAX_READ_REQUESTS_PER_TX> + read_request_membership_witnesses{}; + + new_commitments[0] = fr(1285); + + + read_requests[0] = fr(1285); + read_request_membership_witnesses[0].is_transient = false; // ordering circuit only allows transient reads + + private_inputs.previous_kernel.public_inputs.end.new_commitments = new_commitments; + private_inputs.previous_kernel.public_inputs.end.read_requests = read_requests; + private_inputs.previous_kernel.public_inputs.end.read_request_membership_witnesses = + read_request_membership_witnesses; + + + DummyBuilder builder = + DummyBuilder("native_private_kernel_ordering_tests__native_unresolved_non_transient_read_fails"); + native_private_kernel_circuit_ordering(builder, private_inputs.previous_kernel); + + auto failure = builder.get_first_failure(); + ASSERT_EQ(failure.code, CircuitErrorCode::PRIVATE_KERNEL__UNRESOLVED_NON_TRANSIENT_READ_REQUEST); +} + } // namespace aztec3::circuits::kernel::private_kernel diff --git a/circuits/cpp/src/aztec3/utils/array.hpp b/circuits/cpp/src/aztec3/utils/array.hpp index e0b5f0fb06c..a1b120f4581 100644 --- a/circuits/cpp/src/aztec3/utils/array.hpp +++ b/circuits/cpp/src/aztec3/utils/array.hpp @@ -110,15 +110,26 @@ template void array_rearrange(std::array& arr { size_t target_pos = 0; for (size_t i = 0; i < SIZE; i++) { - if (arr[i] != NT::fr(0)) { - arr[target_pos] = arr[i]; - target_pos++; + if constexpr (std::is_same::value) { + if (arr[i] != NT::fr(0)) { + arr[target_pos] = arr[i]; + target_pos++; + } + } else { + if (!arr[i].is_empty()) { + arr[target_pos] = arr[i]; + target_pos++; + } } } // Cleaning needed to avoid duplicate values, e.g., [1,0,3,0] --> [1,3,3,0] otherwise. for (size_t i = target_pos; i < SIZE; i++) { - arr[i] = NT::fr(0); + if constexpr (std::is_same::value) { + arr[i] = NT::fr(0); + } else { + arr[i] = T{}; + } } } diff --git a/circuits/cpp/src/aztec3/utils/circuit_errors.hpp b/circuits/cpp/src/aztec3/utils/circuit_errors.hpp index e6971982b19..f120320cd71 100644 --- a/circuits/cpp/src/aztec3/utils/circuit_errors.hpp +++ b/circuits/cpp/src/aztec3/utils/circuit_errors.hpp @@ -31,6 +31,7 @@ enum CircuitErrorCode : uint16_t { PRIVATE_KERNEL__READ_REQUEST_PRIVATE_DATA_ROOT_MISMATCH = 2018, PRIVATE_KERNEL__TRANSIENT_READ_REQUEST_NO_MATCH = 2019, PRIVATE_KERNEL__READ_REQUEST_WITNESSES_ARRAY_LENGTH_MISMATCH = 2020, + PRIVATE_KERNEL__UNRESOLVED_NON_TRANSIENT_READ_REQUEST = 2021, // Public kernel related errors PUBLIC_KERNEL_CIRCUIT_FAILED = 3000,