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

Can't call this contract from the constructor #2249

Closed
iAmMichaelConnor opened this issue Sep 12, 2023 · 5 comments
Closed

Can't call this contract from the constructor #2249

iAmMichaelConnor opened this issue Sep 12, 2023 · 5 comments
Labels
T-bug Type: Bug. Something is broken. T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature).

Comments

@iAmMichaelConnor
Copy link
Contributor

Issue found by @LHerskind.
When a contract is deployed, a constructor is executed. That constructor might wish to make calls to other functions of the contract: in particular to internal public functions. Or, the constructor might make calls to other functions, which then re-enter some other function of this.

The problem is, neither kernel circuit (neither public nor private) "knows about" this contract yet, because the contract's data doesn't get inserted into the contract tree until the base rollup circuit.

One suggested approach to resolving this would be:

In both kernel circuits, detect whether the tx is_contract_deployment, and extract the contract_deployment_data. The contract_deployment_data contains the function_tree_root (iirc). If some 'call' to this is made in a later kernel iteration (i.e. after the iteration which processes the constructor), the kernel circuit can prove existence of the call's (contract_address, function_selector, etc) in the function_tree_root. It can't prove existence of the function_tree_root in the contract_root (because it hasn't been inserted), but that's ok as long as the function_tree_root matches the one in the contract_deployment_data. With this approach, the kernel circuit processes any calls to this 'optimistically', with the assumption that they'll be inserted into the contract tree by the base rollup circuit later.

@iAmMichaelConnor iAmMichaelConnor added T-bug Type: Bug. Something is broken. T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature). labels Sep 12, 2023
@github-project-automation github-project-automation bot moved this to Todo in A3 Sep 12, 2023
@iAmMichaelConnor
Copy link
Contributor Author

Tagging @dbanks12 @suyash67 @jeanmon for thoughts.

@jeanmon
Copy link
Contributor

jeanmon commented Sep 12, 2023

Sounds like after transient/pending commitments, read requests, nullifiers, we need to support transient function calls :-)

If I am understanding correctly, we should not have any ordering issue here as there should be only one constructor call per transaction and this one is called first and therefore will be processed first by the private kernel circuit. Correct @iAmMichaelConnor ?

@iAmMichaelConnor
Copy link
Contributor Author

That might be correct. Even if a constructor wasn't processed first (which might become the case in an 'account abstraction' world, where account contract entrypoints might need to be executed first), am I correct in thinking the constructor will always be processed by the kernel before any functions which are called by the constructor?

Related to this topic, there's been talk of removing the contract tree altogether (https://discourse.aztec.network/t/contract-classes-upgrades-and-default-accounts/433/14#get-rid-of-the-contract-tree-6). That might actually make this issue simpler to solve, but it's a complicated set of considerations.

@jeanmon
Copy link
Contributor

jeanmon commented Sep 12, 2023

@iAmMichaelConnor Yes, a parent or ancestor function is processed before a child/descendant in the private kernel. If you represent the function calls with a tree (an edge being a call), we process depth-first search but 'horizontally in a stack-based/reverse order'.

alexghr added a commit that referenced this issue Oct 3, 2023
This PR aims to enable calling of public functions in Noir contract
constructors. In order to fix this issue this PR does the following two
changes:

- previously the `PublicExecutor` only looked at deployed contracts. Now
it also looks at the contracts being deployed in the current block
- the public kernel simulator 'lost' the contract's address so when
publishing the block it would have contract data as all zeros. This PR
updates the common initialisation function to pass on `new_contracts`.

Fix #2509
Related to #2249 

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
Maddiaa0 pushed a commit that referenced this issue Oct 6, 2023
This PR aims to enable calling of public functions in Noir contract
constructors. In order to fix this issue this PR does the following two
changes:

- previously the `PublicExecutor` only looked at deployed contracts. Now
it also looks at the contracts being deployed in the current block
- the public kernel simulator 'lost' the contract's address so when
publishing the block it would have contract data as all zeros. This PR
updates the common initialisation function to pass on `new_contracts`.

Fix #2509
Related to #2249 

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
@rahul-kothari
Copy link
Contributor

I believe this was fixed in #2549 @alexghr ?

@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: Bug. Something is broken. T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature).
Projects
Archived in project
Development

No branches or pull requests

3 participants