feat(contracts): use constants where possible #1238

rahul-kothari · 2023-07-27T12:56:50Z

Description

Unsure about the best name for constantsPerBase or the leafDataLengthsPerBase but these reduce constants, don't increase gas too much (increase of ~400) and are a way to avoid stack too deep error

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-27T12:56:55Z

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

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

LHerskind

Think we can do some cleanup that also helps a bit on the performance for when there are more kernels etc.

LHerskind · 2023-07-27T13:09:53Z

l1-contracts/src/core/libraries/Decoder.sol

@@ -302,8 +302,15 @@ library Decoder {

      // Create the leaf to contain commitments (2 * COMMITMENTS_PER_TX * 020) + nullifiers (2 * NULLIFIERS_PER_TX * 0x20)
      // + new public data writes (8 * 0x40) + contract deployments (2 * 0x60) + logs hashes (2 * 4 * 0x20)
+      ArrayLengths memory constantsLengthsPerBase;


Think the name should be reconsidered, it is a bit odd. The length seems weird when it is not something that changes but might just be me. Also the commitmentCount and such that you get from the ArrayLengths just sounds strange to me when like this.

Also, consider if you actually want to store the count, or if we are better of storing the number of bytes instead.

If you look at where it is used, it is mostly needing the number of bytes to jump and such so we can skip some later computation if we had those instead.

If we are storing bytes, we could

// Stack too deep reee struct MemoryByteConstants { uint256 commitmentBytesPerKernel; uint256 nullifierBytesPerKernel; ... }

Then the baseleaf later is just the sum of those. And we skip a lot of multiplications later in the loop. Can be used for the offsets then as well (around line 440)

LHerskind · 2023-07-27T13:11:38Z

l1-contracts/src/core/libraries/Decoder.sol

      vars.baseLeaf =
-      new bytes(2 * Constants.COMMITMENTS_PER_TX * 0x20 + 2 * Constants.NULLIFIERS_PER_TX * 0x20 + 2 * Constants.PUBLIC_DATA_WRITES_PER_TX * 0x40 + 2 * Constants.CONTRACTS_PER_TX * 0x60 + 2 * 4 * 0x20);
+      new bytes(constantsLengthsPerBase.commitmentCount * 0x20 + constantsLengthsPerBase.nullifierCount * 0x20 + constantsLengthsPerBase.dataWritesCount * 0x40 + constantsLengthsPerBase.contractCount * 0x60 + 2 * 4 * 0x20);


Can you add the note on 2 * 4 * 0x20, add to the constants to have the logs accounted for.

LHerskind · 2023-07-27T13:17:08Z

l1-contracts/src/core/libraries/Decoder.sol

        assembly {
          let baseLeaf := mload(add(vars, 0x40)) // Load the pointer to `vars.baseLeaf`
          let dstPtr := add(baseLeaf, 0x20) // Current position withing `baseLeaf` to write to
+          let leafDataLengthPerBase := mload(constantsLengthsPerBase)


Think you should rename this as well, it is the memoryConstantPtr that you are then looping over to progress nicely.

changed it to something much better ahha ;)

Maddiaa0

This is a great step in the right direction, however i think we can go further. We should be able to remove all of the assembly that isn-t calldatacopys / pointer overloading for most of the assembly blocks below.

I made a branch md/asm-purge that attempts to start it off

LHerskind

Nits and grits

LHerskind · 2023-07-27T18:15:11Z

l1-contracts/src/core/libraries/Decoder.sol

      vars.baseLeaf =
-      new bytes(2 * Constants.COMMITMENTS_PER_TX * 0x20 + 2 * Constants.NULLIFIERS_PER_TX * 0x20 + 2 * Constants.PUBLIC_DATA_WRITES_PER_TX * 0x40 + 2 * Constants.CONTRACTS_PER_TX * 0x60 + 2 * 4 * 0x20);
+      new bytes(Constants.COMMITMENTS_NUM_BYTES_PER_BASE + Constants.NULLIFIERS_NUM_BYTES_PER_BASE + Constants.PUBLIC_DATA_WRITES_NUM_BYTES_PER_BASE + Constants.CONTRACTS_NUM_BYTES_PER_BASE + Constants.CONTRACT_DATA_NUM_BYTES_PER_BASE + 2 * 4 * 0x20);


The 2*4*0x20 pains my eyes. Everything else is with constants why not this?

LHerskind · 2023-07-27T18:23:39Z

l1-contracts/src/core/libraries/Constants.sol

@@ -20,4 +20,19 @@ library Constants {
  uint256 internal constant CONTRACTS_PER_TX = 1;
  uint256 internal constant L2_TO_L1_MSGS_PER_TX = 2;
  uint256 internal constant L1_TO_L2_MSGS_PER_ROLLUP = 16;
+
+  // above constants but in bytes


Would not say that it is really the the above but in bytes, above you have per txs, below you have per base in bytes.

Also, it is not fully consistent if you use BASE_ROLLUP or just BASE.

yea I actually just updated the names cos I hated them

LHerskind · 2023-07-27T18:24:16Z

l1-contracts/src/core/libraries/Constants.sol

+  uint256 internal constant CONTRACT_DATA_NUM_BYTES_PER_BASE =
+    TXS_PER_BASE_ROLLUP * CONTRACTS_PER_TX * 0x40; //aztec address + eth address (padded to 0x20)
+  uint256 internal constant L2_TO_L1_MSGS_NUM_BYTES_PER_BASE =
+    TXS_PER_BASE_ROLLUP * L2_TO_L1_MSGS_PER_TX * 0x20;


Remember to include logs so we don't have the "random" values in the decoder for those values.

LHerskind · 2023-07-27T18:27:41Z

l1-contracts/src/core/libraries/Decoder.sol

-          mstore(dstPtr, mload(add(vars, 0x60))) // `encryptedLogsHashKernel1` starts at 0x60 in `vars`
+        // encryptedLogsHashKernel1
+        dstPtr += 0x14;
+        bytes32 hash = vars.encrypedLogsHashKernel1;


If the ordering of encryptedLogsHashKernel1 etc are changed in the ConsumablesVars you should be able to load in all 4 at once with identity instead of having to jump through one at a time.

true, but this is nice and clear for now

LHerskind · 2023-07-27T18:30:02Z

l1-contracts/src/core/libraries/Decoder.sol

+        dstPtr += 0x20;
+        hash = vars.unencryptedLogsHashKernel2;
+        assembly {
+          mstore(dstPtr, hash)
        }

        offsets.commitmentOffset += 2 * Constants.COMMITMENTS_PER_TX * 0x20;


Why not change the values down here to use constants as well when possible?

Maddiaa0 · 2023-07-28T11:18:42Z

l1-contracts/src/core/libraries/Constants.sol

@@ -20,4 +20,19 @@ library Constants {
  uint256 internal constant CONTRACTS_PER_TX = 1;
  uint256 internal constant L2_TO_L1_MSGS_PER_TX = 2;
  uint256 internal constant L1_TO_L2_MSGS_PER_ROLLUP = 16;
+  uint256 internal constant KERNELS_PER_ROLLUP = 2;


this could also be generated from the cpp script right? lets make a ticket and add it

edit: issue here #1261

Are you talking about BASE_ROLLUPS as ROLLUPS? It gets pretty confusing if you remove that, as now it looks like you are trying to load all bytes in the rollup into a base later on.

@rahul-kothari I don't think you handled this. The ROLLUP instead of BASE_ROLLUP or BASE make me think it is actually a root rollup which it is not.

LHerskind · 2023-07-28T12:51:33Z

l1-contracts/src/core/libraries/Decoder.sol

+      // + new public data writes (2 * PUBLIC_DATA_WRITES_PER_TX * 0x40) + contract deployments (2 * CONTRACTS_PER_TX * 0x60) + logs hashes (2 * 4 * 0x20)
+      // contract deployments is 0x60 since 0x20 for contract data root, 0x40 to store the contract's aztec address and eth address (padded to 32 bytes)
+      // log hash is 4 * 0x20 since each log hash is divided into two field elements. And there are two kinds of logs - enrypted, unencrypted. Hence "4 * 0x20"
+
      vars.baseLeaf =


Something looks wrong here. Where are the L2 -> L1? Should sit in between the public and contracts.

Ah, found the issue. The current constant for L2 -> L1 message is 4. So it was included in the 2*4*0x20... The log hashes num bytes per rollup is 0x40 per tx (2 hashes), and as they are only intermediates, they can be used as 32 bytes even if shown as 2 fields in circuits.

ah shit amazing catch

# Description Fixes #1263

# Description In the latest nightly, noir libs will not compile without a name field in the package.json. This pr adds the name field to allow stuff to compile 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](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) 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.

# Description Fixes #359 and #624. # Checklist: - [ ] I have reviewed my diff in github, line by line. - [ ] Every change is related to the PR description. - [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) 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.

…onstants

LHerskind · 2023-07-31T14:05:31Z

Fixes #953

LHerskind

LGTM

rahul-kothari requested review from LHerskind and Maddiaa0 July 27, 2023 12:56

LHerskind requested changes Jul 27, 2023

View reviewed changes

Maddiaa0 reviewed Jul 27, 2023

View reviewed changes

rahul-kothari force-pushed the rk/sc_constants branch from f096059 to 35251f1 Compare July 27, 2023 18:11

rahul-kothari requested review from Maddiaa0 and LHerskind July 27, 2023 18:11

rahul-kothari force-pushed the rk/sc_constants branch from 35251f1 to 1d34b6b Compare July 27, 2023 18:21

LHerskind reviewed Jul 27, 2023

View reviewed changes

Maddiaa0 changed the title ~~use constants~~ feat(contracts): use constants where possible Jul 28, 2023

Maddiaa0 reviewed Jul 28, 2023

View reviewed changes

rahul-kothari and others added 5 commits July 28, 2023 12:18

use constants

b53d023

feat: purge asm

7141bdf

make things cleaner

93a730d

better naming

3da4337

cleanup

18b522f

rahul-kothari force-pushed the rk/sc_constants branch from da71879 to 18b522f Compare July 28, 2023 12:18

LHerskind reviewed Jul 28, 2023

View reviewed changes

add l2 to l1 messages in baseleaf bytes

be762e4

rahul-kothari requested review from LHerskind and Maddiaa0 July 28, 2023 14:24

benesjan and others added 8 commits July 30, 2023 10:20

feat: L1 to L2 message size checks (#1262)

085d6f2

# Description Fixes #1263

feat: use solidity slices

93ccdd9

refactor: add slice wrapper

0818501

fix: proper padding

2554643

use constants

19250b4

feat: purge asm

59b86a4

rahul-kothari added 4 commits July 31, 2023 10:21

make things cleaner

48f84e6

better naming

226c964

cleanup

3e6832f

add l2 to l1 messages in baseleaf bytes

ab366ed

rahul-kothari force-pushed the rk/sc_constants branch from be762e4 to ab366ed Compare July 31, 2023 10:21

rahul-kothari added 4 commits July 31, 2023 10:41

Merge remote-tracking branch 'origin/lh/solidity_slices' into rk/sc_c…

85d621c

…onstants

specify base_rollup

d18e5a1

Merge branch 'master' into rk/sc_constants

1e19ab8

formatting will kill me someday

217047f

LHerskind linked an issue Jul 31, 2023 that may be closed by this pull request

Cleanup decoder.sol constants #953

Closed

LHerskind approved these changes Jul 31, 2023

View reviewed changes

Maddiaa0 approved these changes Jul 31, 2023

View reviewed changes

rahul-kothari merged commit 6c6a31b into master Jul 31, 2023

rahul-kothari deleted the rk/sc_constants branch July 31, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(contracts): use constants where possible #1238

feat(contracts): use constants where possible #1238

rahul-kothari commented Jul 27, 2023

netlify bot commented Jul 27, 2023 •

edited

Loading

LHerskind left a comment

LHerskind Jul 27, 2023

LHerskind Jul 27, 2023

LHerskind Jul 27, 2023

rahul-kothari Jul 27, 2023

Maddiaa0 left a comment •

edited

Loading

LHerskind left a comment

LHerskind Jul 27, 2023

LHerskind Jul 27, 2023

rahul-kothari Jul 27, 2023

LHerskind Jul 27, 2023

LHerskind Jul 27, 2023

Maddiaa0 Jul 28, 2023 •

edited

Loading

LHerskind Jul 27, 2023

Maddiaa0 Jul 28, 2023 •

edited

Loading

LHerskind Jul 28, 2023

LHerskind Jul 31, 2023

LHerskind Jul 28, 2023

LHerskind Jul 28, 2023

rahul-kothari Jul 28, 2023

LHerskind commented Jul 31, 2023

LHerskind left a comment

feat(contracts): use constants where possible #1238

feat(contracts): use constants where possible #1238

Conversation

rahul-kothari commented Jul 27, 2023

Description

Checklist:

netlify bot commented Jul 27, 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

Maddiaa0 left a comment • edited Loading

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

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 Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maddiaa0 Jul 28, 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

LHerskind commented Jul 31, 2023

LHerskind left a comment

Choose a reason for hiding this comment

netlify bot commented Jul 27, 2023 •

edited

Loading

Maddiaa0 left a comment •

edited

Loading

Maddiaa0 Jul 28, 2023 •

edited

Loading

Maddiaa0 Jul 28, 2023 •

edited

Loading