Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(core-transaction-pool): Switch expiration to chain height #2461

Merged
merged 25 commits into from
Apr 26, 2019
Merged

feat(core-transaction-pool): Switch expiration to chain height #2461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
525f165
feat(core-transaction-pool): Switch expiration to chain height
vasild Apr 23, 2019
e61fa9f
fix(core-transaction-pool): properly get the current chain height
vasild Apr 23, 2019
2b76687
Merge branch 'develop' into transaction-expiration-in-height
faustbrian Apr 24, 2019
f6df72c
Merge branch 'develop' into transaction-expiration-in-height
faustbrian Apr 24, 2019
8186c8a
Update __tests__/unit/core-transaction-pool/connection.test.ts
faustbrian Apr 24, 2019
da79b1c
Update __tests__/unit/core-transaction-pool/connection.test.ts
faustbrian Apr 24, 2019
4675364
Update __tests__/unit/core-transaction-pool/connection.test.ts
faustbrian Apr 24, 2019
086aa30
Update packages/core-transaction-pool/src/memory.ts
faustbrian Apr 24, 2019
6c10fe5
chore(core-transaction-pool): rename MemoryTransaction
vasild Apr 24, 2019
9794070
chore(core-transaction-pool): get current height from "blockchain"
vasild Apr 24, 2019
4d1e25c
chore(core-transaction-pool): remove unused variable
vasild Apr 24, 2019
1e58fe3
Merge branch 'develop' into transaction-expiration-in-height
faustbrian Apr 24, 2019
0167421
fix(core-transaction-pool): get current height in a more robust way
vasild Apr 24, 2019
fb8631b
chore(core-transaction-pool): remove unreachable code
vasild Apr 24, 2019
31c0835
test(core-transaction-pool): disable tx exiration test
vasild Apr 24, 2019
dd58504
Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…
vasild Apr 25, 2019
fd1c4ec
chore(core-transaction-pool): only use app.has if it is available
vasild Apr 25, 2019
de89d7a
fix(core-transaction-pool): handle undefined "expiration"
vasild Apr 25, 2019
68334f7
Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…
vasild Apr 25, 2019
00b0a22
tests(core-transaction-pool): fix a disabled test by proper mocking
vasild Apr 25, 2019
21845ee
Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…
vasild Apr 25, 2019
e5581c8
tests(core-transaction-pool): adjust after a4567cd26
vasild Apr 25, 2019
a2b52a0
chore(core-transaction-pool): simplify, now that we have the last block
vasild Apr 25, 2019
541c2e6
Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…
vasild Apr 26, 2019
156cd28
Merge branch 'develop' into transaction-expiration-in-height
faustbrian Apr 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion __tests__/integration/core-transaction-pool/processor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { generateWallets } from "../../utils/generators/wallets";
import { setUpFull, tearDownFull } from "./__support__/setup";
// import { Crypto, Enums, Managers } from "@arkecosystem/crypto";
// import { Connection } from "../../../packages/core-transaction-pool/src/connection";
// import { MemoryTransaction } from "../../../packages/core-transaction-pool/src/memory-transaction";
// import { SequentialTransaction } from "../../../packages/core-transaction-pool/src/sequential-transaction";
// import { delegates, wallets } from "../../utils/fixtures/unitnet";

let container: Container.IContainer;
Expand Down
45 changes: 28 additions & 17 deletions __tests__/unit/core-transaction-pool/connection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import "jest-extended";

import "./mocks/core-container";
import { container } from "./mocks/core-container";
import { state } from "./mocks/state";

import { Wallets } from "@arkecosystem/core-state";
import { TransactionHandlerRegistry } from "@arkecosystem/core-transactions";
import { Blocks, Constants, Crypto, Enums, Interfaces, Transactions, Utils } from "@arkecosystem/crypto";
import { Blocks, Constants, Enums, Interfaces, Transactions, Utils } from "@arkecosystem/crypto";
import { dato } from "@faustbrian/dato";
import delay from "delay";
import cloneDeep from "lodash.clonedeep";
import randomSeed from "random-seed";
import { Connection } from "../../../packages/core-transaction-pool/src/connection";
import { defaults } from "../../../packages/core-transaction-pool/src/defaults";
import { Memory } from "../../../packages/core-transaction-pool/src/memory";
import { MemoryTransaction } from "../../../packages/core-transaction-pool/src/memory-transaction";
import { SequentialTransaction } from "../../../packages/core-transaction-pool/src/sequential-transaction";
import { Storage } from "../../../packages/core-transaction-pool/src/storage";
import { WalletManager } from "../../../packages/core-transaction-pool/src/wallet-manager";
import { TransactionFactory } from "../../helpers/transaction-factory";
Expand All @@ -22,7 +22,6 @@ import { database as databaseService } from "./mocks/database";

const { BlockFactory } = Blocks;
const { SATOSHI } = Constants;
const { Slots } = Crypto;
const { TransactionTypes } = Enums;

const delegatesSecrets = delegates.map(d => d.secret);
Expand All @@ -49,7 +48,7 @@ beforeEach(() => connection.flush());
describe("Connection", () => {
const addTransactions = transactions => {
for (const tx of transactions) {
memory.remember(new MemoryTransaction(tx), maxTransactionAge);
memory.remember(new SequentialTransaction(tx), maxTransactionAge);
}
};

Expand All @@ -61,11 +60,11 @@ describe("Connection", () => {
it("should return 2 if transactions were added", () => {
expect(connection.getPoolSize()).toBe(0);

memory.remember(new MemoryTransaction(mockData.dummy1), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy1), maxTransactionAge);

expect(connection.getPoolSize()).toBe(1);

memory.remember(new MemoryTransaction(mockData.dummy2), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy2), maxTransactionAge);

expect(connection.getPoolSize()).toBe(2);
});
Expand All @@ -81,11 +80,11 @@ describe("Connection", () => {

expect(connection.getSenderSize(senderPublicKey)).toBe(0);

memory.remember(new MemoryTransaction(mockData.dummy1), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy1), maxTransactionAge);

expect(connection.getSenderSize(senderPublicKey)).toBe(1);

memory.remember(new MemoryTransaction(mockData.dummy3), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy3), maxTransactionAge);

expect(connection.getSenderSize(senderPublicKey)).toBe(2);
});
Expand Down Expand Up @@ -215,10 +214,15 @@ describe("Connection", () => {
});

it("should add the transactions to the pool and they should expire", async () => {
const heightAtStart = 42;

jest.spyOn(container.app, "has").mockReturnValue(true);
jest.spyOn(state, "getLastBlock").mockReturnValue({ data: { height: heightAtStart } });

expect(connection.getPoolSize()).toBe(0);

const expireAfterSeconds = 3;
const expiration = Slots.getTime() + expireAfterSeconds;
const expireAfterBlocks: number = 3;
const expiration: number = heightAtStart + expireAfterBlocks;

const transactions: Interfaces.ITransaction[] = [];

Expand All @@ -231,17 +235,24 @@ describe("Connection", () => {
const insufficientBalanceTx: any = Transactions.TransactionFactory.fromData(
cloneDeep(mockData.dummyExp2.data),
);
transactions.push(insufficientBalanceTx);
insufficientBalanceTx.data.expiration = expiration;
transactions.push(insufficientBalanceTx);

transactions.push(mockData.dummy2);

const { added, notAdded } = connection.addTransactions(transactions);

expect(added).toHaveLength(4);
expect(notAdded).toBeEmpty();

expect(connection.getPoolSize()).toBe(4);
await delay((expireAfterSeconds + 1) * 1000);

jest.spyOn(state, "getLastBlock").mockReturnValue({ data: { height: expiration - 1 } });

expect(connection.getPoolSize()).toBe(4);

jest.spyOn(state, "getLastBlock").mockReturnValue({ data: { height: expiration } });

expect(connection.getPoolSize()).toBe(2);

transactions.forEach(t => connection.removeTransactionById(t.id));
Expand All @@ -250,7 +261,7 @@ describe("Connection", () => {

describe("removeTransaction", () => {
it("should remove the specified transaction from the pool", () => {
memory.remember(new MemoryTransaction(mockData.dummy1), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy1), maxTransactionAge);

expect(connection.getPoolSize()).toBe(1);

Expand All @@ -262,7 +273,7 @@ describe("Connection", () => {

describe("removeTransactionById", () => {
it("should remove the specified transaction from the pool (by id)", () => {
memory.remember(new MemoryTransaction(mockData.dummy1), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy1), maxTransactionAge);

expect(connection.getPoolSize()).toBe(1);

Expand All @@ -272,7 +283,7 @@ describe("Connection", () => {
});

it("should do nothing when asked to delete a non-existent transaction", () => {
memory.remember(new MemoryTransaction(mockData.dummy1), maxTransactionAge);
memory.remember(new SequentialTransaction(mockData.dummy1), maxTransactionAge);

connection.removeTransactionById("nonexistenttransactionid");

Expand Down
28 changes: 0 additions & 28 deletions __tests__/unit/core-transaction-pool/memory-transaction.test.ts
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions __tests__/unit/core-transaction-pool/mocks/core-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ export const container = {
get: () => ({}),
};
},
has: plugin => {
return true;
},
resolve: name => {
return {};
},
Expand Down
1 change: 1 addition & 0 deletions __tests__/unit/core-transaction-pool/mocks/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export const state = {
cacheTransactions: () => null,
getLastBlock: () => ({ data: { height: 0 } }),
removeCachedTransactionIds: () => null,
};
22 changes: 11 additions & 11 deletions __tests__/unit/core-transaction-pool/storage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { MemoryTransaction } from "../../../packages/core-transaction-pool/src/memory-transaction";
import { SequentialTransaction } from "../../../packages/core-transaction-pool/src/sequential-transaction";
import { Storage } from "../../../packages/core-transaction-pool/src/storage";
import { transactions } from "./__fixtures__/transactions";

Expand All @@ -18,13 +18,13 @@ describe("Storage", () => {
describe("bulkAdd", () => {
it("should add the transactions provided", () => {
const memPoolTransactions = [
new MemoryTransaction(transactions.dummy1),
new MemoryTransaction(transactions.dummy2),
new SequentialTransaction(transactions.dummy1),
new SequentialTransaction(transactions.dummy2),
];

storage.bulkAdd(memPoolTransactions);
const allMemoryTransactions = storage.loadAll();
expect(allMemoryTransactions.map(pooltx => pooltx.transaction)).toMatchObject(
const allSequentialTransactions = storage.loadAll();
expect(allSequentialTransactions.map(pooltx => pooltx.transaction)).toMatchObject(
memPoolTransactions.map(pooltx => pooltx.transaction),
);
});
Expand All @@ -33,15 +33,15 @@ describe("Storage", () => {
describe("bulkRemoveById", () => {
it("should remove the transactions corresponding to the ids provided", () => {
const memPoolTransactions = [
new MemoryTransaction(transactions.dummy1),
new MemoryTransaction(transactions.dummy2),
new SequentialTransaction(transactions.dummy1),
new SequentialTransaction(transactions.dummy2),
];
const anotherMemoryTransaction = new MemoryTransaction(transactions.dummy3);
const anotherSequentialTransaction = new SequentialTransaction(transactions.dummy3);

storage.bulkAdd([...memPoolTransactions, anotherMemoryTransaction]);
storage.bulkAdd([...memPoolTransactions, anotherSequentialTransaction]);
storage.bulkRemoveById([transactions.dummy3.id]);
const allMemoryTransactions = storage.loadAll();
expect(allMemoryTransactions.map(pooltx => pooltx.transaction)).toMatchObject(
const allSequentialTransactions = storage.loadAll();
expect(allSequentialTransactions.map(pooltx => pooltx.transaction)).toMatchObject(
memPoolTransactions.map(pooltx => pooltx.transaction),
);
});
Expand Down
4 changes: 2 additions & 2 deletions __tests__/unit/core-transaction-pool/storage.with-mock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { MemoryTransaction } from "../../../packages/core-transaction-pool/src/memory-transaction";
import { SequentialTransaction } from "../../../packages/core-transaction-pool/src/sequential-transaction";
import { Storage } from "../../../packages/core-transaction-pool/src/storage";
import { transactions } from "./__fixtures__/transactions";

Expand All @@ -24,7 +24,7 @@ describe("Storage", () => {
const storage = new Storage();
storage.connect("./tmp");

const memPoolTransaction = new MemoryTransaction(transactions.dummy1);
const memPoolTransaction = new SequentialTransaction(transactions.dummy1);
storage.bulkAdd([memPoolTransaction]);

expect(mockPrepare).toHaveBeenLastCalledWith("ROLLBACK;");
Expand Down
24 changes: 12 additions & 12 deletions packages/core-transaction-pool/src/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { dato, Dato } from "@faustbrian/dato";
import assert from "assert";
import { ITransactionsProcessed } from "./interfaces";
import { Memory } from "./memory";
import { MemoryTransaction } from "./memory-transaction";
import { SequentialTransaction } from "./sequential-transaction";
import { Processor } from "./processor";
import { Storage } from "./storage";
import { WalletManager } from "./wallet-manager";
Expand Down Expand Up @@ -47,7 +47,7 @@ export class Connection implements TransactionPool.IConnection {
this.memory.flush();
this.storage.connect(this.options.storage);

const all: MemoryTransaction[] = this.storage.loadAll();
const all: SequentialTransaction[] = this.storage.loadAll();

for (const transaction of all) {
this.memory.remember(transaction, this.options.maxTransactionAge, true);
Expand All @@ -56,7 +56,7 @@ export class Connection implements TransactionPool.IConnection {
this.purgeExpired();

const forgedIds: string[] = await this.databaseService.getForgedTransactionsIds(
all.map((transaction: MemoryTransaction) => transaction.transaction.id),
all.map((transaction: SequentialTransaction) => transaction.transaction.id),
);

for (const id of forgedIds) {
Expand All @@ -75,7 +75,7 @@ export class Connection implements TransactionPool.IConnection {
return new Processor(this, this.walletManager);
}

public getTransactionsByType(type: number): Set<MemoryTransaction> {
public getTransactionsByType(type: number): Set<SequentialTransaction> {
this.purgeExpired();

return this.memory.getByType(type);
Expand Down Expand Up @@ -152,18 +152,18 @@ export class Connection implements TransactionPool.IConnection {
let transactionBytes: number = 0;

let i = 0;
for (const MemoryTransaction of this.memory.allSortedByFee()) {
for (const SequentialTransaction of this.memory.allSortedByFee()) {
if (i >= start + size) {
break;
}

if (i >= start) {
let pushTransaction: boolean = false;

assert.notStrictEqual(MemoryTransaction.transaction[property], undefined);
assert.notStrictEqual(SequentialTransaction.transaction[property], undefined);

if (maxBytes > 0) {
const transactionSize: number = JSON.stringify(MemoryTransaction.transaction.data).length;
const transactionSize: number = JSON.stringify(SequentialTransaction.transaction.data).length;

if (transactionBytes + transactionSize <= maxBytes) {
transactionBytes += transactionSize;
Expand All @@ -174,7 +174,7 @@ export class Connection implements TransactionPool.IConnection {
}

if (pushTransaction) {
data.push(MemoryTransaction.transaction[property]);
data.push(SequentialTransaction.transaction[property]);
i++;
}
} else {
Expand Down Expand Up @@ -377,8 +377,8 @@ export class Connection implements TransactionPool.IConnection {
public senderHasTransactionsOfType(senderPublicKey: string, transactionType: Enums.TransactionTypes): boolean {
this.purgeExpired();

for (const MemoryTransaction of this.memory.getBySender(senderPublicKey)) {
if (MemoryTransaction.transaction.type === transactionType) {
for (const SequentialTransaction of this.memory.getBySender(senderPublicKey)) {
if (SequentialTransaction.transaction.type === transactionType) {
return true;
}
}
Expand All @@ -401,7 +401,7 @@ export class Connection implements TransactionPool.IConnection {
if (this.options.maxTransactionsInPool <= poolSize) {
// The pool can't accommodate more transactions. Either decline the newcomer or remove
// an existing transaction from the pool in order to free up space.
const all: MemoryTransaction[] = this.memory.allSortedByFee();
const all: SequentialTransaction[] = this.memory.allSortedByFee();
const lowest: Interfaces.ITransaction = all[all.length - 1].transaction;

const fee: Utils.BigNumber = transaction.data.fee;
Expand All @@ -422,7 +422,7 @@ export class Connection implements TransactionPool.IConnection {
}
}

this.memory.remember(new MemoryTransaction(transaction), this.options.maxTransactionAge);
this.memory.remember(new SequentialTransaction(transaction), this.options.maxTransactionAge);

// Apply transaction to pool wallet manager.
const senderWallet: Database.IWallet = this.walletManager.findByPublicKey(transaction.data.senderPublicKey);
Expand Down
5 changes: 4 additions & 1 deletion packages/core-transaction-pool/src/defaults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ export const defaults = {
allowedSenders: [],
maxTransactionsPerRequest: process.env.CORE_TRANSACTION_POOL_MAX_PER_REQUEST || 40,
maxTransactionBytes: process.env.CORE_TRANSACTION_POOL_MAX_TRANSACTIONS_SIZE || 1047876,
maxTransactionAge: 21600,
// Max transaction age in number of blocks produced since receiving a transaction.
// If a transaction stays that long in the pool without being included in any block,
// then it will be removed.
maxTransactionAge: 2700,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to something better so the comment is not needed to understand what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the variable name is fine, any suggestions are welcome though.

It is not possible to condense the information from 3 lines into a variable name. In this case the important information the comment is conveying is:

  • The units of measurement (it is blocks, not seconds)
  • When does the counter start ticking (it is when the transaction has been received, not when it was created like it was before this change)
  • What happens when the time expires.

Please, do not remove the comment.

It is normal to have one or two sentences describing each variable/config parameter, for example:

  -debuglogfile=<file>
       Specify location of debug log file. Relative paths will be prefixed by a
       net-specific datadir location. (-nodebuglogfile to disable;
       default: debug.log)

  -includeconf=<file>
       Specify additional configuration file, relative to the -datadir path
       (only useable from configuration file, not command line)

  -loadblock=<file>
       Imports blocks from external blk000??.dat file on startup

  -maxmempool=<n>
       Keep the transaction memory pool below <n> megabytes (default: 300)

  -maxorphantx=<n>
       Keep at most <n> unconnectable transactions in memory (default: 100)

  -mempoolexpiry=<n>
       Do not keep transactions in the mempool longer than <n> hours (default:
       336)

  -par=<n>
       Set the number of script verification threads (-8 to 16, 0 = auto, <0 =
       leave that many cores free, default: 0)

  -persistmempool
       Whether to save the mempool on shutdown and load on restart (default: 1)

  -prune=<n>
       Reduce storage requirements by enabling pruning (deleting) of old
       blocks. This allows the pruneblockchain RPC to be called to
       delete specific blocks, and enables automatic pruning of old
       blocks if a target size in MiB is provided. This mode is
       incompatible with -txindex and -rescan. Warning: Reverting this
       setting requires re-downloading the entire blockchain. (default:
       0 = disable pruning blocks, 1 = allow manual pruning via RPC,
       >=550 = automatically prune block files to stay under the
       specified target size in MiB)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it not possible to convey that minuscule bit of information in a variable name? All of the following make it pretty clear that it is about lifetime/expiration of the transaction and the unit of measurement is a number of blocks.

  • transactionTtlInBlocks
  • transactionLifetimeInBlocks
  • transactionValidForNumberOfBlocks
  • transactionMaxAgeInBlocks
  • transactionExpiresAfterNumberOfBlocks

dynamicFees: {
enabled: true,
minFeePool: 3000,
Expand Down
Loading