feat: add mined txs back to pending set

yarn-project/p2p/src/client/p2p_client.test.ts

@@ -45,6 +45,7 @@ describe('In-Memory P2P Client', () => {
       getPendingTxHashes: jest.fn().mockReturnValue([]),
       getTxStatus: jest.fn().mockReturnValue(undefined),
       markAsMined: jest.fn(),
+      markMinedAsPending: jest.fn(),
     };
 
     p2pService = {
@@ -295,6 +296,31 @@ describe('In-Memory P2P Client', () => {
       expect(txPool.deleteTxs).toHaveBeenCalledWith([badTx.getTxHash()]);
       await client.stop();
     });
+
+    it('moves mined and valid txs back to the pending set', async () => {
+      client = new P2PClient(kvStore, blockSource, mempools, p2pService, 10, telemetryClient);
+      blockSource.setProvenBlockNumber(0);
+      await client.start();
+
+      // add two txs to the pool. One build against block 90, one against block 95
+      // then prune the chain back to block 90
+      // only one tx should be deleted
+      const goodTx = mockTx();
+      goodTx.data.constants.globalVariables.blockNumber = new Fr(90);
+
+      const badTx = mockTx();
+      badTx.data.constants.globalVariables.blockNumber = new Fr(95);
+
+      txPool.getAllTxs.mockReturnValue([goodTx, badTx]);
+      txPool.getMinedTxHashes.mockReturnValue([goodTx.getTxHash()]);
+
+      blockSource.removeBlocks(10);
+      await sleep(150);
+      expect(txPool.deleteTxs).toHaveBeenCalledWith([badTx.getTxHash()]);
+      await sleep(150);
+      expect(txPool.markMinedAsPending).toHaveBeenCalledWith([goodTx.getTxHash()]);
+      await client.stop();
+    });
   });
 
   // TODO(https://github.com/AztecProtocol/aztec-packages/issues/7971): tests for attestation pool pruning

yarn-project/p2p/src/client/p2p_client.ts

@@ -616,23 +616,23 @@ export class P2PClient extends WithTracer implements P2P {
       }
     }
 
-    // TODO (alexg): Delete or re-add txs that were created against the proven block but mined in one of the pruned blocks
-    // e.g. I create a tx against proven block 42 but it the sequencer includes it in block 45. The chain gets pruned back to 42.
-    // That tx now lingers in the pool as 'mined' but it really is no longer mined. It's also not technically invalid.
-
     this.log.info(
       `Detected chain prune. Removing invalid txs count=${
         txsToDelete.length
       } newLatestBlock=${latestBlock} previousLatestBlock=${this.getSyncedLatestBlockNum()}`,
     );
+
+    // delete invalid txs (both pending and mined)
     await this.txPool.deleteTxs(txsToDelete);
+    // everything left in the mined set was built against a block on the proven chain so its still valid
+    // move back to pending set
+    await this.txPool.markMinedAsPending(this.txPool.getMinedTxHashes());
+
     await this.synchedLatestBlockNumber.set(latestBlock);
-    await this.synchedProvenBlockNumber.set(latestBlock);
     // no need to update block hashes, as they will be updated as new blocks are added
   }
 
   private async startServiceIfSynched() {
-    // TODO (alexg): I don't think this check works if there's a reorg
     if (
       this.currentState === P2PClientState.SYNCHING &&
       this.getSyncedLatestBlockNum() >= this.latestBlockNumberAtStart &&

yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts

@@ -56,6 +56,32 @@ export class AztecKVTxPool implements TxPool {
     });
   }
 
+  public markMinedAsPending(txHashes: TxHash[]): Promise<void> {
+    if (txHashes.length === 0) {
+      return Promise.resolve();
+    }
+
+    return this.#store.transaction(() => {
+      let deleted = 0;
+      let added = 0;
+      for (const hash of txHashes) {
+        const key = hash.toString();
+        if (this.#minedTxs.has(key)) {
+          deleted++;
+          void this.#minedTxs.delete(key);
+        }
+
+        if (this.#txs.has(key)) {
+          added++;
+          void this.#pendingTxs.add(key);
+        }
+      }
+
+      this.#metrics.recordRemovedObjects(deleted, 'mined');
+      this.#metrics.recordAddedObjects(added, 'pending');
+    });
+  }
+
   public getPendingTxHashes(): TxHash[] {
     return Array.from(this.#pendingTxs.entries()).map(x => TxHash.fromString(x));
   }

yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts

@@ -41,6 +41,32 @@ export class InMemoryTxPool implements TxPool {
     return Promise.resolve();
   }
 
+  public markMinedAsPending(txHashes: TxHash[]): Promise<void> {
+    if (txHashes.length === 0) {
+      return Promise.resolve();
+    }
+
+    const keys = txHashes.map(x => x.toBigInt());
+    let deleted = 0;
+    let added = 0;
+    for (const key of keys) {
+      if (this.minedTxs.delete(key)) {
+        deleted++;
+      }
+
+      // only add back to the pending set if we have the tx object
+      if (this.txs.has(key)) {
+        added++;
+        this.pendingTxs.add(key);
+      }
+    }
+
+    this.metrics.recordRemovedObjects(deleted, 'mined');
+    this.metrics.recordAddedObjects(added, 'pending');
+
+    return Promise.resolve();
+  }
+
   public getPendingTxHashes(): TxHash[] {
     return Array.from(this.pendingTxs).map(x => TxHash.fromBigInt(x));
   }

yarn-project/p2p/src/mem_pools/tx_pool/tx_pool.ts

@@ -23,6 +23,13 @@ export interface TxPool {
    */
   markAsMined(txHashes: TxHash[]): Promise<void>;
 
+  /**
+   * Moves mined txs back to the pending set in the case of a reorg.
+   * Note: txs not known by this peer will be ignored.
+   * @param txHashes - Hashes of the txs to flag as pending.
+   */
+  markMinedAsPending(txHashes: TxHash[]): Promise<void>;
+
   /**
    * Deletes transactions from the pool. Tx hashes that are not present are ignored.
    * @param txHashes - An array of tx hashes to be removed from the tx pool.

yarn-project/p2p/src/mem_pools/tx_pool/tx_pool_test_suite.ts

@@ -46,6 +46,35 @@ export function describeTxPool(getTxPool: () => TxPool) {
     expect(pool.getPendingTxHashes()).toEqual([tx2.getTxHash()]);
   });
 
+  it('Marks txs as pending after being mined', async () => {
+    const tx1 = mockTx(1);
+    const tx2 = mockTx(2);
+
+    await pool.addTxs([tx1, tx2]);
+    await pool.markAsMined([tx1.getTxHash()]);
+
+    await pool.markMinedAsPending([tx1.getTxHash()]);
+    expect(pool.getMinedTxHashes()).toEqual([]);
+    const pending = pool.getPendingTxHashes();
+    expect(pending).toHaveLength(2);
+    expect(pending).toEqual(expect.arrayContaining([tx1.getTxHash(), tx2.getTxHash()]));
+  });
+
+  it('Only marks txs as pending if they are known', async () => {
+    const tx1 = mockTx(1);
+    // simulate a situation where not all peers have all the txs
+    const someTxHashThatThisPeerDidNotSee = mockTx(2).getTxHash();
+    await pool.addTxs([tx1]);
+    // this peer knows that tx2 was mined, but it does not have the tx object
+    await pool.markAsMined([tx1.getTxHash(), someTxHashThatThisPeerDidNotSee]);
+    expect(pool.getMinedTxHashes()).toEqual([tx1.getTxHash(), someTxHashThatThisPeerDidNotSee]);
+
+    // reorg: both txs should now become available again
+    await pool.markMinedAsPending([tx1.getTxHash(), someTxHashThatThisPeerDidNotSee]);
+    expect(pool.getMinedTxHashes()).toEqual([]);
+    expect(pool.getPendingTxHashes()).toEqual([tx1.getTxHash()]); // tx2 is not in the pool
+  });
+
   it('Returns all transactions in the pool', async () => {
     const tx1 = mockTx(1);
     const tx2 = mockTx(2);