Skip to content

Commit

Permalink
fix(kernel): dont chop commitments when matched to transient reads (#…
Browse files Browse the repository at this point in the history 
…1079)

* dont chop reads

* remove accidentally added scrap files

* various fixes and cleanup - don't forward non-transient reads to next kernel

* remove log

* tests that only transient reads get forwarded to next kernel

* revert minor diff

* 1073 - jeanmon review's comment

---------

Co-authored-by: jeanmon <jean@aztecprotocol.com>
  • Loading branch information
@dbanks12 @jeanmon
dbanks12 and jeanmon authored Jul 18, 2023
1 parent d48c2e6 commit fdf41ed
Show file tree
Hide file tree
Showing 11 changed files with 447 additions and 104 deletions.
113 changes: 76 additions & 37 deletions circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,24 +61,25 @@ void common_validate_call_stack(DummyBuilder& builder, PrivateCallData<NT> 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<fr, MAX_READ_REQUESTS_PER_CALL> const& read_requests,
std::array<ReadRequestMembershipWitness<NT, PRIVATE_DATA_TREE_HEIGHT>,
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)),
Expand All @@ -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<NT>(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<NT::fr, MAX_READ_REQUESTS_PER_TX> const& read_requests,
std::array<ReadRequestMembershipWitness<NT, PRIVATE_DATA_TREE_HEIGHT>, 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<NT> const& private_call,
KernelCircuitPublicInputs<NT>& public_inputs)
Expand All @@ -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<NT::fr, MAX_READ_REQUESTS_PER_CALL> siloed_read_requests;
for (size_t i = 0; i < read_requests.size(); ++i) {
siloed_read_requests[i] =
read_requests[i] == 0 ? 0 : silo_commitment<NT>(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<NT>(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.
Expand Down Expand Up @@ -315,19 +357,16 @@ void common_contract_logic(DummyBuilder& builder,
}
}

void common_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
KernelCircuitPublicInputs<NT>& public_inputs)
void common_inner_ordering_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
KernelCircuitPublicInputs<NT>& 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;

Expand Down
15 changes: 11 additions & 4 deletions circuits/cpp/src/aztec3/circuits/kernel/private/common.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,16 @@ void common_validate_call_stack(DummyBuilder& builder, PrivateCallData<NT> const

void common_validate_read_requests(DummyBuilder& builder,
NT::fr const& storage_contract_address,
NT::fr const& historic_private_data_tree_root,
std::array<fr, MAX_READ_REQUESTS_PER_CALL> const& read_requests,
std::array<ReadRequestMembershipWitness<NT, PRIVATE_DATA_TREE_HEIGHT>,
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<NT::fr, MAX_READ_REQUESTS_PER_TX> const& read_requests,
std::array<ReadRequestMembershipWitness<NT, PRIVATE_DATA_TREE_HEIGHT>, MAX_READ_REQUESTS_PER_TX> const&
read_request_membership_witnesses);

void common_update_end_values(DummyBuilder& builder,
PrivateCallData<NT> const& private_call,
Expand All @@ -42,6 +48,7 @@ void common_contract_logic(DummyBuilder& builder,
ContractDeploymentData<NT> const& contract_dep_data,
FunctionData<NT> const& function_data);

void common_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
KernelCircuitPublicInputs<NT>& public_inputs);
void common_inner_ordering_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
KernelCircuitPublicInputs<NT>& public_inputs);

} // namespace aztec3::circuits::kernel::private_kernel
23 changes: 13 additions & 10 deletions circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,16 @@ 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());

private_inputs_init.private_call.call_stack_item.public_inputs.new_commitments[0] = fr(12);
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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
6 changes: 3 additions & 3 deletions circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ KernelCircuitPublicInputs<NT> 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);
Expand Down
Loading

0 comments on commit fdf41ed

Please sign in to comment.