L2 -> L1 msgs, sol/cpp/ts #469

LHerskind · 2023-05-04T22:29:53Z

Description

Fixes #381

Add component for calldata hash computation from 4 field elements (useful for testing to compute root calldata etc)
Includes L2 -> L1 msgs in calldata hash computation
Fixes issues in sol decoder unrelated to L2 -> L1 (improper offsets)
Updates TS to use historic tree instead of fresh tree for treeOfHistoricL1ToL2MsgTreeRootsSnapshot
Updates KERNEL_NEW_L2_TO_L1_MSGS_LENGTH to 2 to follow same pattern as nullifier and commitments.
Add ts tests to generate empty and "mixed" block (mixed including everything) that are useful for the sol setup.

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.

LHerskind · 2023-05-05T08:06:16Z

In #463 @benesjan, is adding proper integration for the sol/ts, would be good to get that in here as well.

rahul-kothari

Don't have too much knowledge about decoder.sol but everything else looks good barring some comments I left.

rahul-kothari · 2023-05-05T10:53:32Z

circuits/cpp/src/aztec3/circuits/rollup/root/.test.cpp

    std::vector<uint8_t> const calldata_hash_input_bytes_vec(hash_input.begin(), hash_input.end());
-
    auto calldata_hash = sha256::sha256(calldata_hash_input_bytes_vec);


nit: i know you didn't do this, but can you rename this to expected_calldata_hash

rahul-kothari · 2023-05-05T10:55:42Z

circuits/cpp/src/aztec3/constants.hpp

@@ -23,7 +23,7 @@ constexpr size_t KERNEL_NEW_NULLIFIERS_LENGTH = 4;
 constexpr size_t KERNEL_NEW_CONTRACTS_LENGTH = 1;
 constexpr size_t KERNEL_PRIVATE_CALL_STACK_LENGTH = 8;
 constexpr size_t KERNEL_PUBLIC_CALL_STACK_LENGTH = 8;
-constexpr size_t KERNEL_NEW_L2_TO_L1_MSGS_LENGTH = 4;
+constexpr size_t KERNEL_NEW_L2_TO_L1_MSGS_LENGTH = 2;


How is this different to NEW_L2_TO_L1_MSGS_LENGTH?
Also each tx can now have 2 L2<>L1 messages?

In the same manner as KERNEL_NEW_COMMITMENTS_LENGTH etc. One kernel might have multiple inner function calls and such so it can be useful to allow kernels with more than individual calls. However, for now thought that it made sense that the values were the same similar to what it is for the commitments and nullifiers.

rahul-kothari · 2023-05-05T10:59:48Z

l1-contracts/src/core/Decoder.sol

+ *  | 0x240                         | x          | newCommits
+ *  | 0x240 + x                     | 0x04       | len(newNullifiers) denoted y
+ *  | 0x244 + x                     | y          | newNullifiers
+ *  | 0x244 + a + b                 | 0x04       | len(newPublicDataWrites) denoted c


shouldn't this be 'y'?

if you mean on line 52 it should still be x, however; x and y should be renamed to a and b

Good catch with the naming. Will fix and handle some proper lengths

rahul-kothari · 2023-05-05T11:02:18Z

l1-contracts/src/core/Decoder.sol

+ *  | 0x24c + a + b + c + d         | 0x04       | len(newContracts) denoted e
+ *  | 0x250 + a + b + c + d         | e          | newContracts (each element 32 bytes)
+ *  | 0x250 + a + b + c + d + e     | e          | newContractData (each element 52 bytes)
+ *  | 0x250 + a + b + c + d + e     | 0x04       | len(l1ToL2Messages) denoted f


should be 2e no? Since we are adding e twice

The notation right now is somewhat sloppy as neither a,b,c,d,e,f are byte lengths. Might just fix that entirely and make the width of the table grow.

yarn-project/types/src/l2_block.ts

Maddiaa0

overall looks good, have some minor comments

Maddiaa0 · 2023-05-05T10:41:01Z

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

@@ -72,6 +73,13 @@ std::array<fr, NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP> get_empty_l1_to_l2_messages(
    return l1_to_l2_messages;
 }

+void set_kernel_l2_to_l1_msgs(KernelData& kernel_data, std::array<fr, KERNEL_NEW_L2_TO_L1_MSGS_LENGTH> l2_to_l1_msgs)
+{
+    for (size_t i = 0; i < KERNEL_NEW_L2_TO_L1_MSGS_LENGTH; i++) {


could you not just assign the entire array rather than individually?

kernel_data.public_inputs.end.new_l2_to_l2_msg = l2_to_l1_msgs

Should probably also change for the other setters.

Maddiaa0 · 2023-05-05T10:51:39Z

l1-contracts/src/core/Decoder.sol

      offsets.contractDataOffset = offsets.contractOffset + lengths.contractCount * 0x20;
-      offsets.l1ToL2MessagesOffset = offsets.contractDataOffset + lengths.contractCount * 0x54;
+      offsets.l1ToL2MessagesOffset = offsets.contractDataOffset + 0x4 + lengths.contractCount * 0x34;


Maddiaa0 · 2023-05-05T10:59:12Z

yarn-project/end-to-end/src/integration_l1_publisher.test.ts

-    tx.data.end.newContracts = [makeNewContractData(seed + 0x1000)];
-    return tx;
-  };
+  const makeBloatedProcessedTx = async (seed = 0x1) => {


This is repeated in solo_block_builder.test.ts, is there a case for it to be moved to factories?

Is makePopulatedProcessedTx a better name than bloated?

I used bloated as naming here as the txs was full of everything. Think we could remove it from solo block builder tests as the blocks is essentially the same as integration test which is just doing an extra check on top.

Maddiaa0 · 2023-05-05T11:05:56Z

yarn-project/end-to-end/src/integration_l1_publisher.test.ts

+      const [block] = await builder.buildL2Block(1 + i, txs, l1ToL2Messages);
+
+      /*// Useful for sol tests block generation
+      const encoded = block.encode();


this is lingering although its probably convienent to keep

Kept it like this to make it easy for later use. The sol decoder test is there really only for us to more easily extend and test new decoders in foundry.

LHerskind mentioned this pull request May 4, 2023

L2 to L1 messages #382

Closed

6 tasks

LHerskind force-pushed the lh/l2->l1 branch from b78541a to b43a276 Compare May 4, 2023 22:51

LHerskind marked this pull request as ready for review May 4, 2023 23:29

LHerskind requested review from rahul-kothari and Maddiaa0 May 4, 2023 23:29

LHerskind added 7 commits May 5, 2023 09:51

feat: cpp l2 -> l1

684a725

feat: l2 -> l1 ts

a6a9025

fix: historic tree + filled blocks

841a23b

fix: sol decoder

268f665

fix: update snapshots (4->2 l2->l1 msgs)

350333c

fix: update snapshot post rebase

0c949d4

fix: integration l1 publisher test

dfe0448

LHerskind force-pushed the lh/l2->l1 branch from de6b9c1 to dfe0448 Compare May 5, 2023 10:42

chore: make sol test block generation comment

81518e8

rahul-kothari reviewed May 5, 2023

View reviewed changes

yarn-project/types/src/l2_block.ts Show resolved Hide resolved

Maddiaa0 reviewed May 5, 2023

View reviewed changes

LHerskind added 2 commits May 5, 2023 11:43

chore: decoder outline improvement

9d4b980

chore: remove unneeded helpers

8433052

rahul-kothari self-requested a review May 5, 2023 12:02

rahul-kothari approved these changes May 5, 2023

View reviewed changes

LHerskind merged commit 88783b9 into master May 5, 2023

LHerskind deleted the lh/l2->l1 branch May 5, 2023 13:02

LHerskind mentioned this pull request May 9, 2023

contracts: Remove hardcoded test data #414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L2 -> L1 msgs, sol/cpp/ts #469

L2 -> L1 msgs, sol/cpp/ts #469

LHerskind commented May 4, 2023 •

edited

Loading

LHerskind commented May 5, 2023

rahul-kothari left a comment

rahul-kothari May 5, 2023

rahul-kothari May 5, 2023

LHerskind May 5, 2023

rahul-kothari May 5, 2023

Maddiaa0 May 5, 2023 •

edited

Loading

LHerskind May 5, 2023

rahul-kothari May 5, 2023

LHerskind May 5, 2023

Maddiaa0 left a comment

Maddiaa0 May 5, 2023

LHerskind May 5, 2023

Maddiaa0 May 5, 2023

Maddiaa0 May 5, 2023

Maddiaa0 May 5, 2023

LHerskind May 5, 2023

Maddiaa0 May 5, 2023

LHerskind May 5, 2023

		std::vector<uint8_t> const calldata_hash_input_bytes_vec(hash_input.begin(), hash_input.end());

		auto calldata_hash = sha256::sha256(calldata_hash_input_bytes_vec);

L2 -> L1 msgs, sol/cpp/ts #469

L2 -> L1 msgs, sol/cpp/ts #469

Conversation

LHerskind commented May 4, 2023 • edited Loading

Description

Checklist:

LHerskind commented May 5, 2023

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

Maddiaa0 May 5, 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

Maddiaa0 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind commented May 4, 2023 •

edited

Loading

Maddiaa0 May 5, 2023 •

edited

Loading