From f9774de541b1417117c22da85227b85ddb0045a1 Mon Sep 17 00:00:00 2001 From: Brian Faust Date: Mon, 11 Feb 2019 07:15:56 +0200 Subject: [PATCH] fix(core-transaction-pool): disallow multiple registrations for same delegate --- .../__tests__/guard.test.ts | 54 +++++++++++++++++++ .../core-transaction-pool/src/connection.ts | 11 ++++ packages/core-transaction-pool/src/guard.ts | 31 +++++++++++ packages/core-transaction-pool/src/mem.ts | 37 +++++++++++++ 4 files changed, 133 insertions(+) diff --git a/packages/core-transaction-pool/__tests__/guard.test.ts b/packages/core-transaction-pool/__tests__/guard.test.ts index cf1398949f..308db00d7a 100644 --- a/packages/core-transaction-pool/__tests__/guard.test.ts +++ b/packages/core-transaction-pool/__tests__/guard.test.ts @@ -394,6 +394,33 @@ describe("Transaction Guard", () => { expect(result.errors).toBeNull(); }); + it("should not validate when multiple wallets register the same username in the same transaction payload", async () => { + const delegateRegistrations = [ + generateDelegateRegistration("unitnet", wallets[14].passphrase, 1, false, "test_delegate")[0], + generateDelegateRegistration("unitnet", wallets[15].passphrase, 1, false, "test_delegate")[0], + ]; + + const result = await guard.validate(delegateRegistrations); + expect(result.invalid).toEqual(delegateRegistrations.map(transaction => transaction.id)); + + delegateRegistrations.forEach(tx => { + expect(guard.errors[tx.id]).toEqual([ + { + type: "ERR_CONFLICT", + message: `Multiple delegate registrations for "${ + tx.asset.delegate.username + }" in transaction payload`, + }, + ]); + }); + + const wallet1 = transactionPool.walletManager.findByPublicKey(wallets[14].keys.publicKey); + const wallet2 = transactionPool.walletManager.findByPublicKey(wallets[15].keys.publicKey); + + expect(wallet1.username).toBe(null); + expect(wallet2.username).toBe(null); + }); + describe("Sign a transaction then change some fields shouldn't pass validation", () => { const secondSignatureError = (id, address) => [ id, @@ -891,6 +918,33 @@ describe("Transaction Guard", () => { }); }); + it("should not validate a delegate registration if an existing registration for the same username from a different wallet exists in the pool", async () => { + const delegateRegistrations = [ + generateDelegateRegistration("unitnet", wallets[16].passphrase, 1, false, "test_delegate")[0], + generateDelegateRegistration("unitnet", wallets[17].passphrase, 1, false, "test_delegate")[0], + ]; + + expect(guard.__validateTransaction(delegateRegistrations[0])).toBeTrue(); + guard.accept.set(delegateRegistrations[0].id, delegateRegistrations[0]); + guard.__addTransactionsToPool(); + expect(guard.errors).toEqual({}); + expect(guard.__validateTransaction(delegateRegistrations[1])).toBeFalse(); + expect(guard.errors[delegateRegistrations[1].id]).toEqual([ + { + type: "ERR_PENDING", + message: `Delegate registration for "${ + delegateRegistrations[1].asset.delegate.username + }" already in the pool`, + }, + ]); + + const wallet1 = transactionPool.walletManager.findByPublicKey(wallets[16].keys.publicKey); + const wallet2 = transactionPool.walletManager.findByPublicKey(wallets[17].keys.publicKey); + + expect(wallet1.username).toBe("test_delegate"); + expect(wallet2.username).toBe(null); + }); + it("should not validate when sender has same type transactions in the pool (only for 2nd sig, delegate registration, vote)", async () => { jest.spyOn(guard.pool.walletManager, "canApply").mockImplementation(() => true); const votes = [ diff --git a/packages/core-transaction-pool/src/connection.ts b/packages/core-transaction-pool/src/connection.ts index 8713b69b52..70f27ec364 100644 --- a/packages/core-transaction-pool/src/connection.ts +++ b/packages/core-transaction-pool/src/connection.ts @@ -77,6 +77,17 @@ export class TransactionPool implements transactionPool.ITransactionPool { this.storage.close(); } + /** + * Get all transactions of a given type from the pool. + * @param {Number} type of transaction + * @return {Set of MemPoolTransaction} all transactions of the given type, could be empty Set + */ + public getTransactionsByType(type) { + this.__purgeExpired(); + + return this.mem.getByType(type); + } + /** * Get the number of transactions in the pool. * @return {Number} diff --git a/packages/core-transaction-pool/src/guard.ts b/packages/core-transaction-pool/src/guard.ts index 9c6cb76c62..9aa58c0c00 100644 --- a/packages/core-transaction-pool/src/guard.ts +++ b/packages/core-transaction-pool/src/guard.ts @@ -4,6 +4,7 @@ import { configManager, constants, models, slots } from "@arkecosystem/crypto"; import pluralize from "pluralize"; import { TransactionPool } from "./connection"; import { dynamicFeeMatcher } from "./dynamic-fee"; +import { MemPoolTransaction } from "./mem-pool-transaction"; import { isRecipientOnActiveNetwork } from "./utils/is-on-active-network"; const { TransactionTypes } = constants; @@ -174,6 +175,36 @@ export class TransactionGuard implements transactionPool.ITransactionGuard { } const errors = []; + + // This check must come before canApply otherwise a wallet may be incorrectly assigned a username when multiple + // conflicting delegate registrations for the same username exist in the same transaction payload + if (transaction.type === TransactionTypes.DelegateRegistration) { + const username = transaction.asset.delegate.username; + const delegateRegistrationsInPayload = this.transactions.filter( + tx => tx.type === TransactionTypes.DelegateRegistration && tx.asset.delegate.username === username, + ); + if (delegateRegistrationsInPayload.length > 1) { + this.__pushError( + transaction, + "ERR_CONFLICT", + `Multiple delegate registrations for "${username}" in transaction payload`, + ); + return false; + } + + const delegateRegistrationsInPool: MemPoolTransaction[] = Array.from( + this.pool.getTransactionsByType(TransactionTypes.DelegateRegistration), + ); + if (delegateRegistrationsInPool.some(memTx => memTx.transaction.asset.delegate.username === username)) { + this.__pushError( + transaction, + "ERR_PENDING", + `Delegate registration for "${username}" already in the pool`, + ); + return false; + } + } + if (!this.pool.walletManager.canApply(transaction, errors)) { this.__pushError(transaction, "ERR_APPLY", JSON.stringify(errors)); return false; diff --git a/packages/core-transaction-pool/src/mem.ts b/packages/core-transaction-pool/src/mem.ts index 4f8089f830..2c5aaa090c 100644 --- a/packages/core-transaction-pool/src/mem.ts +++ b/packages/core-transaction-pool/src/mem.ts @@ -8,6 +8,7 @@ export class Mem { public allIsSorted: boolean; public byId: { [key: string]: MemPoolTransaction }; public bySender: { [key: string]: Set }; + public byType: { [key: number]: Set }; public byExpiration: MemPoolTransaction[]; public byExpirationIsSorted: boolean; public dirty: { added: Set; removed: Set }; @@ -58,6 +59,13 @@ export class Mem { */ this.bySender = {}; + /** + * A map of (key=transaction type, value=Set of MemPoolTransaction). + * Used to: + * - get all transactions of a given type + */ + this.byType = {}; + /** * An array of MemPoolTransaction, sorted by expiration (earliest date * comes first). This array may not contain all transactions that are @@ -111,6 +119,8 @@ export class Mem { this.byId[transaction.id] = memPoolTransaction; const sender = transaction.senderPublicKey; + const type = transaction.type; + if (this.bySender[sender] === undefined) { // First transaction from this sender, create a new Set. this.bySender[sender] = new Set([memPoolTransaction]); @@ -119,6 +129,14 @@ export class Mem { this.bySender[sender].add(memPoolTransaction); } + if (this.byType[type] === undefined) { + // First transaction of this type, create a new Set. + this.byType[type] = new Set([memPoolTransaction]); + } else { + // Append to existing transaction ids for this type. + this.byType[type].add(memPoolTransaction); + } + if (memPoolTransaction.expireAt(maxTransactionAge) !== null) { this.byExpiration.push(memPoolTransaction); this.byExpirationIsSorted = false; @@ -152,6 +170,7 @@ export class Mem { } const memPoolTransaction = this.byId[id]; + const type = this.byId[id].transaction.type; // XXX worst case: O(n) let i = this.byExpiration.findIndex(e => e.transaction.id === id); @@ -164,6 +183,11 @@ export class Mem { delete this.bySender[senderPublicKey]; } + this.byType[type].delete(memPoolTransaction); + if (this.byType[type].size === 0) { + delete this.byType[type]; + } + delete this.byId[id]; i = this.all.findIndex(e => e.transaction.id === id); @@ -202,6 +226,19 @@ export class Mem { return new Set(); } + /** + * Get all transactions of a given type. + * @param {Number} type of transaction + * @return {Set of MemPoolTransaction} all transactions of the given type, could be empty Set + */ + public getByType(type) { + const memPoolTransactions = this.byType[type]; + if (memPoolTransactions !== undefined) { + return memPoolTransactions; + } + return new Set(); + } + /** * Get a transaction, given its id. * @param {String} id transaction id