Skip to content

Set historic tree roots in private circuit inputs#425

Merged
PhilWindle merged 7 commits into
masterfrom
palla/set-tree-roots-in-private-circuit
May 4, 2023
Merged

Set historic tree roots in private circuit inputs#425
PhilWindle merged 7 commits into
masterfrom
palla/set-tree-roots-in-private-circuit

Conversation

@spalladino

Copy link
Copy Markdown
Contributor

Populate historic tree roots in the inputs to the private circuit, so these are propagated to the private kernel circuit, and are sent with the tx. These are fetched from the aztec node, via a new method that returns all tree roots.

Fixes #424

@spalladino spalladino requested a review from PhilWindle May 2, 2023 17:40
public async getTreeRoots(): Promise<Record<MerkleTreeId, Fr>> {
const getTreeRoot = async (id: MerkleTreeId) =>
Fr.fromBuffer((await this.merkleTreeDB.getTreeInfo(id, false)).root);
console.log(`NULL TREE`, await getTreeRoot(MerkleTreeId.NULLIFIER_TREE));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this console log be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Most definitely. Good catch.

@spalladino spalladino force-pushed the palla/set-tree-roots-in-private-circuit branch from 8e344f4 to 2c2a3db Compare May 3, 2023 16:00
@spalladino

Copy link
Copy Markdown
Contributor Author

@PhilWindle I addressed the console.log and added a few more changes related to the new l1-to-l2 trees, let me know if you think it's good to merge.

@spalladino spalladino requested a review from PhilWindle May 3, 2023 17:00
* Returns the current committed roots for the data trees.
* @returns the current committed roots for the data trees.
*/
public async getTreeRoots(): Promise<Record<MerkleTreeId, Fr>> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This information is actually already available within the AztecRPCServer as part of the L2Block download. I know we are building the Aztec Sandbox but there will come a point where we want to rely on Aztec Node for data as little as possible, so I wonder if it is worth removing this interaction in favour of taking the locally available version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't know that! It makes a lot more sense. I'll add methods to the aztec-rpc Synchroniser to save in memory the latest roots and use those!

@spalladino spalladino marked this pull request as draft May 4, 2023 11:51
@spalladino spalladino force-pushed the palla/set-tree-roots-in-private-circuit branch from 2c2a3db to 9811991 Compare May 4, 2023 13:08
@spalladino spalladino marked this pull request as ready for review May 4, 2023 14:28
@spalladino

spalladino commented May 4, 2023

Copy link
Copy Markdown
Contributor Author

@PhilWindle I made the change to fetch the tree roots from the blocks as they are processed by the synchroniser. However, I ran into an issue: the aztec-rpc didn't have the concept of an initial sync, so any tests sending a tx once the rpc is setup would run into empty roots. To work around it, I added an initial sync that fetches the latest roots from the aztec node, which is done only once, and then these are updated as new blocks flow in.

However, I'm not sure if there's a deeper problem here: shouldn't the aztec-rpc block sending any txs until it has synced to the top of the chain, so it has processed all nullifiers from the user's accounts?

@PhilWindle

Copy link
Copy Markdown
Collaborator

@PhilWindle I made the change to fetch the tree roots from the blocks as they are processed by the synchroniser. However, I ran into an issue: the aztec-rpc didn't have the concept of an initial sync, so any tests sending a tx once the rpc is setup would run into empty roots. To work around it, I added an initial sync that fetches the latest roots from the aztec node, which is done only once, and then these are updated as new blocks flow in.

However, I'm not sure if there's a deeper problem here: shouldn't the aztec-rpc block sending any txs until it has synced to the top of the chain, so it has processed all nullifiers from the user's accounts?

Yes, I think it should fully sync before the user starts to send txs etc.

@spalladino

Copy link
Copy Markdown
Contributor Author

Yes, I think it should fully sync before the user starts to send txs etc.

Do you think we could merge this one as is, and create an issue to track that bigger change?

@PhilWindle

Copy link
Copy Markdown
Collaborator

Yes, I think it should fully sync before the user starts to send txs etc.

Do you think we could merge this one as is, and create an issue to track that bigger change?

Yeah that's fine

@PhilWindle PhilWindle merged commit 2f59901 into master May 4, 2023
@PhilWindle PhilWindle deleted the palla/set-tree-roots-in-private-circuit branch May 4, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Historic tree roots are not returned by the private kernel circuit

2 participants