Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(kernel): dont chop commitments when matched to transient reads #1079

Merged
merged 7 commits into from
Jul 18, 2023
Merged

fix(kernel): dont chop commitments when matched to transient reads #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
156e92b
dont chop reads
dbanks12 Jul 14, 2023
ac99ec0
remove accidentally added scrap files
dbanks12 Jul 14, 2023
2bba017
various fixes and cleanup - don't forward non-transient reads to next…
dbanks12 Jul 17, 2023
4e86b35
remove log
dbanks12 Jul 17, 2023
a917f7c
tests that only transient reads get forwarded to next kernel
dbanks12 Jul 17, 2023
351972e
revert minor diff
dbanks12 Jul 17, 2023
3b3eccb
1073 - jeanmon review's comment
jeanmon Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 76 additions & 37 deletions circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, common prefix is meant for re-usability and does not imply that all circuits use the pre-fixed method. Adding the "inner_ordering" is a bit too verbose in my opinion but I can live with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I wanted to make it more clear which circuits use them, but i think you're right that it's overkill.

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;
jeanmon marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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