Feat: sol-ts parity checks for decoder-encoder by LHerskind · Pull Request #412 · AztecProtocol/aztec-packages

LHerskind · 2023-04-28T23:23:29Z

Description

Updates computation of calldata hash, needs to be extended in native implementation as well to have parity between CPP, SOL and TS (see #413).

The calldata hash between CPP and (SOL/TS) will not hit parity until BB is fixed to make WASM and CPP use same generators for hash computations.

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.

Maddiaa0

Some comments from a little onceover

Maddiaa0 · 2023-04-29T11:52:01Z

+    }
  }
+
+  bytes shit_block =


This isnt used but lmao

Whoops, will remove 💩.

Maddiaa0 · 2023-04-29T11:52:26Z

-      "Invalid diff root"
-    );
-  }
+  function testEmptyBlocks() public {


Nice refactor on this, looks alot nicer

Maddiaa0 · 2023-04-29T11:52:36Z

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.18;


+1 for seperating

Maddiaa0 · 2023-04-29T11:57:40Z

@@ -0,0 +1,63 @@
+/**


rather than all of these files with bytecode copied and pasted, would we just be better off symlinking the artifacts like what was done in AC?

I think @benesjan already have a separate PR (#408) for handling some of this, which was why i did not want to make more changes to how it worked here.

Maddiaa0 · 2023-04-29T12:00:13Z

+      await setTxHistoricTreeRoots(tx);
+
+      // Calculate what would be the tree roots after the txs from the first base rollup land and update mock circuit output
+      await updateExpectedTreesFromTxs(txsLeft);


this could be moved into a helper function as it shows up alot in other sequencer tests

Maddiaa0 · 2023-04-29T12:00:33Z

+      const decodedRes = await decodeHelper.methods.decode(block.encode()).call();
+      const stateInRollup = await rollup.methods.rollupStateHash().call();
+
+      // @note There seems to be something wrong here. The Bytes32 returned are actually strings :(


Is there a ticket for this?

Maddiaa0 · 2023-04-29T12:01:06Z

@@ -14,17 +38,38 @@ const anvilHost = process.env.ANVIL_HOST ?? 'http://127.0.0.1:8545';
 const chainId = 31337;

 describe.skip('L1Publisher integration', () => {


This is currently skipped?

The tests were set as skipped before due to requiring an anvil instance running, which was not part of the setup. Maybe @ludamad got insights on better solution. Here simply replacing the old block publisher tests.

This is a tech debt so I created a ticket for it. Would make sense to address this in separate PR.

benesjan

Minor comments.

benesjan · 2023-05-02T07:35:53Z


  // Deploy Rollup and unverifiedDataEmitter contracts
+  const decodeHelper = new DecoderHelper(ethRpc, undefined, { from: deployer, gas: 1e6 });
+  await decodeHelper.deploy().send().getReceipt();


would rename decodeHelper to decoderHelper for consistency + please update the comment above.

benesjan · 2023-05-02T07:39:46Z

@@ -14,17 +38,38 @@ const anvilHost = process.env.ANVIL_HOST ?? 'http://127.0.0.1:8545';
 const chainId = 31337;

 describe.skip('L1Publisher integration', () => {


This is a tech debt so I created a ticket for it. Would make sense to address this in separate PR.

benesjan · 2023-05-02T07:53:52Z

+      this.getCalldataHash(),
+    );
+    const temp = toBigIntBE(sha256(buf));
+    const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');


Suggested change

const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');

// Prime order of BN254 curve

const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');

benesjan · 2023-05-02T07:54:20Z

        )
+        dstOffset := add(dstOffset, mul(2, 0x20))
+
+        // Kernel1.contract.aztecaddress


Suggested change

// Kernel1.contract.aztecaddress

// Kernel1.contract.aztecAddress

benesjan · 2023-05-02T07:54:34Z

        )
+        dstOffset := add(dstOffset, 0x20)
+
        // Kernel2.contract.aztecaddress


Suggested change

// Kernel2.contract.aztecaddress

// Kernel2.contract.aztecAddress

benesjan · 2023-05-02T07:59:46Z

+    for (let i = 0; i < leafCount; i++) {
+      const commitmentPerBase = KERNEL_NEW_COMMITMENTS_LENGTH * 2;
+      const nullifierPerBase = KERNEL_NEW_NULLIFIERS_LENGTH * 2;
+      const publicDataWritesPerBase = STATE_TRANSITIONS_LENGTH * 2; // @note why is this constant named differently?


@note why is this constant named differently?

What name did you expect here?

If you look at the commitments and nullifier constants you see that there are KERNEL_ prefix making it clear that this is per kernel, for STATE_TRANSITIONS_LENTGH it is not clear if per kernel, per rollup or whatever it is. Also that it is called STATE_TRANSITIONS here but public data writes other places, and state transition could also be the general one from StartStateHash to EndStateHash.

Ok, good catch. In that case replacing STATE_TRANSITIONS with KERNEL_PUB_DATA_WRITES everywhere sounds good to me. It should probably be done in a separate PR but I don't really care if you do it in the same one. If you are too busy with other things I would have time to pick it up once we merge this.

I can do it. Have one for L2 -> L1 stuff that right now is part of my other pr, but might be better to just have one with the naming 🤷

@benesjan or @PhilWindle, is there both a function limit and a kernel limit, and do they differ? e.g., for new commitments, there are both a function limit and a kernel limit but the value is the same atm, but have you used the value as a function or kenel?

LHerskind and others added 2 commits April 28, 2023 23:21

feat: sol-ts parity checks

d9e2500

Merge branch 'master' into lh/e2e_encoder_decoder

c65afc6

LHerskind mentioned this pull request Apr 28, 2023

Base rollup calldata parity #413

Closed

LHerskind requested a review from Maddiaa0 April 28, 2023 23:42

LHerskind marked this pull request as ready for review April 28, 2023 23:42

Maddiaa0 reviewed Apr 29, 2023

View reviewed changes

benesjan mentioned this pull request Apr 29, 2023

Contract artifact propagation tech debt #408

Merged

6 tasks

fix: remove shit_block

6719db8

LHerskind requested a review from benesjan April 29, 2023 13:39

benesjan requested changes May 2, 2023

View reviewed changes

fix: address comments

90bfa11

benesjan approved these changes May 2, 2023

View reviewed changes

chore: explain magic numbers

1e568c0

Maddiaa0 approved these changes May 2, 2023

View reviewed changes

Comment thread l1-contracts/src/core/Decoder.sol

Comment thread l1-contracts/src/core/Decoder.sol

Maddiaa0 merged commit 6470364 into master May 2, 2023

Maddiaa0 deleted the lh/e2e_encoder_decoder branch May 2, 2023 09:46

This was referenced May 3, 2023

contracts: Remove hardcoded test data #414

Closed

Extend TS l2block with helpers for state hashs etc #404

Closed

ludamad pushed a commit that referenced this pull request Jul 14, 2023

Ultra Honk (#412)

9074ab3

This was referenced Jul 25, 2023

chore(aztec-sandbox): Release 0.2.0-alpha27 #1180

Closed

chore(aztec-sandbox): Release 0.2.0 #1181

Closed

codygunton pushed a commit that referenced this pull request Jan 23, 2024

Ultra Honk (#412)

6ac16d0

		@@ -0,0 +1,19 @@
		// SPDX-License-Identifier: UNLICENSED
		pragma solidity >=0.8.18;

		@@ -14,17 +38,38 @@ const anvilHost = process.env.ANVIL_HOST ?? 'http://127.0.0.1:8545';
		const chainId = 31337;

		describe.skip('L1Publisher integration', () => {

	const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');
	// Prime order of BN254 curve
	const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');

	// Kernel1.contract.aztecaddress
	// Kernel1.contract.aztecAddress

	// Kernel2.contract.aztecaddress
	// Kernel2.contract.aztecAddress

Conversation

LHerskind commented Apr 28, 2023 • edited by benesjan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LHerskind commented Apr 28, 2023 •

edited by benesjan

Loading

benesjan May 2, 2023 •

edited

Loading