fix(transaction-pool-service): take nonce into account when filtering…

… by highest priority (#807) * fix tx pool priority * remove obsolete tx pool api routes * update comments * style: resolve style guide violations * remove unused dep * Reorder --------- Co-authored-by: oXtxNt9U <[email protected]> Co-authored-by: sebastijankuzner <[email protected]>
ArkEcosystem · Jan 8, 2025 · 48df951 · 48df951
1 parent 5599765
commit 48df951
Show file tree

Hide file tree

Showing 7 changed files with 72 additions and 120 deletions.
diff --git a/packages/api-transaction-pool/package.json b/packages/api-transaction-pool/package.json
@@ -28,7 +28,6 @@
 		"@mainsail/container": "workspace:*",
 		"@mainsail/contracts": "workspace:*",
 		"@mainsail/kernel": "workspace:*",
-		"@mainsail/transactions": "workspace:*",
 		"joi": "17.12.2"
 	},
 	"devDependencies": {

diff --git a/packages/api-transaction-pool/source/controllers/transactions.ts b/packages/api-transaction-pool/source/controllers/transactions.ts
@@ -3,8 +3,6 @@ import Hapi from "@hapi/hapi";
 import { AbstractController } from "@mainsail/api-common";
 import { inject, injectable } from "@mainsail/container";
 import { Contracts, Identifiers } from "@mainsail/contracts";
-import { Utils } from "@mainsail/kernel";
-import { Handlers } from "@mainsail/transactions";
 
 import { TransactionResource } from "../resources/index.js";
 
@@ -16,9 +14,6 @@ export class TransactionsController extends AbstractController {
 	@inject(Identifiers.TransactionPool.Query)
 	private readonly poolQuery!: Contracts.TransactionPool.Query;
 
-	@inject(Identifiers.Transaction.Handler.Registry)
-	private readonly nullHandlerRegistry!: Handlers.Registry;
-
 	public async store(request: Hapi.Request) {
 		const result = await this.processor.process(
 			// @ts-ignore
@@ -64,52 +59,4 @@ export class TransactionsController extends AbstractController {
 
 		return super.respondWithResource(transaction.data, TransactionResource, !!request.query.transform);
 	}
-
-	public async types(request: Hapi.Request) {
-		const activatedTransactionHandlers = await this.nullHandlerRegistry.getActivatedHandlers();
-		const typeGroups: Record<string | number, Record<string, number>> = {};
-
-		for (const handler of activatedTransactionHandlers) {
-			const constructor = handler.getConstructor();
-
-			const type: number | undefined = constructor.type;
-			const typeGroup: number | undefined = constructor.typeGroup;
-			const key: string | undefined = constructor.key;
-
-			Utils.assert.defined<number>(type);
-			Utils.assert.defined<number>(typeGroup);
-			Utils.assert.defined<string>(key);
-
-			if (typeGroups[typeGroup] === undefined) {
-				typeGroups[typeGroup] = {};
-			}
-
-			typeGroups[typeGroup][key[0].toUpperCase() + key.slice(1)] = type;
-		}
-
-		return { data: typeGroups };
-	}
-
-	public async schemas(request: Hapi.Request) {
-		const activatedTransactionHandlers = await this.nullHandlerRegistry.getActivatedHandlers();
-		const schemasByType: Record<string, Record<string, any>> = {};
-
-		for (const handler of activatedTransactionHandlers) {
-			const constructor = handler.getConstructor();
-
-			const type: number | undefined = constructor.type;
-			const typeGroup: number | undefined = constructor.typeGroup;
-
-			Utils.assert.defined<number>(type);
-			Utils.assert.defined<number>(typeGroup);
-
-			if (schemasByType[typeGroup] === undefined) {
-				schemasByType[typeGroup] = {};
-			}
-
-			schemasByType[typeGroup][type] = constructor.getSchema().properties;
-		}
-
-		return { data: schemasByType };
-	}
 }
diff --git a/packages/api-transaction-pool/source/resources/transaction.ts b/packages/api-transaction-pool/source/resources/transaction.ts
@@ -8,28 +8,6 @@ export class TransactionResource implements Contracts.Api.Resource {
 	}
 
 	public async transform(resource: Contracts.Crypto.TransactionData): Promise<object> {
-		//AppUtils.assert.defined<string>(resource.senderPublicKey);
-
-		// const wallet = await this.stateService.getStore().walletRepository.findByPublicKey(resource.senderPublicKey);
-		// const sender: string = wallet.getAddress();
-
-		return {
-			// amount: resource.amount.toFixed(),
-			// asset: resource.asset,
-			// blockId: resource.blockId,
-			// fee: resource.fee.toFixed(),
-			// id: resource.id,
-			// nonce: resource.nonce?.toFixed(),
-			// recipient: resource.recipientId,
-			// // sender,
-			// // senderPublicKey: resource.senderPublicKey,
-			// signature: resource.signature,
-			// signatures: resource.signatures,
-			// timestamp: resource.timestamp,
-			// type: resource.type,
-			// typeGroup: resource.typeGroup,
-			// vendorField: resource.vendorField,
-			// version: resource.version,
-		};
+		return this.raw(resource);
 	}
 }
diff --git a/packages/api-transaction-pool/source/routes/transactions.ts b/packages/api-transaction-pool/source/routes/transactions.ts
@@ -80,16 +80,4 @@ export const register = (server: Contracts.Api.ApiServer): void => {
 		},
 		path: "/transactions/unconfirmed/{id}",
 	});
-
-	server.route({
-		handler: (request: Hapi.Request) => controller.types(request),
-		method: "GET",
-		path: "/transactions/types",
-	});
-
-	server.route({
-		handler: (request: Hapi.Request) => controller.schemas(request),
-		method: "GET",
-		path: "/transactions/schemas",
-	});
 };
diff --git a/packages/transaction-pool-service/source/query.test.ts b/packages/transaction-pool-service/source/query.test.ts
@@ -30,7 +30,7 @@ describe<{
 				amount: BigNumber.make(100),
 				gasPrice: 100,
 				nonce: BigNumber.make(1),
-				senderPublicKey: "sender-public-key",
+				senderAddress: "sender1",
 				type: 1,
 				version: 2,
 			},
@@ -46,7 +46,7 @@ describe<{
 				amount: BigNumber.make(100),
 				gasPrice: 200,
 				nonce: BigNumber.make(2),
-				senderPublicKey: "sender-public-key",
+				senderAddress: "sender1",
 				type: 1,
 				version: 2,
 			},
@@ -62,7 +62,7 @@ describe<{
 				amount: BigNumber.make(100),
 				gasPrice: 300,
 				nonce: BigNumber.make(3),
-				senderPublicKey: "sender-public-key",
+				senderAddress: "sender2",
 				type: 1,
 				version: 2,
 			},
@@ -78,7 +78,7 @@ describe<{
 				amount: BigNumber.make(100),
 				gasPrice: 400,
 				nonce: BigNumber.make(4),
-				senderPublicKey: "sender-public-key",
+				senderAddress: "sender2",
 				type: 1,
 				version: 2,
 			},
@@ -121,37 +121,37 @@ describe<{
 		getSenderStub.calledWith("sender public key");
 	});
 
-	it("getFromLowestPriority - should return transactions reverse ordered by fee", async (context) => {
+	it("getFromLowestPriority - should return transactions reverse ordered by nonce/fee", async (context) => {
 		stub(context.mempool, "getSenderMempools").returnValueOnce([
 			{ getFromLatest: () => [context.sender1Transaction200, context.sender1Transaction100] },
-			{ getFromLatest: () => [context.sender2Transaction100, context.sender2Transaction200] },
+			{ getFromLatest: () => [context.sender2Transaction200, context.sender2Transaction100] },
 		]);
 
 		const query = context.container.resolve(Query);
 		const result = await query.getFromLowestPriority().all();
 
 		assert.equal(result, [
-			context.sender1Transaction100,
 			context.sender1Transaction200,
-			context.sender2Transaction100,
+			context.sender1Transaction100,
 			context.sender2Transaction200,
+			context.sender2Transaction100,
 		]);
 	});
 
-	it("getFromHighestPriority - should return transactions order by fee", async (context) => {
+	it("getFromHighestPriority - should return transactions order by nonce/fee", async (context) => {
 		stub(context.mempool, "getSenderMempools").returnValueOnce([
-			{ getFromEarliest: () => [context.sender1Transaction200, context.sender1Transaction100] },
+			{ getFromEarliest: () => [context.sender1Transaction100, context.sender1Transaction200] },
 			{ getFromEarliest: () => [context.sender2Transaction100, context.sender2Transaction200] },
 		]);
 
 		const query = context.container.resolve(Query);
 		const result = await query.getFromHighestPriority().all();
 
 		assert.equal(result, [
-			context.sender2Transaction200,
 			context.sender2Transaction100,
-			context.sender1Transaction200,
+			context.sender2Transaction200,
 			context.sender1Transaction100,
+			context.sender1Transaction200,
 		]);
 	});
 

diff --git a/packages/transaction-pool-service/source/query.ts b/packages/transaction-pool-service/source/query.ts
@@ -1,5 +1,17 @@
 import { inject, injectable } from "@mainsail/container";
 import { Contracts, Identifiers } from "@mainsail/contracts";
+import { Utils } from "@mainsail/kernel";
+
+type SortFunction = (a: Contracts.Crypto.Transaction, b: Contracts.Crypto.Transaction) => number;
+type SenderMempoolSelectorFunction = (
+	mempool: Contracts.TransactionPool.SenderMempool,
+) => Contracts.Crypto.Transaction[];
+
+const sortByHighestGasPrice = (a: Contracts.Crypto.Transaction, b: Contracts.Crypto.Transaction) =>
+	b.data.gasPrice - a.data.gasPrice;
+
+const sortByLowestGasPrice = (a: Contracts.Crypto.Transaction, b: Contracts.Crypto.Transaction) =>
+	a.data.gasPrice - b.data.gasPrice;
 
 export class QueryIterable implements Contracts.TransactionPool.QueryIterable {
 	public transactions: Contracts.Crypto.Transaction[];
@@ -85,24 +97,55 @@ export class Query implements Contracts.TransactionPool.Query {
 	}
 
 	public getFromLowestPriority(): QueryIterable {
-		return new QueryIterable(
-			[...this.mempool.getSenderMempools()]
-				.flatMap((senderMempool) => [...senderMempool.getFromLatest()])
-				.sort(
-					(a: Contracts.Crypto.Transaction, b: Contracts.Crypto.Transaction) =>
-						a.data.gasPrice - b.data.gasPrice,
-				),
-		);
+		return this.#getTransactions((senderMempool) => [...senderMempool.getFromLatest()], sortByLowestGasPrice);
 	}
 
 	public getFromHighestPriority(): QueryIterable {
-		return new QueryIterable(
-			[...this.mempool.getSenderMempools()]
-				.flatMap((senderMempool) => [...senderMempool.getFromEarliest()])
-				.sort(
-					(a: Contracts.Crypto.Transaction, b: Contracts.Crypto.Transaction) =>
-						b.data.gasPrice - a.data.gasPrice,
-				),
-		);
+		return this.#getTransactions((senderMempool) => [...senderMempool.getFromEarliest()], sortByHighestGasPrice);
+	}
+
+	#getTransactions(selector: SenderMempoolSelectorFunction, prioritySort: SortFunction): QueryIterable {
+		// Group transactions by sender mempool
+		const transactionsBySenderMempool: Record<string, Contracts.Crypto.Transaction[]> = {};
+		for (const senderMempool of this.mempool.getSenderMempools()) {
+			const transactions = selector(senderMempool);
+			if (transactions.length === 0) {
+				continue;
+			}
+
+			transactionsBySenderMempool[transactions[0].data.senderAddress] = transactions;
+		}
+
+		// Add first transaction of each sender mempool
+		const priorityQueue: Contracts.Crypto.Transaction[] = [];
+		for (const transactions of Object.values(transactionsBySenderMempool)) {
+			priorityQueue.push(transactions[0]);
+		}
+
+		// Sort by priority (nonces are per sender and already handled by the provided selector)
+		priorityQueue.sort(prioritySort);
+
+		// Lastly, select transactions from priority queue until all sender mempools are empty.
+		const selectedTransactions: Contracts.Crypto.Transaction[] = [];
+		while (priorityQueue.length > 0) {
+			// Pick next priority transaction
+			const transaction = priorityQueue.shift();
+			Utils.assert.defined(transaction);
+			selectedTransactions.push(transaction);
+
+			// Remove the selected transaction from sender mempool
+			const senderMempool = transactionsBySenderMempool[transaction.data.senderAddress];
+			senderMempool.shift();
+
+			// If the sender has more transactions, add the next one to the queue
+			if (senderMempool.length > 0) {
+				priorityQueue.push(senderMempool[0]);
+			}
+
+			// Re-sort the priority queue
+			priorityQueue.sort(prioritySort);
+		}
+
+		return new QueryIterable(selectedTransactions);
 	}
 }
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml