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.

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

feat: internal keyword + lending contract and tests #978

Merged
merged 21 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,18 @@ jobs:
name: "Test"
command: cond_spot_run_tests end-to-end e2e_deploy_contract.test.ts

e2e-lending-contract:
docker:
- image: aztecprotocol/alpine-build-image
resource_class: small
steps:
- *checkout
- *setup_env
- run:
name: "Test"
command: cond_spot_run_tests end-to-end e2e_lending_contract.test.ts


e2e-zk-token-contract:
docker:
- image: aztecprotocol/alpine-build-image
Expand Down Expand Up @@ -1115,6 +1127,7 @@ workflows:

- e2e-2-rpc-servers: *e2e_test
- e2e-deploy-contract: *e2e_test
- e2e-lending-contract: *e2e_test
- e2e-zk-token-contract: *e2e_test
- e2e-block-building: *e2e_test
- e2e-nested-contract: *e2e_test
Expand All @@ -1136,6 +1149,7 @@ workflows:
requires:
- e2e-2-rpc-servers
- e2e-deploy-contract
- e2e-lending-contract
- e2e-zk-token-contract
- e2e-block-building
- e2e-nested-contract
Expand Down
2 changes: 1 addition & 1 deletion circuits/cpp/barretenberg/sol/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ ffi = true
[fuzz]
runs = 1

solc = "0.8.18"
solc = "0.8.21"

# See more config options https://github.com/foundry-rs/foundry/tree/master/config
12 changes: 9 additions & 3 deletions circuits/cpp/src/aztec3/circuits/abis/function_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ template <typename NCT> struct FunctionData {
using fr = typename NCT::fr;

uint32 function_selector; // e.g. 1st 4-bytes of abi-encoding of function.
boolean is_internal = false;
boolean is_private = false;
boolean is_constructor = false;

MSGPACK_FIELDS(function_selector, is_private, is_constructor);
MSGPACK_FIELDS(function_selector, is_internal, is_private, is_constructor);

boolean operator==(FunctionData<NCT> const& other) const
{
return function_selector == other.function_selector && is_private == other.is_private &&
is_constructor == other.is_constructor;
return function_selector == other.function_selector && is_internal == other.is_internal &&
is_private == other.is_private && is_constructor == other.is_constructor;
};

template <typename Builder> FunctionData<CircuitTypes<Builder>> to_circuit_type(Builder& builder) const
Expand All @@ -38,6 +39,7 @@ template <typename NCT> struct FunctionData {

FunctionData<CircuitTypes<Builder>> function_data = {
to_ct(function_selector),
to_ct(is_internal),
to_ct(is_private),
to_ct(is_constructor),
};
Expand All @@ -52,6 +54,7 @@ template <typename NCT> struct FunctionData {

FunctionData<NativeTypes> function_data = {
to_nt(function_selector),
to_nt(is_internal),
to_nt(is_private),
to_nt(is_constructor),
};
Expand All @@ -64,6 +67,7 @@ template <typename NCT> struct FunctionData {
static_assert(!(std::is_same<NativeTypes, NCT>::value));

fr(function_selector).set_public();
fr(is_internal).set_public();
fr(is_private).set_public();
fr(is_constructor).set_public();
}
Expand All @@ -73,6 +77,7 @@ template <typename NCT> struct FunctionData {
{
std::vector<fr> const inputs = {
fr(function_selector),
fr(is_internal),
fr(is_private),
fr(is_constructor),
};
Expand All @@ -84,6 +89,7 @@ template <typename NCT> struct FunctionData {
template <typename NCT> std::ostream& operator<<(std::ostream& os, FunctionData<NCT> const& function_data)
{
return os << "function_selector: " << function_data.function_selector << "\n"
<< "is_internal: " << function_data.is_internal << "\n"
<< "is_private: " << function_data.is_private << "\n"
<< "is_constructor: " << function_data.is_constructor << "\n";
}
Expand Down
24 changes: 10 additions & 14 deletions circuits/cpp/src/aztec3/circuits/abis/function_leaf_preimage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,15 @@ template <typename NCT> struct FunctionLeafPreimage {
using uint32 = typename NCT::uint32;

uint32 function_selector = 0;
boolean is_internal = false;
Copy link
Member

Choose a reason for hiding this comment

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

This being almost the same as function_data makes me think that the types should maybe be unified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, 3 are overlapping. But the function data is accesible inside noir as well, so maybe the leaf should include the function data: leaf = {function_data, vk_hash, acir_hash}. Seems more intuitive to have the is_constructor stored in the leaf as well. Don't recall how we right now are ensuring the constructors are not rerun @iAmMichaelConnor.

Copy link
Member

Choose a reason for hiding this comment

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

I like the sounds of that

Copy link
Contributor

Choose a reason for hiding this comment

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

I think perhaps the difference between function_data and function_leaf_preimage (at least, when things were initially designed... having quickly looked back at the diagram, which is basically an extension of my memory at this point)... is...

function_data is information about the function you wish to call, which becomes part of a tx_request. It's separate from any deployment information. Perhaps it should be renamed to target_function or entrypoint_function or something. Given this information, a node which has already synced can lookup the rest of the function's info (vk_hash, acir_hash, etc).

function_leaf_preimage is data required to prove existence of the function in the contracts tree. Some of that data comes from function_data, and some of that data is grabbed by the rpc server.

Some of this is faint memories, so I could be wrong, but I believe their separation was intentional.

Re storing is_constructor in the leaf. In the initial design, the constructor isn't included as a leaf in the tree (because it never needs to be called after deployment); so the constructor's information is only included in the constructor_hash which is included in the contract's address. I'm not sure if that's still the case in the code. I intend to write lots of specs soon, and we'll review all this.

Don't recall how we right now are ensuring the constructors are not rerun
We emit a nullifier = h(contract_address). You can only call a constructor at the time of deployment, and at deployment the nullifier gets emitted, so there's no way to call a constructor twice.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, it can stay as is!

boolean is_private = false;
fr vk_hash = 0;
fr acir_hash = 0;

boolean operator==(FunctionLeafPreimage<NCT> const& other) const
{
return function_selector == other.function_selector && is_private == other.is_private &&
vk_hash == other.vk_hash && acir_hash == other.acir_hash;
return function_selector == other.function_selector && is_internal == other.is_internal &&
is_private == other.is_private && vk_hash == other.vk_hash && acir_hash == other.acir_hash;
};

template <typename Builder> FunctionLeafPreimage<CircuitTypes<Builder>> to_circuit_type(Builder& builder) const
Expand All @@ -50,10 +51,7 @@ template <typename NCT> struct FunctionLeafPreimage {
auto to_ct = [&](auto& e) { return aztec3::utils::types::to_ct(builder, e); };

FunctionLeafPreimage<CircuitTypes<Builder>> preimage = {
to_ct(function_selector),
to_ct(is_private),
to_ct(vk_hash),
to_ct(acir_hash),
to_ct(function_selector), to_ct(is_internal), to_ct(is_private), to_ct(vk_hash), to_ct(acir_hash),
};

return preimage;
Expand All @@ -65,10 +63,7 @@ template <typename NCT> struct FunctionLeafPreimage {
auto to_nt = [&](auto& e) { return aztec3::utils::types::to_nt<Builder>(e); };

FunctionLeafPreimage<NativeTypes> preimage = {
to_nt(function_selector),
to_nt(is_private),
to_nt(vk_hash),
to_nt(acir_hash),
to_nt(function_selector), to_nt(is_internal), to_nt(is_private), to_nt(vk_hash), to_nt(acir_hash),
};

return preimage;
Expand All @@ -79,6 +74,7 @@ template <typename NCT> struct FunctionLeafPreimage {
static_assert(!(std::is_same<NativeTypes, NCT>::value));

function_selector.set_public();
fr(is_internal).set_public();
fr(is_private).set_public();
vk_hash.set_public();
acir_hash.set_public();
Expand All @@ -87,10 +83,7 @@ template <typename NCT> struct FunctionLeafPreimage {
fr hash() const
{
std::vector<fr> const inputs = {
function_selector,
fr(is_private),
vk_hash,
acir_hash,
function_selector, fr(is_internal), fr(is_private), vk_hash, acir_hash,
};
return NCT::compress(inputs, GeneratorIndex::FUNCTION_LEAF);
}
Expand All @@ -101,6 +94,7 @@ template <typename NCT> void read(uint8_t const*& it, FunctionLeafPreimage<NCT>&
using serialize::read;

read(it, preimage.function_selector);
read(it, preimage.is_internal);
read(it, preimage.is_private);
read(it, preimage.vk_hash);
read(it, preimage.acir_hash);
Expand All @@ -111,6 +105,7 @@ template <typename NCT> void write(std::vector<uint8_t>& buf, FunctionLeafPreima
using serialize::write;

write(buf, preimage.function_selector);
write(buf, preimage.is_internal);
write(buf, preimage.is_private);
write(buf, preimage.vk_hash);
write(buf, preimage.acir_hash);
Expand All @@ -119,6 +114,7 @@ template <typename NCT> void write(std::vector<uint8_t>& buf, FunctionLeafPreima
template <typename NCT> std::ostream& operator<<(std::ostream& os, FunctionLeafPreimage<NCT> const& preimage)
{
return os << "function_selector: " << preimage.function_selector << "\n"
<< "is_internal: " << preimage.is_internal << "\n"
<< "is_private: " << preimage.is_private << "\n"
<< "vk_hash: " << preimage.vk_hash << "\n"
<< "acir_hash: " << preimage.acir_hash << "\n";
Expand Down
3 changes: 3 additions & 0 deletions circuits/cpp/src/aztec3/circuits/hash.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ void check_membership(Builder& builder,
*
* @tparam NCT (native or circuit)
* @param function_selector in leaf preimage
* @param is_internal in leaf preimage
* @param is_private in leaf preimage
* @param vk_hash in leaf preimage
* @param acir_hash in leaf preimage
Expand All @@ -263,6 +264,7 @@ void check_membership(Builder& builder,
*/
template <typename NCT> typename NCT::fr function_tree_root_from_siblings(
typename NCT::uint32 const& function_selector,
typename NCT::boolean const& is_internal,
typename NCT::boolean const& is_private,
typename NCT::fr const& vk_hash,
typename NCT::fr const& acir_hash,
Expand All @@ -271,6 +273,7 @@ template <typename NCT> typename NCT::fr function_tree_root_from_siblings(
{
const auto function_leaf_preimage = FunctionLeafPreimage<NCT>{
.function_selector = function_selector,
.is_internal = is_internal,
.is_private = is_private,
.vk_hash = vk_hash,
.acir_hash = acir_hash,
Expand Down
12 changes: 10 additions & 2 deletions circuits/cpp/src/aztec3/circuits/kernel/private/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,16 +365,24 @@ void common_contract_logic(DummyBuilder& builder,

/* We need to compute the root of the contract tree, starting from the function's VK:
* - Compute the vk_hash (done above)
* - Compute the function_leaf: hash(function_selector, is_private, vk_hash, acir_hash)
* - Compute the function_leaf: hash(function_selector, is_internal, is_private, vk_hash, acir_hash)
* - Hash the function_leaf with the function_leaf's sibling_path to get the function_tree_root
* - Compute the contract_leaf: hash(contract_address, portal_contract_address, function_tree_root)
* - Hash the contract_leaf with the contract_leaf's sibling_path to get the contract_tree_root
*/

// The logic below ensures that the contract exists in the contracts tree
// Ensures that if the function is internal, only the contract itself can call it
if (private_call.call_stack_item.function_data.is_internal) {
builder.do_assert(
storage_contract_address == private_call.call_stack_item.public_inputs.call_context.msg_sender,
"call is internal, but msg_sender is not self",
CircuitErrorCode::PRIVATE_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL);
}

// The logic below ensures that the contract exists in the contracts tree
auto const& computed_function_tree_root =
function_tree_root_from_siblings<NT>(private_call.call_stack_item.function_data.function_selector,
private_call.call_stack_item.function_data.is_internal,
true, // is_private
private_call_vk_hash,
private_call.acir_hash,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ TEST_F(native_private_kernel_inner_tests, private_function_zero_storage_contract
"contract address can't be 0 for non-contract deployment related transactions");
}


TEST_F(native_private_kernel_inner_tests, private_function_incorrect_is_internal)
{
auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args());

// Make the call internal but msg_sender != storage_contract_address.
private_inputs.private_call.call_stack_item.function_data.is_internal = true;
private_inputs.private_call.call_stack_item.public_inputs.call_context.msg_sender = 1;
private_inputs.private_call.call_stack_item.public_inputs.call_context.storage_contract_address = 2;

// Invoke the native private kernel circuit
DummyBuilder builder = DummyBuilder("private_kernel_tests__private_function_incorrect_contract_tree_root_fails");
native_private_kernel_circuit_inner(builder, private_inputs);

// Assertion checks
EXPECT_TRUE(builder.failed());
EXPECT_EQ(builder.get_first_failure().code, CircuitErrorCode::PRIVATE_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL);
EXPECT_EQ(builder.get_first_failure().message, "call is internal, but msg_sender is not self");
}

TEST_F(native_private_kernel_inner_tests, private_function_incorrect_contract_tree_root_fails)
{
auto private_inputs = do_private_call_get_kernel_inputs_inner(false, deposit, standard_test_args());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ std::pair<PrivateCallData<NT>, ContractDeploymentData<NT>> create_private_call_d
call_context.storage_contract_address = contract_address;
} else {
const NT::fr& function_tree_root = function_tree_root_from_siblings<NT>(function_data.function_selector,
function_data.is_internal,
function_data.is_private,
private_circuit_vk_hash,
acir_hash,
Expand Down
16 changes: 16 additions & 0 deletions circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,22 @@ TEST(public_kernel_tests, no_bytecode_hash_should_fail)
ASSERT_EQ(dummyBuilder.get_first_failure().code, CircuitErrorCode::PUBLIC_KERNEL__BYTECODE_HASH_INVALID);
}


TEST(public_kernel_tests, invalid_is_internal)
{
DummyBuilder dummyBuilder = DummyBuilder("public_kernel_tests__no_bytecode_hash_should_fail");
PublicKernelInputs<NT> inputs = get_kernel_inputs_with_previous_kernel(true);

// Make the call internal but msg_sender != storage_contract_address.
inputs.public_call.call_stack_item.function_data.is_internal = true;
inputs.public_call.call_stack_item.public_inputs.call_context.msg_sender = 1;
inputs.public_call.call_stack_item.public_inputs.call_context.storage_contract_address = 2;

auto public_inputs = native_public_kernel_circuit_private_previous_kernel(dummyBuilder, inputs);
ASSERT_TRUE(dummyBuilder.failed());
ASSERT_EQ(dummyBuilder.get_first_failure().code, CircuitErrorCode::PUBLIC_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL);
}

TEST(public_kernel_tests, contract_address_must_be_valid)
{
DummyBuilder dummyBuilder = DummyBuilder("public_kernel_tests__contract_address_must_be_valid");
Expand Down
9 changes: 9 additions & 0 deletions circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,15 @@ void common_validate_inputs(DummyBuilder& builder, KernelInput const& public_ker
builder.do_assert(public_kernel_inputs.public_call.bytecode_hash != 0,
"Bytecode hash must be non-zero",
CircuitErrorCode::PUBLIC_KERNEL__BYTECODE_HASH_INVALID);

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a cpp test testing this code path, similarly for the other assert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will do. It's tested in the TS as well.

if (this_call_stack_item.function_data.is_internal) {
auto const target = this_call_stack_item.contract_address;
auto const sender = this_call_stack_item.public_inputs.call_context.msg_sender;

builder.do_assert(target == sender,
"call is internal, but msg_sender is not self",
CircuitErrorCode::PUBLIC_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL);
}
}

template <typename KernelInput, typename Builder>
Expand Down
6 changes: 3 additions & 3 deletions circuits/cpp/src/aztec3/constants.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ enum GeneratorIndex {
OUTER_NULLIFIER, // Size = 2
PUBLIC_DATA_READ, // Size = 2
PUBLIC_DATA_UPDATE_REQUEST, // Size = 3
FUNCTION_DATA, // Size = 3
FUNCTION_LEAF, // Size = 4
FUNCTION_DATA, // Size = 4
FUNCTION_LEAF, // Size = 5
CONTRACT_DEPLOYMENT_DATA, // Size = 4
CONSTRUCTOR, // Size = 3
CONSTRUCTOR_ARGS, // Size = 8
Expand Down Expand Up @@ -207,7 +207,7 @@ constexpr size_t VIEW_NOTE_ORACLE_RETURN_LENGTH = MAX_NOTES_PER_PAGE * (MAX_NOTE

constexpr size_t CALL_CONTEXT_LENGTH = 6;
constexpr size_t COMMITMENT_TREES_ROOTS_LENGTH = 5;
constexpr size_t FUNCTION_DATA_LENGTH = 3;
constexpr size_t FUNCTION_DATA_LENGTH = 4;
constexpr size_t CONTRACT_DEPLOYMENT_DATA_LENGTH = 6;

// Change this ONLY if you have changed the PrivateCircuitPublicInputs structure in C++.
Expand Down
2 changes: 2 additions & 0 deletions circuits/cpp/src/aztec3/utils/circuit_errors.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ enum CircuitErrorCode : uint16_t {
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,
PRIVATE_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL = 2022,

// Public kernel related errors
PUBLIC_KERNEL_CIRCUIT_FAILED = 3000,
Expand Down Expand Up @@ -61,6 +62,7 @@ enum CircuitErrorCode : uint16_t {
PUBLIC_KERNEL__NEW_NULLIFIERS_NOT_EMPTY_IN_FIRST_ITERATION = 3025,
PUBLIC_KERNEL__NEW_COMMITMENTS_PROHIBITED_IN_STATIC_CALL = 3026,
PUBLIC_KERNEL__NEW_NULLIFIERS_PROHIBITED_IN_STATIC_CALL = 3027,
PUBLIC_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL = 3028,

BASE__CIRCUIT_FAILED = 4000,
BASE__KERNEL_PROOF_VERIFICATION_FAILED = 4001,
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
src = 'src'
out = 'out'
libs = ['lib']
solc = "0.8.18"
solc = "0.8.21"

remappings = [
"@oz/=lib/openzeppelin-contracts/contracts/",
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/lib/openzeppelin-contracts
Loading