refactor: archiver identifies prune #8666

LHerskind · 2024-09-20T09:29:50Z

Fixes #8620, by using the status function to figure out when a prune have happened and unwind state. It do NOT tell the world state synchronizer about the unwinding and that should be addressed as part of #8665.

Adds tests to e2e_synching making sure that an archiver will correctly catch the prune happening and delete the blocks and accompanying data. Also adds e2e_synching to earthfile.

Removes the proven_store since we learn this from the status and it is not more closely linked to the fetching of blocks.

Note that we are not handling L1 re-orgs specifically in here, but they will for some cases be caught as well. But not all, so there is still #8621 as well.

Fixes an issue where asking for block 0 would return block 1, as there is no block 0. Explicitly return undefined if block 0 is requested now, and gives an error if using getBlocks from a block that is before the initial block.

LHerskind · 2024-09-20T09:30:11Z

refactor: optimise l1 to l2 message fetching #8672
refactor: archiver identifies prune #8666 👈
refactor: minor archiver refactor #8715
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @LHerskind and the rest of your teammates on Graphite

Maddiaa0

Nice work, missing unit test + semantics question

Maddiaa0 · 2024-09-20T16:38:01Z

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

+        this.log.verbose(`L2 prune have occurred, unwind state`);
+
+        let tipAfterUnwind = localPendingBlockNumber;
+        while (true) {


This condition can be hit if the Pending chain reorgs from a prune

In which case the entire epoch will be dropped, correct me if im wrong but if this is the median case then we would make less requests if we start from the proven chain then move forward?

Essentially yes. I think I had a weird edge case where I needed to figure out whether to use the local proven or the destination proven and were like, I will just take the simple thing that works and then deal with optimisations later if they are really needed. As the expected max depth should be 2*epoch I seems ok to potentially do that when a prune happens, as it would not happen frequently in time, e.g., you cannot do 2 prunes right in a row because there is nothing to prune etc, so doing up to 60 reads at most every 30 minutes seems fine 🤷.

If running the updateProvenBlock in the case of localPendingBlockInChain that should probably be covered though.

Separately, I think when I was doing it, part of having it like this was the case where the l2 prune was caused by an L1 reorg such that the proven local chain was invalid and would also end up being unwound here, but that would not actually update our local proven block number in the same round which is kinda odd 🤷. Actually the updateProvenBlock should probably not only update if provenBlock have moved ahead since that is going to kinda nuke itself with a reorg caused by L1.

Maddiaa0 · 2024-09-20T16:52:42Z

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

+   * @param blocksToUnwind - The number of blocks we are to unwind
+   * @returns True if the operation is successful
+   */
+  unwindBlocks(from: number, blocksToUnwind: number): Promise<boolean>;


This is not tested in the archiver_store test

just-mitch · 2024-09-20T17:43:07Z

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

@@ -50,6 +50,15 @@ export interface ArchiverDataStore {
   */
  addBlocks(blocks: L1Published<L2Block>[]): Promise<boolean>;

+  /**
+   * Unwinds blocks from the database
+   * @param from -  The tip of the chain, passed for verification purposes,


The "tip of the chain" in the implementation is the most recently synced L2 block, but the two might be different.

Can you clarify a bit. I'm not following. Both of the values are based on the synched L2 block, e.g., the from and the value we are checking against, so we should not be updating it in the middle.

Got it- my question/concern was around how does the caller know what to pass as "from", and how would it know that it is the tip.

The caller should only really be the archiver, and it will be able to just fetch the information from the store directly. It so mainly to avoid someone doing a call to it directly.

just-mitch · 2024-09-20T17:44:01Z

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

   * @returns The requested L2 block.
   */
  public async getBlock(number: number): Promise<L2Block | undefined> {
    // If the number provided is -ve, then return the latest block.
    if (number < 0) {
      number = await this.store.getSynchedL2BlockNumber();
    }
+    if (number == 0) {


Isn't this genesis block? Should it be undefined?

There is a "genesis" state, but there is not actually a block that you have.

just-mitch · 2024-09-20T18:10:24Z

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

-      });
-    }
-  }
-
  private async handleL2blocks(blockUntilSynced: boolean, blocksSynchedTo: bigint, currentL1BlockNumber: bigint) {


It's really hard to follow all the logic branches in this function.

Will look at clearing it up.

just-mitch · 2024-09-20T18:14:45Z

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

@@ -321,7 +352,7 @@ export class Archiver implements ArchiveSource {
    );

    if (retrievedBlocks.length === 0) {
-      await this.store.setBlockSynchedL1BlockNumber(currentL1BlockNumber);
+      // We are not calling `setBlockSynchedL1BlockNumber` because it may cause sync issues if based off infura.


Can you explain this comment?

Also, shouldn't we expect that we retrieve blocks if my local pending block num is less than what the rollup says?

If you look higher up in the archiver there is a larger description around infura and consistency. So yes, while we SHOULD receive something. Infura might not return it to us because of the eventual consistency.

PhilWindle · 2024-09-21T20:38:21Z

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

+        if (block === undefined) {
+          throw new Error(`Cannot remove block ${blockNumber} from the store, we don't have it`);
+        }
+        void this.#blocks.delete(block.data.number);


Should we delete all the logs/contracts etc associated with the unwound blocks?

Yes. I forgot, different store.

Since all of these are added per block, seems like the apis could be made more convenient if we just have an addBlock and unwindBlock on the archiver_store and handle the logic in there. The archiver don't really need to know about these, as long as the store does.

LHerskind · 2024-09-23T09:12:01Z

Going to create a pr that will be "before" this in the stack looking at refactoring the archiver and its stores with api updates.

just-mitch · 2024-09-26T17:11:20Z

yarn-project/end-to-end/src/e2e_synching.test.ts

-      await rollup.write.prune();
-    };
+          sequencer?.updateSequencerConfig({ minTxsPerBlock: variant.txCount, maxTxsPerBlock: variant.txCount });
+          const txs = await variant.createAndSendTxs();


I'm kind of surprised this works.

The node has had its archive pruned, but the world state is still unpruned. So we would expect this to fail to prove/verify at the moment right? And the roots it is publishing are garbage?

Look at the start of that test.

it.skip('node following prunes and can extend chain', async () => { // @todo This test is to be activated when we can unwind the world state // It will currently stall forever as the state will never match.

Fixes #8620, by using the status function to figure out when a prune have happened and unwind state. It do NOT tell the world state synchronizer about the unwinding and that should be addressed as part of #8665. Adds tests to `e2e_synching` making sure that an archiver will correctly catch the prune happening and delete the blocks and accompanying data. Also adds e2e_synching to earthfile. Removes the `proven_store` since we learn this from the status and it is not more closely linked to the fetching of blocks. Note that we are not handling L1 re-orgs specifically in here, but they will for some cases be caught as well. But not all, so there is still #8621 as well. Fixes an issue where asking for block 0 would return block 1, as there is no block 0. Explicitly return undefined if block 0 is requested now, and gives an error if using `getBlocks` from a block that is before the initial block.

LHerskind marked this pull request as ready for review September 20, 2024 09:36

LHerskind force-pushed the lh/8620-handle-prune branch from 99da5ad to 708eb75 Compare September 20, 2024 09:46

LHerskind added the e2e-all CI: Enables this CI job. label Sep 20, 2024

LHerskind force-pushed the lh/8620-handle-prune branch from 708eb75 to 76c87ac Compare September 20, 2024 10:26

LHerskind requested review from Maddiaa0 and just-mitch September 20, 2024 11:10

LHerskind mentioned this pull request Sep 20, 2024

refactor: optimise l1 to l2 message fetching #8672

Merged

Maddiaa0 reviewed Sep 20, 2024

View reviewed changes

just-mitch reviewed Sep 20, 2024

View reviewed changes

PhilWindle reviewed Sep 21, 2024

View reviewed changes

LHerskind force-pushed the lh/8620-handle-prune branch from 76c87ac to a2820bf Compare September 24, 2024 11:11

LHerskind changed the base branch from master to lh/archiver-refactor September 24, 2024 11:11

LHerskind mentioned this pull request Sep 24, 2024

refactor: minor archiver refactor #8715

Merged

LHerskind force-pushed the lh/archiver-refactor branch 2 times, most recently from 0c7b804 to 6822f3f Compare September 24, 2024 22:27

LHerskind force-pushed the lh/8620-handle-prune branch from a2820bf to ccaa1be Compare September 24, 2024 22:27

LHerskind mentioned this pull request Sep 24, 2024

Epic: 🐢 Code Quality #7820

Open

LHerskind force-pushed the lh/archiver-refactor branch from 6822f3f to b12db36 Compare September 25, 2024 08:30

LHerskind force-pushed the lh/8620-handle-prune branch 2 times, most recently from 46715c6 to c79a869 Compare September 25, 2024 12:03

Base automatically changed from lh/archiver-refactor to master September 25, 2024 17:07

LHerskind force-pushed the lh/8620-handle-prune branch from c79a869 to f591972 Compare September 25, 2024 17:25

LHerskind added 3 commits September 25, 2024 22:01

refactor: archiver identifies prune

92ede54

chore: update test to have proven block

c707338

chore: address comments

7b683a1

LHerskind added 3 commits September 25, 2024 22:01

chore: unwinding work

eb5f8f4

feat: deleting instances/classes + extended test

ae2cddf

fix: update earthfile

adf1a0b

LHerskind force-pushed the lh/8620-handle-prune branch from f591972 to adf1a0b Compare September 25, 2024 22:01

LHerskind requested review from Maddiaa0 and just-mitch September 26, 2024 07:56

just-mitch reviewed Sep 26, 2024

View reviewed changes

just-mitch approved these changes Sep 26, 2024

View reviewed changes

LHerskind merged commit 4cf0f70 into master Sep 26, 2024
93 checks passed

LHerskind deleted the lh/8620-handle-prune branch September 26, 2024 17:51

AztecBot mentioned this pull request Sep 26, 2024

chore(master): Release 0.57.0 #8788

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: archiver identifies prune #8666

refactor: archiver identifies prune #8666

LHerskind commented Sep 20, 2024 •

edited

Loading

LHerskind commented Sep 20, 2024 •

edited

Loading

Maddiaa0 left a comment

Maddiaa0 Sep 20, 2024

LHerskind Sep 21, 2024 •

edited

Loading

Maddiaa0 Sep 20, 2024

just-mitch Sep 20, 2024

LHerskind Sep 21, 2024

just-mitch Sep 21, 2024

LHerskind Sep 24, 2024

just-mitch Sep 20, 2024

LHerskind Sep 21, 2024

just-mitch Sep 20, 2024

LHerskind Sep 21, 2024

just-mitch Sep 20, 2024

LHerskind Sep 21, 2024

PhilWindle Sep 21, 2024

LHerskind Sep 23, 2024

LHerskind Sep 23, 2024

LHerskind commented Sep 23, 2024

just-mitch Sep 26, 2024

LHerskind Sep 26, 2024

refactor: archiver identifies prune #8666

refactor: archiver identifies prune #8666

Conversation

LHerskind commented Sep 20, 2024 • edited Loading

LHerskind commented Sep 20, 2024 • edited Loading

Maddiaa0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind Sep 21, 2024 • 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

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 commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind commented Sep 20, 2024 •

edited

Loading

LHerskind commented Sep 20, 2024 •

edited

Loading

LHerskind Sep 21, 2024 •

edited

Loading