refactor(core-transaction-pool): collator service

In attempt to simplify pool's connection and reduce its responsibilities: * Created separate validator (and factory) to validate transactions against temp wallet repository. * Moved block candidate transaction selection out of pool's connection into separate collator class that is using validator. * Removed candidate transaction selection functions from pool's connection. * Almost removed validation from pool's connection * Get transactions method that is used by controller now only reads transactions from memory instead of creating temporary wallet and performing validation.
ArkEcosystem · Jan 20, 2020 · 319f778 · 319f778
1 parent 1d10acf
commit 319f778
Show file tree

Hide file tree

Showing 15 changed files with 147 additions and 152 deletions.
diff --git a/packages/core-api/src/controllers/transactions.ts b/packages/core-api/src/controllers/transactions.ts
@@ -70,18 +70,12 @@ export class TransactionsController extends Controller {
 
     public async unconfirmed(request: Hapi.Request, h: Hapi.ResponseToolkit) {
         const pagination = super.paginate(request);
-
-        const data = (await this.transactionPool.getTransactions(pagination.offset, pagination.limit)).map(
-            transaction => ({
-                serialized: transaction.toString("hex"),
-            }),
-        );
+        const transactions = await this.transactionPool.getTransactions(pagination.offset, pagination.limit);
+        const poolSize = await this.transactionPool.getPoolSize();
+        const data = transactions.map(t => ({ serialized: t.serialized.toString("hex") }));
 
         return super.toPagination(
-            {
-                count: await this.transactionPool.getPoolSize(),
-                rows: data,
-            },
+            { count: poolSize, rows: data },
             TransactionResource,
             (request.query.transform as unknown) as boolean,
         );

diff --git a/packages/core-forger/src/forger-service.ts b/packages/core-forger/src/forger-service.ts
@@ -251,24 +251,18 @@ export class ForgerService {
      * @memberof ForgerService
      */
     public async getTransactionsForForging(): Promise<Interfaces.ITransactionData[]> {
-        const response: Contracts.P2P.ForgingTransactions = await this.client.getTransactions();
-
+        const response = await this.client.getTransactions();
         if (AppUtils.isEmpty(response)) {
             this.logger.error("Could not get unconfirmed transactions from transaction pool.");
-
             return [];
         }
-
-        const transactions: Interfaces.ITransactionData[] = response.transactions.map(
-            (hex: string) => Transactions.TransactionFactory.fromBytesUnsafe(Buffer.from(hex, "hex")).data,
+        const transactions = response.transactions.map(
+            hex => Transactions.TransactionFactory.fromBytesUnsafe(Buffer.from(hex, "hex")).data,
         );
-
         this.logger.debug(
-            `Received ${AppUtils.pluralize("transaction", transactions.length, true)} from the pool containing ${
-                response.poolSize
-            }`,
+            `Received ${AppUtils.pluralize("transaction", transactions.length, true)} ` +
+                `from the pool containing ${response.poolSize}`,
         );
-
         return transactions;
     }
 

diff --git a/packages/core-kernel/src/contracts/state/index.ts b/packages/core-kernel/src/contracts/state/index.ts
@@ -2,3 +2,4 @@ export * from "./state-store";
 export * from "./wallets";
 export * from "./blocks";
 export * from "./dpos";
+export * from "./transaction-validator";
diff --git a/packages/core-kernel/src/contracts/state/transaction-validator.ts b/packages/core-kernel/src/contracts/state/transaction-validator.ts
@@ -0,0 +1,7 @@
+import { Interfaces } from "@arkecosystem/crypto";
+
+export interface TransactionValidator {
+    validate(transaction: Interfaces.ITransaction): Promise<void>;
+}
+
+export type TransactionValidatorFactory = () => TransactionValidator;
diff --git a/packages/core-kernel/src/contracts/transaction-pool/collator.ts b/packages/core-kernel/src/contracts/transaction-pool/collator.ts
@@ -0,0 +1,5 @@
+import { Interfaces } from "@arkecosystem/crypto";
+
+export interface Collator {
+    getBlockCandidateTransactions(): Promise<Interfaces.ITransaction[]>;
+}
diff --git a/packages/core-kernel/src/contracts/transaction-pool/connection.ts b/packages/core-kernel/src/contracts/transaction-pool/connection.ts
@@ -22,10 +22,8 @@ export interface Connection {
     buildWallets(): Promise<void>;
     replay(transactions: Interfaces.ITransaction[]): Promise<void>;
     getTransaction(id: string): Promise<Interfaces.ITransaction | undefined>;
-    getTransactionIdsForForging(start: number, size: number): Promise<string[]>;
-    getTransactions(start: number, size: number, maxBytes?: number): Promise<Buffer[]>;
+    getTransactions(start: number, size: number): Promise<Interfaces.ITransaction[]>;
     getTransactionsByType(type: number, typeGroup?: number): Promise<Set<Interfaces.ITransaction>>;
-    getTransactionsForForging(blockSize: number): Promise<string[]>;
     has(transactionId: string): Promise<boolean>;
     hasExceededMaxTransactions(senderPublicKey: string): Promise<boolean>;
     senderHasTransactionsOfType(senderPublicKey: string, type: number, typeGroup?: number): Promise<boolean>;

diff --git a/packages/core-kernel/src/contracts/transaction-pool/index.ts b/packages/core-kernel/src/contracts/transaction-pool/index.ts
@@ -1,2 +1,3 @@
 export * from "./connection";
 export * from "./processor";
+export * from "./collator";
diff --git a/packages/core-kernel/src/ioc/identifiers.ts b/packages/core-kernel/src/ioc/identifiers.ts
@@ -62,6 +62,8 @@ export const Identifiers = {
     StateStore: Symbol.for("State<StateStore>"),
     StateTransactionStore: Symbol.for("State<TransactionStore>"),
     WalletFactory: Symbol.for("State<WalletFactory>"),
+    TransactionValidator: Symbol("State<TransactionValidator>"),
+    TransactionValidatorFactory: Symbol("State<TransactionValidatorFactory>"),
 
     // Derived states
     DposState: Symbol.for("State<DposState>"),
@@ -78,6 +80,8 @@ export const Identifiers = {
     TransactionPoolMemory: Symbol.for("TransactionPool<Memory>"),
     TransactionPoolStorage: Symbol.for("TransactionPool<Storage>"),
     TransactionPoolSynchronizer: Symbol.for("TransactionPool<Synchronizer>"),
+    TransactionPoolCollator: Symbol.for("TransactionPool<Collator>"),
+
     // Transactions - @todo: better names that won't clash
     WalletAttributes: Symbol.for("Wallet<Attributes>"),
     // TransactionHandler

diff --git a/packages/core-p2p/src/socket-server/versions/internal.ts b/packages/core-p2p/src/socket-server/versions/internal.ts
@@ -54,17 +54,14 @@ export const getUnconfirmedTransactions = async ({
 }: {
     app: Contracts.Kernel.Application;
 }): Promise<Contracts.P2P.UnconfirmedTransactions> => {
-    const blockchain = app.get<Contracts.Blockchain.Blockchain>(Container.Identifiers.BlockchainService);
-    const { maxTransactions } = Managers.configManager.getMilestone(blockchain.getLastBlock().data.height).block;
-
+    const collator = app.get<Contracts.TransactionPool.Collator>(Container.Identifiers.TransactionPoolCollator);
     const transactionPool: Contracts.TransactionPool.Connection = app.get<Contracts.TransactionPool.Connection>(
         Container.Identifiers.TransactionPoolService,
     );
+    const transactions = await collator.getBlockCandidateTransactions();
+    const poolSize = await transactionPool.getPoolSize();
 
-    return {
-        transactions: await transactionPool.getTransactionsForForging(maxTransactions),
-        poolSize: await transactionPool.getPoolSize(),
-    };
+    return { poolSize, transactions: transactions.map(t => t.serialized.toString("hex")) };
 };
 
 export const getCurrentRound = async ({

diff --git a/packages/core-state/src/service-provider.ts b/packages/core-state/src/service-provider.ts
@@ -7,6 +7,7 @@ import { StateBuilder } from "./state-builder";
 import { BlockStore } from "./stores/blocks";
 import { StateStore } from "./stores/state";
 import { TransactionStore } from "./stores/transactions";
+import { TransactionValidator } from "./transaction-validator";
 import { TempWalletRepository, Wallet, WalletRepository } from "./wallets";
 
 const dposPreviousRoundStateProvider = (context: Container.interfaces.Context) => {
@@ -50,6 +51,12 @@ export class ServiceProvider extends Providers.ServiceProvider {
         this.app
             .bind<Contracts.State.DposPreviousRoundStateProvider>(Container.Identifiers.DposPreviousRoundStateProvider)
             .toProvider(dposPreviousRoundStateProvider);
+
+        this.app.bind(Container.Identifiers.TransactionValidator).to(TransactionValidator);
+
+        this.app
+            .bind(Container.Identifiers.TransactionValidatorFactory)
+            .toAutoFactory(Container.Identifiers.TransactionValidator);
     }
 
     public async boot(): Promise<void> {

diff --git a/packages/core-state/src/transaction-validator.ts b/packages/core-state/src/transaction-validator.ts
@@ -0,0 +1,18 @@
+import { Container, Contracts } from "@arkecosystem/core-kernel";
+import { Handlers } from "@arkecosystem/core-transactions";
+import { Interfaces, Transactions } from "@arkecosystem/crypto";
+import { strictEqual } from "assert";
+
+@Container.injectable()
+export class TransactionValidator implements Contracts.State.TransactionValidator {
+    @Container.inject(Container.Identifiers.TransactionHandlerRegistry)
+    @Container.tagged("state", "temp")
+    private readonly handlerRegistry!: Handlers.Registry;
+
+    public async validate(transaction: Interfaces.ITransaction): Promise<void> {
+        const deserialized: Interfaces.ITransaction = Transactions.TransactionFactory.fromBytes(transaction.serialized);
+        strictEqual(transaction.id, deserialized.id);
+        const handler = await this.handlerRegistry.getActivatedHandlerForData(transaction.data);
+        await handler.apply(transaction);
+    }
+}
diff --git a/packages/core-transaction-pool/src/collator.ts b/packages/core-transaction-pool/src/collator.ts
@@ -0,0 +1,56 @@
+import { Container, Contracts } from "@arkecosystem/core-kernel";
+import { Interfaces, Managers } from "@arkecosystem/crypto";
+
+import { Connection } from "./connection";
+import { Memory } from "./memory";
+
+@Container.injectable()
+export class Collator implements Contracts.TransactionPool.Collator {
+    @Container.inject(Container.Identifiers.TransactionValidatorFactory)
+    private readonly createTransactionValidator!: Contracts.State.TransactionValidatorFactory;
+
+    @Container.inject(Container.Identifiers.BlockchainService)
+    private readonly blockchain!: Contracts.Blockchain.Blockchain;
+
+    @Container.inject(Container.Identifiers.TransactionPoolService)
+    private readonly pool!: Connection;
+
+    @Container.inject(Container.Identifiers.TransactionPoolMemory)
+    private readonly memory!: Memory;
+
+    @Container.inject(Container.Identifiers.LogService)
+    private readonly logger!: Contracts.Kernel.Logger;
+
+    public async getBlockCandidateTransactions(): Promise<Interfaces.ITransaction[]> {
+        let bytesLeft = this.pool.options.maxTransactionBytes ? this.pool.options.maxTransactionBytes : null;
+
+        const height = this.blockchain.getLastBlock().data.height;
+        const milestone = Managers.configManager.getMilestone(height);
+        const transactions: Interfaces.ITransaction[] = [];
+        const validator = this.createTransactionValidator();
+
+        for (const transaction of this.memory.allSortedByFee().slice()) {
+            if (transactions.length === milestone.block.maxTransactions) {
+                break;
+            }
+
+            try {
+                await validator.validate(transaction);
+                if (bytesLeft !== null) {
+                    bytesLeft -= JSON.stringify(transaction.data).length;
+                    if (bytesLeft < 0) {
+                        break;
+                    }
+                }
+                transactions.push(transaction);
+            } catch (error) {
+                this.pool.removeTransactionById(transaction.id!);
+                this.logger.error(
+                    `[Pool] Removed ${transaction.id} before forging because it is no longer valid: ${error.message}`,
+                );
+            }
+        }
+
+        return transactions;
+    }
+}