Feat: sol-ts parity checks for decoder-encoder #412

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

l1-contracts/test/Decoder.t.sol

  }
+
+  bytes shit_block =


This isnt used but lmao

Whoops, will remove 💩.

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

l1-contracts/test/Decoder.t.sol

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


Nice refactor on this, looks alot nicer

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

l1-contracts/test/DecoderHelper.sol

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


+1 for seperating

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

yarn-project/l1-contracts/src/viem-contracts/DecoderHelper.ts

@@ -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

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

+      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

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

+      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

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

@@ -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

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

@@ -65,6 +210,9 @@ async function deployRollup() {
  const ethRpc = new EthereumRpc(provider);

  // 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

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

@@ -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

yarn-project/types/src/l2_block.ts

+      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

l1-contracts/src/core/Decoder.sol

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


Suggested change

// Kernel1.contract.aztecaddress

// Kernel1.contract.aztecAddress

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

l1-contracts/src/core/Decoder.sol

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


Suggested change

// Kernel2.contract.aztecaddress

// Kernel2.contract.aztecAddress

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

yarn-project/types/src/l2_block.ts

+    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?

l1-contracts/src/core/Decoder.sol

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

l1-contracts/src/core/Decoder.sol Show resolved Hide resolved

l1-contracts/src/core/Decoder.sol Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: sol-ts parity checks for decoder-encoder #412

Feat: sol-ts parity checks for decoder-encoder #412

LHerskind commented Apr 28, 2023 •

edited by benesjan

Loading

Maddiaa0 left a comment

Maddiaa0 Apr 29, 2023

LHerskind Apr 29, 2023

iAmMichaelConnor May 2, 2023

Maddiaa0 Apr 29, 2023

Maddiaa0 Apr 29, 2023

Maddiaa0 Apr 29, 2023

LHerskind Apr 29, 2023

Maddiaa0 Apr 29, 2023

Maddiaa0 Apr 29, 2023

Maddiaa0 Apr 29, 2023

LHerskind Apr 29, 2023

benesjan May 2, 2023

benesjan left a comment

benesjan May 2, 2023

benesjan May 2, 2023

benesjan May 2, 2023

benesjan May 2, 2023

benesjan May 2, 2023

benesjan May 2, 2023 •

edited

Loading

LHerskind May 2, 2023

benesjan May 2, 2023

LHerskind May 2, 2023

LHerskind May 2, 2023

		@@ -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

Feat: sol-ts parity checks for decoder-encoder #412

Feat: sol-ts parity checks for decoder-encoder #412

Conversation

LHerskind commented Apr 28, 2023 • edited by benesjan Loading

Description

Checklist:

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

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

Choose a reason for hiding this comment

benesjan May 2, 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

LHerskind commented Apr 28, 2023 •

edited by benesjan

Loading

benesjan May 2, 2023 •

edited

Loading