Skip to content

feat(sequencer): get correct witnesses for base rollup #155

Merged
Maddiaa0 merged 27 commits into
masterfrom
md/nullifier-tree-construction
Apr 6, 2023
Merged

feat(sequencer): get correct witnesses for base rollup #155
Maddiaa0 merged 27 commits into
masterfrom
md/nullifier-tree-construction

Conversation

@Maddiaa0

@Maddiaa0 Maddiaa0 commented Apr 3, 2023

Copy link
Copy Markdown
Member

Description

Depends on AztecProtocol/aztec3-circuits#131

Updates the sequencer to get the witnesses for insertion in an alternative way compatible with the base rollup.

See a description of the insertion algorithm over at https://colab.research.google.com/drive/1A0gizduSi4FIiIJZ8OylwIpO9-OTqV-R

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

TODO:

  • Tests
  • Alter starting indexed merkle tree to be prepopulated with values for one rollup

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

Base automatically changed from pw/kernel-integration to master April 4, 2023 09:38
@Maddiaa0 Maddiaa0 force-pushed the md/nullifier-tree-construction branch from f7f645b to e28a09c Compare April 4, 2023 22:30
@Maddiaa0 Maddiaa0 force-pushed the md/nullifier-tree-construction branch from e28a09c to 9c704bf Compare April 5, 2023 09:54
Comment thread yarn-project/merkle-tree/src/indexed_tree/indexed_tree.ts
Comment thread yarn-project/merkle-tree/src/indexed_tree/indexed_tree.ts
Comment thread yarn-project/merkle-tree/src/indexed_tree/indexed_tree.ts Outdated
Comment thread yarn-project/merkle-tree/src/merkle_tree.ts
Comment thread yarn-project/sequencer-client/src/block_builder/circuit_powered_block_builder.ts Outdated
Comment thread yarn-project/sequencer-client/src/sequencer/index.ts
Comment thread yarn-project/merkle-tree/src/merkle_tree.ts
@Maddiaa0 Maddiaa0 changed the title wip: feat(sequencer): get correct witnesses for base rollup feat(sequencer): get correct witnesses for base rollup Apr 5, 2023
Comment thread yarn-project/circuits.js/package.json Outdated
},
"dependencies": {
"@aztec/foundation": "workspace:^",
"@aztec/merkle-tree": "workspace:^",

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.

What's this new dependency for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good spot, it can be removed. In shared.ts 'fromBufferArray' was previously 'fromSiblingPath' which used SiblingPath from merkle tree.
I refactored last second to prevent dependency spread

* WARNING: This function has side effects, it will insert values into the tree.
*
* Assumptions:
* 1. There are 8 nullifiers provided and they are all unique

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is excluding empty nullifiers, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes! I will update the comment

Comment thread yarn-project/merkle-tree/src/merkle_tree.ts
this.validateTree(rollupOutput, MerkleTreeId.DATA_TREE, 'PrivateData'),
// TODO: Wait for new implementation of nullifier tree to avoid mismatches here
// this.validateTree(rollupOutput, MerkleTreeId.NULLIFIER_TREE, 'Nullifier'),
this.validateTree(rollupOutput, MerkleTreeId.NULLIFIER_TREE, 'Nullifier'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome

Comment thread yarn-project/circuits.js/package.json Outdated
},
"dependencies": {
"@aztec/foundation": "workspace:^",
"@aztec/merkle-tree": "workspace:^",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sure to run yarn prepare in the root of yarn-projects if you change dependencies, so these get reflected in the build manifest

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

dep removed


// For each calculated new leaf, we insert it into the tree at the next position
for (let i = 0; i < insertionSubtree.length; i++) {
// We can skip inserting empty leaves

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stale comment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, good catch

nextIndex: BigInt(insertionSubtree[i].nextIndex),
};

await this.db.updateLeaf(MerkleTreeId.NULLIFIER_TREE, asLeafData, startInsertionIndex + BigInt(i));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use appendLeaf here instead? Not too important, I see updateLeaf updates the size anyway.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

unfortunately we cant, appending a leaf will fail as we have already forcefully updated pointers underneath in the previous step

@Maddiaa0 Maddiaa0 merged commit eb69865 into master Apr 6, 2023
@Maddiaa0 Maddiaa0 deleted the md/nullifier-tree-construction branch April 6, 2023 08:59
* The tree must be initially padded as the pre-populated 0 index prevents efficient subtree insertion.
* Padding with some values solves this issue.
*/
export const INITIAL_NULLIFIER_TREE_SIZE = 8;

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.

Just to be sure. Is this taking us down a road of fixed size rollups?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think it will have that effect long term, it will just need to be the size of a nullifier tree sub tree insertion

ludamad added a commit that referenced this pull request Apr 14, 2023
* build: dont always clean

* Update bootstrap.sh

* Update bootstrap.sh
ludamad added a commit that referenced this pull request Apr 17, 2023
* build: dont always clean

* Update bootstrap.sh

* Update bootstrap.sh
ludamad added a commit that referenced this pull request Apr 17, 2023
* build: dont always clean

* Update bootstrap.sh

* Update bootstrap.sh
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.

3 participants