feat(blocks_tree): compute block hashes within root rollup circuit #1214

Maddiaa0 · 2023-07-26T18:13:53Z

Description

resolves #1162

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

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 merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

netlify · 2023-07-26T18:13:57Z

✅ Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name	Link
🔨 Latest commit	`2942f95`
🔍 Latest deploy log	https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64c1626357bceb0008750eef

LHerskind

Not sure if we really need to add the block hash in the public inputs.

LHerskind · 2023-08-01T11:29:59Z

circuits/cpp/src/aztec3/circuits/abis/rollup/root/root_rollup_public_inputs.hpp

@@ -22,6 +22,8 @@ template <typename NCT> struct RootRollupPublicInputs {

    GlobalVariables<NCT> globalVariables;

+    fr block_hash;


Do we actually need the block_hash as part of the inputs? Using the global variables and the other values you can compute it. What value does it bring to include it directly here?

LHerskind · 2023-08-01T11:33:14Z

circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp

@@ -378,6 +381,20 @@ RootRollupInputs get_root_rollup_inputs(utils::DummyBuilder& builder,
        .next_available_leaf_index = 1,
    };

+    // Blocks tree
+    auto blocks_tree_sibling_path = get_sibling_path<HISTORIC_BLOCKS_TREE_HEIGHT>(historic_blocks_tree, 0, 0);
+    info("problematic sibling path");


Should some of these infos be removed?

LHerskind · 2023-08-01T11:33:26Z

circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp

+        .next_available_leaf_index = 0,
+    };
+
+    info("snapshot");


More infos down here.

LHerskind · 2023-08-01T11:34:56Z

yarn-project/circuits.js/src/structs/rollup/root_rollup.ts

+    /**
+     * The hash of the block being included into the historic block hashes tree.
+     */
+    public blockHash: Fr,


As earlier. Not sure you actually need this.

LHerskind · 2023-08-01T11:39:09Z

yarn-project/sequencer-client/src/block_builder/solo_block_builder.ts

@@ -249,7 +248,7 @@ export class SoloBlockBuilder implements BlockBuilder {

    // Run the root rollup with the last two merge rollups (or base, if no merge layers)
    const [mergeOutputLeft, mergeOutputRight] = mergeRollupInputs;
-    return this.rootRollupCircuit(mergeOutputLeft, mergeOutputRight, newL1ToL2Messages);
+    return this.rootRollupCircuit(mergeOutputLeft, mergeOutputRight, newL1ToL2Messages, globalVariables);


The globalVariables should already be in both mergeOutputLeft and mergeOutputRight, you could just read it from there?

LHerskind

A few comments on indexing of historic trees

circuits/cpp/src/aztec3/circuits/hash.hpp

LHerskind · 2023-08-01T13:59:00Z

circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp

+    // Blocks tree snapshots
+    AppendOnlyTreeSnapshot const start_historic_blocks_tree_snapshot = {
+        .root = historic_blocks_tree.root(),
+        .next_available_leaf_index = 0,


The next available should probably be one here similarly to how the other historic trees are handled now?

fixing in follow up

LHerskind · 2023-08-01T13:59:25Z

circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp

@@ -378,6 +381,15 @@ RootRollupInputs get_root_rollup_inputs(utils::DummyBuilder& builder,
        .next_available_leaf_index = 1,
    };

+    // Blocks tree
+    auto blocks_tree_sibling_path = get_sibling_path<HISTORIC_BLOCKS_TREE_HEIGHT>(historic_blocks_tree, 0, 0);


Naming wise would add the historic to the name here for clarity. Should probably also be for index 1? As our historic starts there instead of at 0 where the initial was inserted?

fixing in followup

LHerskind · 2023-08-01T14:01:21Z

circuits/cpp/src/aztec3/circuits/rollup/test_utils/utils.cpp

@@ -351,6 +350,9 @@ RootRollupInputs get_root_rollup_inputs(utils::DummyBuilder& builder,
    MemoryStore l1_to_l2_msg_tree_store;
    MerkleTree l1_to_l2_msg_tree(l1_to_l2_msg_tree_store, L1_TO_L2_MSG_TREE_HEIGHT);

+    MemoryStore historic_blocks_tree_store;
+    MerkleTree historic_blocks_tree(historic_blocks_tree_store, HISTORIC_BLOCKS_TREE_HEIGHT);
+


Should probably be initialised with empty root for position 0 like we were doing with the others.

fixing in follow up

rahul-kothari

other than lasse's comments lgtm

rahul-kothari · 2023-08-02T09:46:53Z

circuits/cpp/src/aztec3/circuits/abis/rollup/root/root_rollup_inputs.hpp

@@ -16,18 +16,22 @@ template <typename NCT> struct RootRollupInputs {
    using fr = typename NCT::fr;

    // All below are shared between the base and merge rollups
-    std::array<PreviousRollupData<NCT>, 2> previous_rollup_data;
+    std::array<PreviousRollupData<NCT>, 2> previous_rollup_data{};


rahul-kothari · 2023-08-02T09:48:47Z

circuits/cpp/src/aztec3/circuits/abis/rollup/root/root_rollup_inputs.hpp

+    AppendOnlyTreeSnapshot<NCT> start_historic_tree_l1_to_l2_message_tree_roots_snapshot{};
+
+    // inputs required to add the block hash
+    AppendOnlyTreeSnapshot<NCT> start_historic_blocks_tree_snapshot{};


why do we not have to add start_historic_blocks_tree_snapshot and end_historic_blocks_tree_snapshot into the root rollup PublicInputs?

Edit: just realised it was always there in that file lol

rahul-kothari · 2023-08-02T09:58:43Z

yarn-project/sequencer-client/src/block_builder/solo_block_builder.test.ts

-    baseRollupOutputLeft = makeBaseOrMergeRollupPublicInputs();
-    baseRollupOutputRight = makeBaseOrMergeRollupPublicInputs();
-    rootRollupOutput = makeRootRollupPublicInputs();
+    baseRollupOutputLeft = makeBaseOrMergeRollupPublicInputs(0, blockNumber, globalVariables);


doesn't globalVariables have blockNumber already?

good point it does, i can refactor this to remove the arg

Maddiaa0 added 6 commits July 26, 2023 16:42

feat: add blocks tree data to l2 block

8553fa0

feat: use constants to represent offsets

4580093

chore: forge fmt

1991a57

fix: linter

e0808b8

fix: stub circuit output for the meantime

98692dd

feat: add naive block hash calculations to root rollup circuit

2942f95

Maddiaa0 added 8 commits July 27, 2023 14:18

Merge branch 'master' into md/blocks-tree-circuits

e8f9fbc

temp

376e71a

Merge branch 'master' into md/blocks-tree-circuits

9403830

fix: update snapshot

7520217

feat: include block hash calcs in sequencer

3c9422c

fix: make calculate block hash a cbind

de68edd

Merge branch 'master' into md/blocks-tree-circuits

520b9a6

feat: add block tree checking in ts

55fbd5f

Maddiaa0 marked this pull request as ready for review August 1, 2023 10:44

Maddiaa0 requested review from LHerskind and rahul-kothari August 1, 2023 11:11

LHerskind reviewed Aug 1, 2023

View reviewed changes

Maddiaa0 force-pushed the md/blocks-tree-circuits branch 2 times, most recently from e9f0626 to dc20631 Compare August 1, 2023 13:16

Maddiaa0 added 3 commits August 1, 2023 13:17

clean

f1d9fc7

lint

3ae5a4d

review comments

011a150

Maddiaa0 force-pushed the md/blocks-tree-circuits branch from dc20631 to 011a150 Compare August 1, 2023 13:17

Merge branch 'master' into md/blocks-tree-circuits

830721f

LHerskind reviewed Aug 1, 2023

View reviewed changes

Maddiaa0 added 3 commits August 1, 2023 14:29

fix: include generator indices

035c41f

lint

5340a79

update snapshot

d9bda7b

rahul-kothari reviewed Aug 2, 2023

View reviewed changes

Maddiaa0 added 2 commits August 2, 2023 10:34

fix: remove references to block num in factories

455058a

Merge branch 'master' into md/blocks-tree-circuits

515bb60

Maddiaa0 requested review from LHerskind and rahul-kothari August 2, 2023 10:37

Merge branch 'master' into md/blocks-tree-circuits

613e8be

Maddiaa0 enabled auto-merge (squash) August 2, 2023 13:24

rahul-kothari approved these changes Aug 2, 2023

View reviewed changes

Maddiaa0 merged commit 71dc039 into master Aug 2, 2023

Maddiaa0 deleted the md/blocks-tree-circuits branch August 2, 2023 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blocks_tree): compute block hashes within root rollup circuit #1214

feat(blocks_tree): compute block hashes within root rollup circuit #1214

Maddiaa0 commented Jul 26, 2023

netlify bot commented Jul 26, 2023 •

edited

Loading

LHerskind left a comment

LHerskind Aug 1, 2023

LHerskind Aug 1, 2023

LHerskind Aug 1, 2023

LHerskind Aug 1, 2023

LHerskind Aug 1, 2023

LHerskind left a comment

LHerskind Aug 1, 2023

Maddiaa0 Aug 1, 2023 •

edited

Loading

LHerskind Aug 1, 2023

Maddiaa0 Aug 1, 2023

LHerskind Aug 1, 2023

Maddiaa0 Aug 1, 2023

rahul-kothari left a comment

rahul-kothari Aug 2, 2023

rahul-kothari Aug 2, 2023

rahul-kothari Aug 2, 2023

Maddiaa0 Aug 2, 2023

		@@ -22,6 +22,8 @@ template <typename NCT> struct RootRollupPublicInputs {

		GlobalVariables<NCT> globalVariables;

		fr block_hash;

feat(blocks_tree): compute block hashes within root rollup circuit #1214

feat(blocks_tree): compute block hashes within root rollup circuit #1214

Conversation

Maddiaa0 commented Jul 26, 2023

Description

Checklist:

netlify bot commented Jul 26, 2023 • edited Loading

✅ Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maddiaa0 Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul-kothari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jul 26, 2023 •

edited

Loading

Maddiaa0 Aug 1, 2023 •

edited

Loading