From 14e5b7bf0b110a5da761ad84aeb7dede2ec01fca Mon Sep 17 00:00:00 2001
From: Alex Gherghisan <alexg@aztecprotocol.com>
Date: Mon, 16 Oct 2023 18:31:02 +0000
Subject: [PATCH] fix: abort block building if block height changes

---
 .../src/sequencer/sequencer.test.ts           | 32 +++++++++++++++++--
 .../src/sequencer/sequencer.ts                | 19 +++++++++--
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts
index 28c1acb992da..e198f2645ca6 100644
--- a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts
+++ b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts
@@ -9,7 +9,7 @@ import { P2P, P2PClientState } from '@aztec/p2p';
 import { L1ToL2MessageSource, L2Block, L2BlockSource, MerkleTreeId, Tx, TxHash, mockTx } from '@aztec/types';
 import { MerkleTreeOperations, WorldStateRunningState, WorldStateSynchronizer } from '@aztec/world-state';
 
-import { MockProxy, mock } from 'jest-mock-extended';
+import { MockProxy, mock, mockFn } from 'jest-mock-extended';
 import times from 'lodash.times';
 
 import { BlockBuilder } from '../block_builder/index.js';
@@ -65,7 +65,7 @@ describe('sequencer', () => {
     });
 
     l2BlockSource = mock<L2BlockSource>({
-      getBlockNumber: () => Promise.resolve(lastBlockNumber),
+      getBlockNumber: mockFn().mockResolvedValue(lastBlockNumber),
     });
 
     l1ToL2MessageSource = mock<L1ToL2MessageSource>({
@@ -181,6 +181,34 @@ describe('sequencer', () => {
     expect(publisher.processL2Block).toHaveBeenCalledWith(block);
     expect(p2p.deleteTxs).toHaveBeenCalledWith([await invalidChainTx.getTxHash()]);
   });
+
+  fit('aborts building a block if the chain moves underneath it', async () => {
+    const tx = mockTx();
+    tx.data.constants.txContext.chainId = chainId;
+    const block = L2Block.random(lastBlockNumber + 1);
+    const proof = makeEmptyProof();
+
+    p2p.getTxs.mockResolvedValueOnce([tx]);
+    blockBuilder.buildL2Block.mockResolvedValueOnce([block, proof]);
+    publisher.processL2Block.mockResolvedValueOnce(true);
+    globalVariableBuilder.buildGlobalVariables.mockResolvedValueOnce(
+      new GlobalVariables(chainId, version, new Fr(lastBlockNumber + 1), Fr.ZERO),
+    );
+
+    await sequencer.initialSync();
+
+    l2BlockSource.getBlockNumber
+      // let it work for a bit
+      .mockResolvedValueOnce(lastBlockNumber)
+      .mockResolvedValueOnce(lastBlockNumber)
+      .mockResolvedValueOnce(lastBlockNumber)
+      // then tell it to abort
+      .mockResolvedValue(lastBlockNumber + 1);
+
+    await sequencer.work();
+
+    expect(publisher.processL2Block).not.toHaveBeenCalled();
+  });
 });
 
 class TestSubject extends Sequencer {
diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.ts
index 930ffb532876..5623022d371f 100644
--- a/yarn-project/sequencer-client/src/sequencer/sequencer.ts
+++ b/yarn-project/sequencer-client/src/sequencer/sequencer.ts
@@ -130,6 +130,17 @@ export class Sequencer {
       this.log.info(`Retrieved ${pendingTxs.length} txs from P2P pool`);
 
       const blockNumber = (await this.l2BlockSource.getBlockNumber()) + 1;
+
+      /**
+       * We'll call this function before running expensive operations to avoid wasted work.
+       */
+      const assertBlockHeight = async () => {
+        const currentBlockNumber = await this.l2BlockSource.getBlockNumber();
+        if (currentBlockNumber + 1 !== blockNumber) {
+          throw new Error('New block was emitted while building block');
+        }
+      };
+
       const newGlobalVariables = await this.globalsBuilder.buildGlobalVariables(new Fr(blockNumber));
 
       // Filter out invalid txs
@@ -155,7 +166,7 @@ export class Sequencer {
       // Only accept processed transactions that are not double-spends,
       // public functions emitting nullifiers would pass earlier check but fail here.
       // Note that we're checking all nullifiers generated in the private execution twice,
-      // we could store the ones already checked and skip them here as an optimisation.
+      // we could store the ones already checked and skip them here as an optimization.
       const processedValidTxs = await this.takeValidTxs(processedTxs, newGlobalVariables);
 
       if (processedValidTxs.length === 0) {
@@ -163,6 +174,8 @@ export class Sequencer {
         return;
       }
 
+      await assertBlockHeight();
+
       // Get l1 to l2 messages from the contract
       this.log('Requesting L1 to L2 messages from contract');
       const l1ToL2Messages = await this.getPendingL1ToL2Messages();
@@ -171,6 +184,8 @@ export class Sequencer {
       // Build the new block by running the rollup circuits
       this.log(`Assembling block with txs ${processedValidTxs.map(tx => tx.hash).join(', ')}`);
 
+      await assertBlockHeight();
+
       const emptyTx = await processor.makeEmptyProcessedTx();
       const [rollupCircuitsDuration, block] = await elapsed(() =>
         this.buildBlock(processedValidTxs, l1ToL2Messages, emptyTx, newGlobalVariables),
@@ -189,7 +204,7 @@ export class Sequencer {
       await this.publishL2Block(block);
       this.log.info(`Submitted rollup block ${block.number} with ${processedValidTxs.length} transactions`);
     } catch (err) {
-      this.log.error(`Rolling back world state DB due to error assembling block`, err);
+      this.log.error(`Rolling back world state DB due to error assembling block`, (err as any).stack);
       await this.worldState.getLatest().rollback();
     }
   }