refactor: from < genesis allowed in getBlocks #2816

benesjan · 2023-10-12T08:35:11Z

Fixes #411

renamed getL2Blocks in archiver as getBlocks to make it consistent with the naming in AztecNode interface.
removed the getBlocksLength function from archiver interface. It's redundant because getBlockNumber can be used instead.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

AztecBot · 2023-10-12T09:37:59Z

Benchmark results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit a3649df0 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric	8 txs	32 txs	128 txs
l1_rollup_calldata_size_in_bytes	45,444	179,588	716,132
l1_rollup_calldata_gas	222,972	868,172	3,446,288
l1_rollup_execution_gas	842,059	3,595,280	22,201,657
l2_block_processing_time_in_ms	1,026 (-1%)	3,942	15,584
note_successful_decrypting_time_in_ms	329 (-1%)	1,011 (-1%)	3,733 (-2%)
note_trial_decrypting_time_in_ms	⚠️ 31.0 (-11%)	108	138 (-1%)
l2_block_building_time_in_ms	9,009	36,017	151,065 (-1%)
l2_block_rollup_simulation_time_in_ms	6,687	26,730	106,346
l2_block_public_tx_process_time_in_ms	2,283 (-1%)	9,160 (-1%)	44,088 (-3%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric	5 blocks	10 blocks
node_history_sync_time_in_ms	14,710 (+2%)	31,926 (+2%)
note_history_successful_decrypting_time_in_ms	2,379 (+1%)	4,819 (+2%)
note_history_trial_decrypting_time_in_ms	123 (+2%)	150 (+1%)
node_database_size_in_bytes	1,646,214	1,190,009
pxe_database_size_in_bytes	27,188	54,187

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit	circuit_simulation_time_in_ms	circuit_input_size_in_bytes	circuit_output_size_in_bytes
private-kernel-init	44.0	56,577	14,745
private-kernel-ordering	21.8 (+2%)	20,137	8,089
base-rollup	852	631,605	811
root-rollup	38.2 (+1%)	4,072	1,097
private-kernel-inner	36.8 (+1%)	72,288	14,745
public-kernel-private-input	46.8 (-1%)	37,359	14,745
public-kernel-non-first-iteration	28.2	37,401	14,745
merge-rollup	0.899 (+2%)	2,592	873

rahul-kothari · 2023-10-12T10:17:38Z

yarn-project/archiver/src/archiver/archiver.ts

@@ -209,7 +208,7 @@ export class Archiver implements L2BlockSource, L2LogsSource, ContractDataSource
    // ********** Events that are processed per block **********

    // Read all data from chain and then write to our stores at the end
-    const nextExpectedL2BlockNum = BigInt(this.store.getBlocksLength() + INITIAL_L2_BLOCK_NUM);
+    const nextExpectedL2BlockNum = BigInt((await this.store.getBlockNumber()) + 1);


Doesn't this break if we restart the archiver? Say L2 is at block 30. Say archiver restarts. So nextExpectedL2BlockNum would be 1. But I suppose we don't care about re-syncing until testnet?

It would have been one in the first instance too?

This seems correct to me because when we start archiver the next expected L2 block num is 1 because that's the first block.

yarn-project/archiver/src/archiver/archiver_store.test.ts

rahul-kothari · 2023-10-12T10:26:57Z

yarn-project/archiver/src/archiver/archiver_store.ts

    }
-    if (from < INITIAL_L2_BLOCK_NUM || from > this.l2BlockContexts.length) {
+
+    const fromIndex = Math.max(from - INITIAL_L2_BLOCK_NUM, 0);


we use INITIAL_L2_BLOCK_NUM here but not in the archiver? Is that an issue?

It was being used in an overloaded context in the archiver. ( incrementing to next block / getting the first block number )

Maddiaa0 · 2023-10-12T10:24:23Z

yarn-project/types/src/l2_block_downloader/l2_block_downloader.ts

@@ -87,7 +87,7 @@ export class L2BlockDownloader {
   * @param timeout - optional timeout value to prevent permanent blocking
   * @returns The next batch of blocks from the queue.
   */
-  public async getL2Blocks(timeout?: number) {
+  public async getBlocks(timeout?: number): Promise<L2Block[]> {


Should L2Block just be renamed to block? To keep with this naming trend?

I think there is some value in having the "L2" in the block type name because it clarifies that it's an L2 block and not an L1 block. I didn't like the getL2Blocks name simply because everywhere else in codebase it's getBlocks.

Maddiaa0 · 2023-10-12T10:42:19Z

yarn-project/archiver/src/archiver/archiver.ts

@@ -209,7 +208,7 @@ export class Archiver implements L2BlockSource, L2LogsSource, ContractDataSource
    // ********** Events that are processed per block **********

    // Read all data from chain and then write to our stores at the end
-    const nextExpectedL2BlockNum = BigInt(this.store.getBlocksLength() + INITIAL_L2_BLOCK_NUM);
+    const nextExpectedL2BlockNum = BigInt((await this.store.getBlockNumber()) + 1);


It would have been one in the first instance too?

Maddiaa0 · 2023-10-12T10:43:32Z

yarn-project/archiver/src/archiver/archiver_store.ts

    }
-    if (from < INITIAL_L2_BLOCK_NUM || from > this.l2BlockContexts.length) {
+
+    const fromIndex = Math.max(from - INITIAL_L2_BLOCK_NUM, 0);


It was being used in an overloaded context in the archiver. ( incrementing to next block / getting the first block number )

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.8.11</summary> ## [0.8.11](aztec-packages-v0.8.10...aztec-packages-v0.8.11) (2023-10-13) ### Features * **archiver:** Use registry to fetch searchStartBlock ([#2830](#2830)) ([e5bc067](e5bc067)) * Configure sandbox for network ([#2818](#2818)) ([d393a59](d393a59)) * **docker-sandbox:** Allow forks in sandbox ([#2831](#2831)) ([ed8431c](ed8431c)), closes [#2726](#2726) * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](#2802)) ([3c3cd9f](3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](#2795)) ([b36fdc4](b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](#2805)) ([b3d1f28](b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](#2790)) ([9a354c9](9a354c9)) * Integrate ZeroMorph into Honk ([#2774](#2774)) ([ea86869](ea86869)) * Purge non native token + reorder params in token portal ([#2723](#2723)) ([447dade](447dade)) * Throw compile error if read/write public state from private ([#2804](#2804)) ([a3649df](a3649df)) * Unencrypted log filtering ([#2600](#2600)) ([7ae554a](7ae554a)), closes [#1498](#1498) [#1500](#1500) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](#2764)) ([32c69ae](32c69ae)) ### Bug Fixes * Outdated `noir:clean` ([#2821](#2821)) ([2ea199f](2ea199f)) ### Miscellaneous * Benchmark tx sizes in p2p pool ([#2810](#2810)) ([f63219c](f63219c)) * Change acir_tests branch to point to master ([#2815](#2815)) ([73f229d](73f229d)) * Fix typo ([#2839](#2839)) ([5afdf91](5afdf91)) * From < genesis allowed in getBlocks ([#2816](#2816)) ([5622b50](5622b50)) * Remove Ultra Grumpkin flavor ([#2825](#2825)) ([bde77b8](bde77b8)) * Remove work queue from honk ([#2814](#2814)) ([bca7d12](bca7d12)) * Spell check ([#2817](#2817)) ([4777a11](4777a11)) ### Documentation * Slight changes to update portal page ([#2799](#2799)) ([eb65819](eb65819)) * Update aztec_connect_sunset.mdx ([#2808](#2808)) ([5f659a7](5f659a7)) </details> <details><summary>barretenberg.js: 0.8.11</summary> ## [0.8.11](barretenberg.js-v0.8.10...barretenberg.js-v0.8.11) (2023-10-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.8.11</summary> ## [0.8.11](barretenberg-v0.8.10...barretenberg-v0.8.11) (2023-10-13) ### Features * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](#2802)) ([3c3cd9f](3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](#2795)) ([b36fdc4](b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](#2805)) ([b3d1f28](b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](#2790)) ([9a354c9](9a354c9)) * Integrate ZeroMorph into Honk ([#2774](#2774)) ([ea86869](ea86869)) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](#2764)) ([32c69ae](32c69ae)) ### Miscellaneous * Change acir_tests branch to point to master ([#2815](#2815)) ([73f229d](73f229d)) * Remove Ultra Grumpkin flavor ([#2825](#2825)) ([bde77b8](bde77b8)) * Remove work queue from honk ([#2814](#2814)) ([bca7d12](bca7d12)) * Spell check ([#2817](#2817)) ([4777a11](4777a11)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@aztec-packages-v0.8.10...aztec-packages-v0.8.11) (2023-10-13) ### Features * **archiver:** Use registry to fetch searchStartBlock ([#2830](AztecProtocol/aztec-packages#2830)) ([e5bc067](AztecProtocol/aztec-packages@e5bc067)) * Configure sandbox for network ([#2818](AztecProtocol/aztec-packages#2818)) ([d393a59](AztecProtocol/aztec-packages@d393a59)) * **docker-sandbox:** Allow forks in sandbox ([#2831](AztecProtocol/aztec-packages#2831)) ([ed8431c](AztecProtocol/aztec-packages@ed8431c)), closes [#2726](AztecProtocol/aztec-packages#2726) * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](AztecProtocol/aztec-packages#2802)) ([3c3cd9f](AztecProtocol/aztec-packages@3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](AztecProtocol/aztec-packages#2795)) ([b36fdc4](AztecProtocol/aztec-packages@b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](AztecProtocol/aztec-packages#2805)) ([b3d1f28](AztecProtocol/aztec-packages@b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](AztecProtocol/aztec-packages#2790)) ([9a354c9](AztecProtocol/aztec-packages@9a354c9)) * Integrate ZeroMorph into Honk ([#2774](AztecProtocol/aztec-packages#2774)) ([ea86869](AztecProtocol/aztec-packages@ea86869)) * Purge non native token + reorder params in token portal ([#2723](AztecProtocol/aztec-packages#2723)) ([447dade](AztecProtocol/aztec-packages@447dade)) * Throw compile error if read/write public state from private ([#2804](AztecProtocol/aztec-packages#2804)) ([a3649df](AztecProtocol/aztec-packages@a3649df)) * Unencrypted log filtering ([#2600](AztecProtocol/aztec-packages#2600)) ([7ae554a](AztecProtocol/aztec-packages@7ae554a)), closes [#1498](AztecProtocol/aztec-packages#1498) [#1500](AztecProtocol/aztec-packages#1500) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](AztecProtocol/aztec-packages#2764)) ([32c69ae](AztecProtocol/aztec-packages@32c69ae)) ### Bug Fixes * Outdated `noir:clean` ([#2821](AztecProtocol/aztec-packages#2821)) ([2ea199f](AztecProtocol/aztec-packages@2ea199f)) ### Miscellaneous * Benchmark tx sizes in p2p pool ([#2810](AztecProtocol/aztec-packages#2810)) ([f63219c](AztecProtocol/aztec-packages@f63219c)) * Change acir_tests branch to point to master ([#2815](AztecProtocol/aztec-packages#2815)) ([73f229d](AztecProtocol/aztec-packages@73f229d)) * Fix typo ([#2839](AztecProtocol/aztec-packages#2839)) ([5afdf91](AztecProtocol/aztec-packages@5afdf91)) * From < genesis allowed in getBlocks ([#2816](AztecProtocol/aztec-packages#2816)) ([5622b50](AztecProtocol/aztec-packages@5622b50)) * Remove Ultra Grumpkin flavor ([#2825](AztecProtocol/aztec-packages#2825)) ([bde77b8](AztecProtocol/aztec-packages@bde77b8)) * Remove work queue from honk ([#2814](AztecProtocol/aztec-packages#2814)) ([bca7d12](AztecProtocol/aztec-packages@bca7d12)) * Spell check ([#2817](AztecProtocol/aztec-packages#2817)) ([4777a11](AztecProtocol/aztec-packages@4777a11)) ### Documentation * Slight changes to update portal page ([#2799](AztecProtocol/aztec-packages#2799)) ([eb65819](AztecProtocol/aztec-packages@eb65819)) * Update aztec_connect_sunset.mdx ([#2808](AztecProtocol/aztec-packages#2808)) ([5f659a7](AztecProtocol/aztec-packages@5f659a7)) </details> <details><summary>barretenberg.js: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@barretenberg.js-v0.8.10...barretenberg.js-v0.8.11) (2023-10-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@barretenberg-v0.8.10...barretenberg-v0.8.11) (2023-10-13) ### Features * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](AztecProtocol/aztec-packages#2802)) ([3c3cd9f](AztecProtocol/aztec-packages@3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](AztecProtocol/aztec-packages#2795)) ([b36fdc4](AztecProtocol/aztec-packages@b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](AztecProtocol/aztec-packages#2805)) ([b3d1f28](AztecProtocol/aztec-packages@b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](AztecProtocol/aztec-packages#2790)) ([9a354c9](AztecProtocol/aztec-packages@9a354c9)) * Integrate ZeroMorph into Honk ([#2774](AztecProtocol/aztec-packages#2774)) ([ea86869](AztecProtocol/aztec-packages@ea86869)) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](AztecProtocol/aztec-packages#2764)) ([32c69ae](AztecProtocol/aztec-packages@32c69ae)) ### Miscellaneous * Change acir_tests branch to point to master ([#2815](AztecProtocol/aztec-packages#2815)) ([73f229d](AztecProtocol/aztec-packages@73f229d)) * Remove Ultra Grumpkin flavor ([#2825](AztecProtocol/aztec-packages#2825)) ([bde77b8](AztecProtocol/aztec-packages@bde77b8)) * Remove work queue from honk ([#2814](AztecProtocol/aztec-packages#2814)) ([bca7d12](AztecProtocol/aztec-packages@bca7d12)) * Spell check ([#2817](AztecProtocol/aztec-packages#2817)) ([4777a11](AztecProtocol/aztec-packages@4777a11)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

benesjan force-pushed the janb/allowing-from-block-be-less-than-genesis branch from c7eba4c to 6ee827d Compare October 12, 2023 09:20

rahul-kothari reviewed Oct 12, 2023

View reviewed changes

Maddiaa0 approved these changes Oct 12, 2023

View reviewed changes

benesjan added 6 commits October 12, 2023 12:22

from block can be less than genesis

abf4a2b

refactor: consistent block functions names

3c81585

fix + cleanup

9830c73

linter fix

68e3fca

rahul nit fix

3d094d6

better naming

df1a603

benesjan force-pushed the janb/allowing-from-block-be-less-than-genesis branch from 9a8b028 to df1a603 Compare October 12, 2023 12:22

benesjan enabled auto-merge (squash) October 12, 2023 12:35

rahul-kothari approved these changes Oct 12, 2023

View reviewed changes

benesjan merged commit 5622b50 into master Oct 12, 2023

benesjan deleted the janb/allowing-from-block-be-less-than-genesis branch October 12, 2023 12:39

AztecBot mentioned this pull request Oct 12, 2023

chore(master): Release 0.8.11 #2811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: from < genesis allowed in getBlocks #2816

refactor: from < genesis allowed in getBlocks #2816

benesjan commented Oct 12, 2023 •

edited

Loading

AztecBot commented Oct 12, 2023 •

edited

Loading

rahul-kothari Oct 12, 2023

Maddiaa0 Oct 12, 2023

benesjan Oct 12, 2023 •

edited

Loading

rahul-kothari Oct 12, 2023

Maddiaa0 Oct 12, 2023

Maddiaa0 Oct 12, 2023

benesjan Oct 12, 2023

Maddiaa0 Oct 12, 2023

Maddiaa0 Oct 12, 2023

refactor: from < genesis allowed in getBlocks #2816

refactor: from < genesis allowed in getBlocks #2816

Conversation

benesjan commented Oct 12, 2023 • edited Loading

Checklist:

AztecBot commented Oct 12, 2023 • edited Loading

Benchmark results

L2 block published to L1

L2 chain processing

Circuits stats

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesjan Oct 12, 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

Choose a reason for hiding this comment

benesjan commented Oct 12, 2023 •

edited

Loading

AztecBot commented Oct 12, 2023 •

edited

Loading

benesjan Oct 12, 2023 •

edited

Loading