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

Remove historic private data root from app circuit public inputs - add to PrivateCallData #778

Closed
dbanks12 opened this issue Jun 8, 2023 · 7 comments · Fixed by #2671 or #2669
Closed
Assignees
Labels
C-protocol-circuits Component: Protocol circuits (kernel & rollup)

Comments

@dbanks12
Copy link
Collaborator

dbanks12 commented Jun 8, 2023

No description provided.

@dbanks12
Copy link
Collaborator Author

Do we actually want to do this? How will inputs change when we have full historic trees to check? Maybe we leave this in but separately add this to PrivateCallData.

@jeanmon
Copy link
Contributor

jeanmon commented Oct 3, 2023

In between, some refactoring tasks changed the setting. Currently, the various data roots (nullifierTreeRoot, privateDataTreeRoot, etc, ..) lie in a data structure named HistoricBlockData.
HistoricBlockData is used in private AND public world, for instance, is a member of:

  • PublicCircuitPublicInputs
  • PrivateCircuitPublicInputs
  • PublicProcessor

Removing privateDataTreeRoot from HistoricBlockData does not seem a viable option as public world related data certainly needs this information.

Creating a separate data structure e.g., PrivateHistoricBlockData which would be HistoricBlockData removed from privateDataTreeRoot seems a bit of an overkill unless there is strong reason.

@dbanks12 What do you think?
@Maddiaa0 I saw you implemented related factoring tasks. Is my analysis correct and do you have any opinion on this?

@Maddiaa0
Copy link
Member

Maddiaa0 commented Oct 3, 2023

All of the information is available in public and private mainly so the block hash can be reconstructed and checked in the base rollup circuit ( so that all of the tree roots are from the same block )

I dont have a particularly strong opinion around changes, however im not entirely sure why it should move into calldata, do we not want to allow all users to be able to make claims about the historic state?

@dbanks12
Copy link
Collaborator Author

dbanks12 commented Oct 3, 2023

I think I agree that it doesn't make sense to move this. I created this ticket before we really had any other historic info.

I suspect that we are never setting this historic root from Noir. I think we need to be setting it whenever we are outputting read requests. Does this sound right @Maddiaa0?

Here are the next steps discussed in scrum:

  1. Try removing kernelProver code that overrides historic root (probably will lead to failure)
  2. Look for other locations in Noir where historic roots are set and possibly mimic here
  3. Maybe set historic privateDataRoot when read-requests are created in Noir
  4. Confirm whether the historic roots actually checked somewhere in C++ circuits (probably base rollup). If not, create ticket to do so.

@jeanmon
Copy link
Contributor

jeanmon commented Oct 4, 2023

@jeanmon
Copy link
Contributor

jeanmon commented Oct 4, 2023

My understanding is that TS script code is setting these tree roots as part of the acir simulator and pass then them to noir code.

Relevant code:

const historicBlockData = await this.db.getHistoricBlockData();

const context = new ClientExecutionContext(

Passing to noir code:

private_data_tree_root : fields[113],

@Maddiaa0 Is my analysis correct? (l am not sure about the noir code part.)

@dbanks12 I think that the above TS line sets all the historic roots (which would answer 2.)

@jeanmon
Copy link
Contributor

jeanmon commented Oct 4, 2023

Answer to 4., all members are used in kernel or rollup code, except the 2 following members:

  • blocks_tree_root
  • private_kernel_vk_tree_root

The latter is certainly not used because we still work with native code. What about the former? @Maddiaa0 @dbanks12

@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 4, 2023
spalladino pushed a commit that referenced this issue Oct 4, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.2</summary>

##
[0.8.2](aztec-packages-v0.8.1...aztec-packages-v0.8.2)
(2023-10-04)


### Features

* Constrain return notes from oracle call.
([#2639](#2639))
([248be1b](248be1b))
* Multiple pixies 1 Sandbox
([#2492](#2492))
([572d572](572d572))
* Take an optional owner to create the initialization nullifier
([#2647](#2647))
([fefc443](fefc443))


### Bug Fixes

* Add missing properties to deployed packages
([#2678](#2678))
([343df30](343df30))
* Include ignition data in package or save after 1st download
([#2591](#2591))
([d5e9f8b](d5e9f8b)),
closes
[#2445](#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](#2660))
([3cb9639](3cb9639))
* Removal of setting private data root in kernel prover
([#2671](#2671))
([6a2cc28](6a2cc28)),
closes
[#778](#778)


### Miscellaneous

* 1 deploy_dockerhub CI task
([#2670](#2670))
([dff396c](dff396c))
* Check that portal address is saved
([#2641](#2641))
([9ebef6e](9ebef6e))
* Fixes in deploy scripts
([#2659](#2659))
([f44568b](f44568b))
* Measure circuit simulation times and input/output sizes
([#2663](#2663))
([027f7ec](027f7ec))
* Remove sandbox base image and force_deploy_build. Generalise in
check_rebuild.
([#2645](#2645))
([805fe18](805fe18))
</details>

<details><summary>barretenberg.js: 0.8.2</summary>

##
[0.8.2](barretenberg.js-v0.8.1...barretenberg.js-v0.8.2)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.2</summary>

##
[0.8.2](barretenberg-v0.8.1...barretenberg-v0.8.2)
(2023-10-04)


### Bug Fixes

* Include ignition data in package or save after 1st download
([#2591](#2591))
([d5e9f8b](d5e9f8b)),
closes
[#2445](#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](#2660))
([3cb9639](3cb9639))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this issue Oct 5, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.2</summary>

##
[0.8.2](AztecProtocol/aztec-packages@aztec-packages-v0.8.1...aztec-packages-v0.8.2)
(2023-10-04)


### Features

* Constrain return notes from oracle call.
([#2639](AztecProtocol/aztec-packages#2639))
([248be1b](AztecProtocol/aztec-packages@248be1b))
* Multiple pixies 1 Sandbox
([#2492](AztecProtocol/aztec-packages#2492))
([572d572](AztecProtocol/aztec-packages@572d572))
* Take an optional owner to create the initialization nullifier
([#2647](AztecProtocol/aztec-packages#2647))
([fefc443](AztecProtocol/aztec-packages@fefc443))


### Bug Fixes

* Add missing properties to deployed packages
([#2678](AztecProtocol/aztec-packages#2678))
([343df30](AztecProtocol/aztec-packages@343df30))
* Include ignition data in package or save after 1st download
([#2591](AztecProtocol/aztec-packages#2591))
([d5e9f8b](AztecProtocol/aztec-packages@d5e9f8b)),
closes
[#2445](AztecProtocol/aztec-packages#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](AztecProtocol/aztec-packages#2660))
([3cb9639](AztecProtocol/aztec-packages@3cb9639))
* Removal of setting private data root in kernel prover
([#2671](AztecProtocol/aztec-packages#2671))
([6a2cc28](AztecProtocol/aztec-packages@6a2cc28)),
closes
[#778](AztecProtocol/aztec-packages#778)


### Miscellaneous

* 1 deploy_dockerhub CI task
([#2670](AztecProtocol/aztec-packages#2670))
([dff396c](AztecProtocol/aztec-packages@dff396c))
* Check that portal address is saved
([#2641](AztecProtocol/aztec-packages#2641))
([9ebef6e](AztecProtocol/aztec-packages@9ebef6e))
* Fixes in deploy scripts
([#2659](AztecProtocol/aztec-packages#2659))
([f44568b](AztecProtocol/aztec-packages@f44568b))
* Measure circuit simulation times and input/output sizes
([#2663](AztecProtocol/aztec-packages#2663))
([027f7ec](AztecProtocol/aztec-packages@027f7ec))
* Remove sandbox base image and force_deploy_build. Generalise in
check_rebuild.
([#2645](AztecProtocol/aztec-packages#2645))
([805fe18](AztecProtocol/aztec-packages@805fe18))
</details>

<details><summary>barretenberg.js: 0.8.2</summary>

##
[0.8.2](AztecProtocol/aztec-packages@barretenberg.js-v0.8.1...barretenberg.js-v0.8.2)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.2</summary>

##
[0.8.2](AztecProtocol/aztec-packages@barretenberg-v0.8.1...barretenberg-v0.8.2)
(2023-10-04)


### Bug Fixes

* Include ignition data in package or save after 1st download
([#2591](AztecProtocol/aztec-packages#2591))
([d5e9f8b](AztecProtocol/aztec-packages@d5e9f8b)),
closes
[#2445](AztecProtocol/aztec-packages#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](AztecProtocol/aztec-packages#2660))
([3cb9639](AztecProtocol/aztec-packages@3cb9639))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Maddiaa0 pushed a commit that referenced this issue Oct 6, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.2</summary>

##
[0.8.2](aztec-packages-v0.8.1...aztec-packages-v0.8.2)
(2023-10-04)


### Features

* Constrain return notes from oracle call.
([#2639](#2639))
([248be1b](248be1b))
* Multiple pixies 1 Sandbox
([#2492](#2492))
([572d572](572d572))
* Take an optional owner to create the initialization nullifier
([#2647](#2647))
([fefc443](fefc443))


### Bug Fixes

* Add missing properties to deployed packages
([#2678](#2678))
([343df30](343df30))
* Include ignition data in package or save after 1st download
([#2591](#2591))
([d5e9f8b](d5e9f8b)),
closes
[#2445](#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](#2660))
([3cb9639](3cb9639))
* Removal of setting private data root in kernel prover
([#2671](#2671))
([6a2cc28](6a2cc28)),
closes
[#778](#778)


### Miscellaneous

* 1 deploy_dockerhub CI task
([#2670](#2670))
([dff396c](dff396c))
* Check that portal address is saved
([#2641](#2641))
([9ebef6e](9ebef6e))
* Fixes in deploy scripts
([#2659](#2659))
([f44568b](f44568b))
* Measure circuit simulation times and input/output sizes
([#2663](#2663))
([027f7ec](027f7ec))
* Remove sandbox base image and force_deploy_build. Generalise in
check_rebuild.
([#2645](#2645))
([805fe18](805fe18))
</details>

<details><summary>barretenberg.js: 0.8.2</summary>

##
[0.8.2](barretenberg.js-v0.8.1...barretenberg.js-v0.8.2)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.2</summary>

##
[0.8.2](barretenberg-v0.8.1...barretenberg-v0.8.2)
(2023-10-04)


### Bug Fixes

* Include ignition data in package or save after 1st download
([#2591](#2591))
([d5e9f8b](d5e9f8b)),
closes
[#2445](#2445)
* Make target architecture configurable, target westmere in GA.
([#2660](#2660))
([3cb9639](3cb9639))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-protocol-circuits Component: Protocol circuits (kernel & rollup)
Projects
Archived in project
3 participants