From fe99e2855a2ce7a0759692588bbf552e931e5cf8 Mon Sep 17 00:00:00 2001 From: Alex Gherghisan Date: Mon, 28 Oct 2024 13:51:31 +0000 Subject: [PATCH] feat: add mined txs back to pending set --- .../p2p/src/client/p2p_client.test.ts | 26 +++++++++++++++++ yarn-project/p2p/src/client/p2p_client.ts | 12 ++++---- .../src/mem_pools/tx_pool/aztec_kv_tx_pool.ts | 26 +++++++++++++++++ .../src/mem_pools/tx_pool/memory_tx_pool.ts | 26 +++++++++++++++++ .../p2p/src/mem_pools/tx_pool/tx_pool.ts | 7 +++++ .../mem_pools/tx_pool/tx_pool_test_suite.ts | 29 +++++++++++++++++++ 6 files changed, 120 insertions(+), 6 deletions(-) diff --git a/yarn-project/p2p/src/client/p2p_client.test.ts b/yarn-project/p2p/src/client/p2p_client.test.ts index 9591815b1ad..3db5197008e 100644 --- a/yarn-project/p2p/src/client/p2p_client.test.ts +++ b/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 diff --git a/yarn-project/p2p/src/client/p2p_client.ts b/yarn-project/p2p/src/client/p2p_client.ts index 53005a90750..ef37e03cfed 100644 --- a/yarn-project/p2p/src/client/p2p_client.ts +++ b/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 && diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts b/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts index 432eefd012a..4600aa9ee42 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/aztec_kv_tx_pool.ts +++ b/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 { + 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)); } diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts b/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts index 9e6d72ea5a4..80efff265d6 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/memory_tx_pool.ts +++ b/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 { + 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)); } diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool.ts b/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool.ts index 4cf434bb3e0..3ce6ed670e2 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool.ts +++ b/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool.ts @@ -23,6 +23,13 @@ export interface TxPool { */ markAsMined(txHashes: TxHash[]): Promise; + /** + * 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; + /** * 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. diff --git a/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool_test_suite.ts b/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool_test_suite.ts index 2c72c1afa80..1911fb0ec1a 100644 --- a/yarn-project/p2p/src/mem_pools/tx_pool/tx_pool_test_suite.ts +++ b/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);