Add E2E calldataHash test (sol/ts/cpp) #431

LHerskind · 2023-05-02T18:46:12Z

Description

Fixes #390 and #399.

Updates calldata hash in cpp to include the public writes;
Check that cpp/sol/ts all get same calldata hash;
Update contract tree prep in test_utils/utils.cpp to have proper index and tree generation.
Removes unused standalone_block_builder
Renames circuit_block_builder to solo_block_builder

l2-block-publisher tests are unfunctional since #408 was merged in. But @benesjan is addresseing this with #458.

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.

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

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

circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp

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

spalladino · 2023-05-03T17:38:55Z

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

@@ -183,7 +180,7 @@ TEST_F(root_rollup_tests, native_check_block_hashes_empty_blocks)
    EXPECT_FALSE(composer.failed());

    // Expected hash of public inputs for an empty L2 block. Also used in the contract tests.
-    fr const expected_hash = uint256_t("11840efc30e9fcbdd0aae30da2a5b441132420b4f0cc4ffd6bdc41888845f775");
+    fr const expected_hash = uint256_t("23f698e487c4e1b7383889f8864bc19591773d136c364c01d980bd13d0786ba1");


How do we recalculate this value? Is it viable to calculate it on the fly in the test instead of hardcoding?

Its values from the l2-block-publisher.test.ts, there to make sure that they are on par.

@LHerskind please put there a comment that the value is from l2-block-publisher.test.ts

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

spalladino · 2023-05-03T17:40:39Z

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

@@ -120,6 +120,43 @@ export class RootRollupPublicInputs {
    return new RootRollupPublicInputs(...RootRollupPublicInputs.getFields(fields));
  }

+  static makeFake() {


Let's rename to empty()

Was my original name, but used makefake as it cointained the aggregation object that was named makefake so assumed there was a reason the underlying was not called empty and that it should convey that as well.

spalladino · 2023-05-03T17:43:58Z

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

      expect(l2Block.number).toEqual(blockNumber);
+      expect(l2Block.getCalldataHash()).toEqual(rootRollupPublicInputs.sha256CalldataHash());


If this check is the only reason why we're exposing the root rollup simulation output from the block builder, then I'd make it a responsibility of the block builder to make this check instead, and revert the change to its interface.

@Cheethas and I spoke about it. Main annoyance with that it broke some tests, but probably better to fix those mocks anyway.

spalladino · 2023-05-03T17:47:20Z

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

Goodbye StandaloneBlockBuilder, you served well!

We could use this opportunity to rename the circuit_block_builder to just block_builder. WDYT?

please i beg

Will do, what should we do with the interface? Currently there is a BlockBuilder interface, should we still keep that or just have the block builder?

On interfaces, is there a reason we are not naming them starting on I or similar, always have a headache looking through the code and not knowing if it is an interface or not before following it to the root.

Changed it to SoloBlockBuilder as we will probably see the distributed block builder further down the road, and not having issues with interface and class having same name

spalladino · 2023-05-03T17:48:15Z

circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp

+    // 8 commitments (4 per kernel) -> 8 fields
+    // 8 nullifiers (4 per kernel) -> 8 fields
+    // 8 public state transitions (4 per kernel) -> 16 fields
+    // 2 contract deployments (1 per kernel) -> 6 fields
+    std::array<NT::fr, 38> calldata_hash_inputs;


Do you think this 38 could be derived out of constants in constants.hpp using constexprs?

Yes, as long as we are not adding new types or anything but extending current lists should be fine.

spalladino · 2023-05-03T17:53:41Z

yarn-project/sequencer-client/src/test/l2-block-publisher.test.ts

+    return await makeProcessedTx(publicTx, kernelOutput, makeProof());
+  };
+
+  it('Build 2 blocks of 4 txs building on each other', async () => {


Why not use the actual block builder here, rather than manually assembling the block? Seems like it'd lead to less duplication.

Just good ol' debt.

benesjan

Just went through the comments from Santiago and Sean and they mostly seem sufficiently addressed. Once you address my comments feel free to merge it.

benesjan · 2023-05-04T10:15:03Z

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

-        kernel_data[1].public_inputs.end.new_commitments[i] = fr(i + 4 + 1);
+    std::vector<uint8_t> input_data = test_utils::utils::get_empty_calldata_leaf();
+
+    // Update commitment and nullifiers in kernels and testing byte array.


Suggested change

// Update commitment and nullifiers in kernels and testing byte array.

// Update commitments and nullifiers in kernels and testing byte array.

benesjan · 2023-05-04T10:15:28Z

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

+
+    // Update commitment and nullifiers in kernels and testing byte array.
+    // Commitments and nullifiers are 32 bytes long, so we can update them in the byte array by offsetting by 32 bytes
+    // for every insertion As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32) To insert


Suggested change

// for every insertion As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32) To insert

// for every insertion. As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32). To insert

benesjan · 2023-05-04T10:15:50Z

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

+    // Update commitment and nullifiers in kernels and testing byte array.
+    // Commitments and nullifiers are 32 bytes long, so we can update them in the byte array by offsetting by 32 bytes
+    // for every insertion As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32) To insert
+    // correctly, the insertions of values from the second kernel must be offset by 4*32 bytes (kernel_offset) Further


Suggested change

// correctly, the insertions of values from the second kernel must be offset by 4*32 bytes (kernel_offset) Further

// correctly, the insertions of values from the second kernel must be offset by 4*32 bytes (kernel_offset). Further

benesjan · 2023-05-04T10:20:27Z

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

@@ -183,7 +180,7 @@ TEST_F(root_rollup_tests, native_check_block_hashes_empty_blocks)
    EXPECT_FALSE(composer.failed());

    // Expected hash of public inputs for an empty L2 block. Also used in the contract tests.
-    fr const expected_hash = uint256_t("11840efc30e9fcbdd0aae30da2a5b441132420b4f0cc4ffd6bdc41888845f775");
+    fr const expected_hash = uint256_t("23f698e487c4e1b7383889f8864bc19591773d136c364c01d980bd13d0786ba1");


@LHerskind please put there a comment that the value is from l2-block-publisher.test.ts

LHerskind changed the title ~~test: add E2E calldataHash (sol/ts/cpp)~~ Add E2E calldataHash test (sol/ts/cpp) May 2, 2023

LHerskind marked this pull request as ready for review May 2, 2023 19:14

LHerskind requested review from Maddiaa0, spalladino and rahul-kothari and removed request for Maddiaa0 May 2, 2023 19:14

LHerskind marked this pull request as draft May 3, 2023 09:37

LHerskind marked this pull request as ready for review May 3, 2023 09:54

This was referenced May 3, 2023

feat: l1 to l2 messaging #385

Merged

contracts: Remove hardcoded test data #414

Closed

LHerskind linked an issue May 3, 2023 that may be closed by this pull request

Extend E2E to use contract and check state #399

Closed

Maddiaa0 reviewed May 3, 2023

View reviewed changes

circuits/cpp/src/aztec3/circuits/rollup/base/.test.cpp Outdated Show resolved Hide resolved

circuits/cpp/src/aztec3/circuits/rollup/merge/.test.cpp Outdated Show resolved Hide resolved

LHerskind marked this pull request as draft May 3, 2023 13:01

LHerskind force-pushed the lh/circuit-public-calldata-hash branch from d1702c4 to 2a95d3a Compare May 3, 2023 17:25

LHerskind mentioned this pull request May 3, 2023

Extend E2E public input hash test with L1 -> L2 messages #448

Closed

spalladino reviewed May 3, 2023

View reviewed changes

LHerskind force-pushed the lh/circuit-public-calldata-hash branch from 615c7e2 to 8daacbe Compare May 4, 2023 06:51

LHerskind added 10 commits May 4, 2023 09:51

test: add E2E calldataHash (sol/ts/cpp)

7cd21fe

fix: contract tree test preparation

747b08b

fix: include pub data roots in public input hash computation (cpp)

5207015

fix: rebasing issues -> can run one block but breaks on second

863682c

fix: address cpp comments

d6f6a36

fix: address ts comments

2f66fe1

chore: prettier

57d5255

fix: tidying

b4e3a42

fix: tidy issue

68208e8

fix: add nolint

1ab7588

benesjan mentioned this pull request May 4, 2023

Move l2-block-publisher.test.ts to e2e and re-enable it #458

Closed

LHerskind force-pushed the lh/circuit-public-calldata-hash branch from 7fab833 to 1ab7588 Compare May 4, 2023 10:09

benesjan self-requested a review May 4, 2023 10:21

benesjan approved these changes May 4, 2023

View reviewed changes

fix: addes @benesjan comments

ca790d0

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

fix: explicit return buffer

5fb918c

Maddiaa0 approved these changes May 4, 2023

View reviewed changes

LHerskind merged commit 9abcbc9 into master May 4, 2023

LHerskind deleted the lh/circuit-public-calldata-hash branch May 4, 2023 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add E2E calldataHash test (sol/ts/cpp) #431

Add E2E calldataHash test (sol/ts/cpp) #431

LHerskind commented May 2, 2023 •

edited

Loading

spalladino May 3, 2023

LHerskind May 4, 2023

benesjan May 4, 2023

spalladino May 3, 2023

LHerskind May 4, 2023

spalladino May 3, 2023

LHerskind May 4, 2023

spalladino May 3, 2023

Maddiaa0 May 3, 2023

LHerskind May 4, 2023 •

edited

Loading

LHerskind May 4, 2023 •

edited

Loading

LHerskind May 4, 2023

spalladino May 3, 2023

LHerskind May 4, 2023

spalladino May 3, 2023

LHerskind May 4, 2023

benesjan left a comment

benesjan May 4, 2023

benesjan May 4, 2023

benesjan May 4, 2023

benesjan May 4, 2023

		expect(l2Block.number).toEqual(blockNumber);
		expect(l2Block.getCalldataHash()).toEqual(rootRollupPublicInputs.sha256CalldataHash());

	// Update commitment and nullifiers in kernels and testing byte array.
	// Update commitments and nullifiers in kernels and testing byte array.

	// for every insertion As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32) To insert
	// for every insertion. As there are two kernels in every leaf, nullifiers are offset by 8 elements (8*32). To insert

	// correctly, the insertions of values from the second kernel must be offset by 4*32 bytes (kernel_offset) Further
	// correctly, the insertions of values from the second kernel must be offset by 4*32 bytes (kernel_offset). Further

Add E2E calldataHash test (sol/ts/cpp) #431

Add E2E calldataHash test (sol/ts/cpp) #431

Conversation

LHerskind commented May 2, 2023 • edited Loading

Description

Checklist:

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 May 4, 2023 • edited Loading

Choose a reason for hiding this comment

LHerskind May 4, 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

Choose a reason for hiding this comment

benesjan 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

LHerskind commented May 2, 2023 •

edited

Loading

LHerskind May 4, 2023 •

edited

Loading

LHerskind May 4, 2023 •

edited

Loading