Skip to content

Commit

Permalink
refactor(core-forger): memory leak, log spam, cleanup (#2341)
Browse files Browse the repository at this point in the history
  • Loading branch information
@spkjp @faustbrian
spkjp authored and faustbrian committed Mar 31, 2019
1 parent ee0398b commit 327b98c
Show file tree
Hide file tree
Showing 21 changed files with 272 additions and 305 deletions.
30 changes: 15 additions & 15 deletions __tests__/integration/core-forger/client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { httpie } from "@arkecosystem/core-utils";
import "jest-extended";
import nock from "nock";
import { Client } from "../../../packages/core-forger/src/client";
import { HostNoResponseError } from "../../../packages/core-forger/src/errors";
import { sampleBlocks } from "./__fixtures__/blocks";

jest.setTimeout(30000);
Expand Down Expand Up @@ -52,7 +53,7 @@ describe("Client", () => {
return requestBody;
});

await client.__chooseHost();
await client.selectHost();

const wasBroadcasted = await client.broadcast(sampleBlock.toJson());
expect(wasBroadcasted).toBeTruthy();
Expand Down Expand Up @@ -84,7 +85,7 @@ describe("Client", () => {
.get("/internal/transactions/forging")
.reply(200, { data: expectedResponse });

await client.__chooseHost();
await client.selectHost();
const response = await client.getTransactions();

expect(response).toEqual(expectedResponse);
Expand All @@ -100,7 +101,7 @@ describe("Client", () => {
.get("/internal/network/state")
.reply(200, { data: expectedResponse });

await client.__chooseHost();
await client.selectHost();
const response = await client.getNetworkState();

expect(response).toEqual(expectedResponse);
Expand All @@ -115,23 +116,22 @@ describe("Client", () => {
.get("/internal/blockchain/sync")
.reply(200);

await client.selectHost();
await client.syncCheck();

expect(httpie.get).toHaveBeenCalledWith(`${host}/internal/blockchain/sync`, expect.any(Object));
});
});

describe("getUsernames", () => {
it("should fetch usernames", async () => {
jest.spyOn(httpie, "get");
const expectedResponse = { foo: "bar" };
nock(host)
.get("/internal/utils/usernames")
.reply(200, { data: expectedResponse });

const response = await client.getUsernames();
describe("selectHost", () => {
it("should fallback to responsive host", async () => {
client = new Client(["http://127.0.0.2:4000", "http://127.0.0.3:4000", host]);
await expect(client.selectHost()).toResolve();
});

expect(response).toEqual(expectedResponse);
it("should throw error when no host is responsive", async () => {
client = new Client(["http://127.0.0.2:4000", "http://127.0.0.3:4000"]);
await expect(client.selectHost()).rejects.toThrowError(HostNoResponseError);
});
});

Expand All @@ -145,15 +145,15 @@ describe("Client", () => {
return [200];
});

await client.__chooseHost();
await client.selectHost();
await client.emitEvent("foo", "bar");

expect(httpie.post).toHaveBeenCalledWith(`${host}/internal/utils/events`, {
body: JSON.stringify({ event: "foo", body: "bar" }),
headers: {
"Content-Type": "application/json",
nethash: {},
port: "4000",
port: 4000,
version: "2.3.0",
"x-auth": "forger",
},
Expand Down
48 changes: 13 additions & 35 deletions __tests__/unit/core-forger/manager.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,15 @@ describe("Forger Manager", () => {
it("should be ok with configured delegates", async () => {
const secret = "a secret";
forgeManager.secrets = [secret];
// @ts-ignore
forgeManager.client.getUsernames.mockReturnValue([]);

const delegates = await forgeManager.loadDelegates();

expect(delegates).toBeArray();
delegates.forEach(value => expect(value).toBeInstanceOf(Delegate));
expect(forgeManager.client.getUsernames).toHaveBeenCalled();
});
});

describe("__forgeNewBlock", () => {
describe("forgeNewBlock", () => {
it("should forge a block", async () => {
// NOTE: make sure we have valid transactions from an existing wallet
const transactions = generateTransfer(
Expand All @@ -68,7 +65,7 @@ describe("Forger Manager", () => {
reward: 2 * 1e8,
};

await forgeManager.__forgeNewBlock(del, round, {
await forgeManager.forgeNewBlock(del, round, {
lastBlockId: round.lastBlock.id,
nodeHeight: round.lastBlock.height,
});
Expand All @@ -86,7 +83,7 @@ describe("Forger Manager", () => {

describe("__monitor", () => {
it("should emit failed event if error while monitoring", async () => {
forgeManager.client.getUsernames.mockRejectedValue(new Error("oh bollocks"));
forgeManager.client.getRound.mockRejectedValue(new Error("oh bollocks"));

setTimeout(() => forgeManager.stop(), 1000);
await forgeManager.__monitor();
Expand All @@ -95,12 +92,12 @@ describe("Forger Manager", () => {
});
});

describe("__getTransactionsForForging", () => {
describe("getTransactionsForForging", () => {
it("should return zero transactions if none to forge", async () => {
// @ts-ignore
forgeManager.client.getTransactions.mockReturnValue({});

const transactions = await forgeManager.__getTransactionsForForging();
const transactions = await forgeManager.getTransactionsForForging();

expect(transactions).toHaveLength(0);
expect(forgeManager.client.getTransactions).toHaveBeenCalled();
Expand All @@ -111,7 +108,7 @@ describe("Forger Manager", () => {
transactions: [Transaction.fromData(sampleTransaction).serialized.toString("hex")],
});

const transactions = await forgeManager.__getTransactionsForForging();
const transactions = await forgeManager.getTransactionsForForging();

expect(transactions).toHaveLength(1);
expect(forgeManager.client.getTransactions).toHaveBeenCalled();
Expand All @@ -120,31 +117,12 @@ describe("Forger Manager", () => {
});
});

describe("__isDelegateActivated", () => {
it("should be ok", async () => {
forgeManager.delegates = [
{
username: "arkxdev",
publicKey: "0310ad026647eed112d1a46145eed58b8c19c67c505a67f1199361a511ce7860c0",
},
];

const forger = await forgeManager.__isDelegateActivated(
"0310ad026647eed112d1a46145eed58b8c19c67c505a67f1199361a511ce7860c0",
);

expect(forger).toBeObject();
expect(forger.username).toBe("arkxdev");
expect(forger.publicKey).toBe("0310ad026647eed112d1a46145eed58b8c19c67c505a67f1199361a511ce7860c0");
});
});

describe("__parseNetworkState", () => {
describe("parseNetworkState", () => {
it("should be TRUE when quorum > 0.66", async () => {
const networkState = new NetworkState(NetworkStateStatus.Default);
Object.assign(networkState, { getQuorum: () => 0.9, nodeHeight: 100, lastBlockId: "1233443" });

const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);

expect(canForge).toBeTrue();
});
Expand All @@ -153,7 +131,7 @@ describe("Forger Manager", () => {
const networkState = new NetworkState(NetworkStateStatus.Unknown);
Object.assign(networkState, { getQuorum: () => 1, nodeHeight: 100, lastBlockId: "1233443" });

const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);

expect(canForge).toBeFalse();
});
Expand All @@ -162,21 +140,21 @@ describe("Forger Manager", () => {
const networkState = new NetworkState(NetworkStateStatus.Default);
Object.assign(networkState, { getQuorum: () => 0.65, nodeHeight: 100, lastBlockId: "1233443" });

const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);

expect(canForge).toBeFalse();
});

it("should be FALSE when coldStart is active", async () => {
const networkState = new NetworkState(NetworkStateStatus.ColdStart);
const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);

expect(canForge).toBeFalse();
});

it("should be FALSE when minimumNetworkReach is not sufficient", async () => {
const networkState = new NetworkState(NetworkStateStatus.BelowMinimumPeers);
const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);

expect(canForge).toBeFalse();
});
Expand All @@ -200,7 +178,7 @@ describe("Forger Manager", () => {
},
});

const canForge = await forgeManager.__parseNetworkState(networkState, delegate);
const canForge = await forgeManager.parseNetworkState(networkState, delegate);
expect(canForge).toBeFalse();
});
});
Expand Down
7 changes: 3 additions & 4 deletions packages/core-api/src/versions/1/delegates/controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,14 @@ export class DelegatesController extends Controller {
const delegatesCount = this.config.getMilestone(lastBlock).activeDelegates;
const currentSlot = slots.getSlotNumber(lastBlock.data.timestamp);

let activeDelegates = await this.databaseService.getActiveDelegates(lastBlock.data.height);
activeDelegates = activeDelegates.map(delegate => delegate.publicKey);

const activeDelegates = await this.databaseService.getActiveDelegates(lastBlock.data.height);
const nextForgers = [];

for (let i = 1; i <= delegatesCount && i <= limit; i++) {
const delegate = activeDelegates[(currentSlot + i) % delegatesCount];

if (delegate) {
nextForgers.push(delegate);
nextForgers.push(delegate.publicKey);
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/core-database-postgres/src/repositories/rounds.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export class RoundsRepository extends Repository implements Database.IRoundsRepo
* @param {Number} round
* @return {Promise}
*/
public async findById(round) {
public async findById(round: number): Promise<Database.IRound[]> {
return this.db.manyOrNone(sql.find, { round });
}

Expand All @@ -20,15 +20,15 @@ export class RoundsRepository extends Repository implements Database.IRoundsRepo
* @param {Number} round
* @return {Promise}
*/
public async delete(round) {
public async delete(round): Promise<void> {
return this.db.none(sql.delete, { round });
}

/**
* Get the model related to this repository.
* @return {Round}
*/
public getModel() {
public getModel(): Round {
return new Round(this.pgp);
}
}
9 changes: 6 additions & 3 deletions packages/core-database/src/database-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class DatabaseService implements Database.IDatabaseService {
public blocksInCurrentRound: any[] = null;
public stateStarted: boolean = false;
public restoredDatabaseIntegrity: boolean = false;
public forgingDelegates: any[] = null;
public forgingDelegates: Database.IDelegateWallet[] = null;
public cache: Map<any, any> = new Map();

constructor(
Expand Down Expand Up @@ -141,7 +141,7 @@ export class DatabaseService implements Database.IDatabaseService {
this.connection.enqueueDeleteRound(height);
}

public async getActiveDelegates(height: number, delegates?: any[]) {
public async getActiveDelegates(height: number, delegates?: Database.IDelegateWallet[]) {
const maxDelegates = this.config.getMilestone(height).activeDelegates;
const round = Math.floor((height - 1) / maxDelegates) + 1;

Expand All @@ -151,7 +151,9 @@ export class DatabaseService implements Database.IDatabaseService {

// When called during applyRound we already know the delegates, so we don't have to query the database.
if (!delegates || delegates.length === 0) {
delegates = await this.connection.roundsRepository.findById(round);
delegates = ((await this.connection.roundsRepository.findById(
round,
)) as unknown) as Database.IDelegateWallet[];
}

const seedSource = round.toString();
Expand All @@ -169,6 +171,7 @@ export class DatabaseService implements Database.IDatabaseService {

this.forgingDelegates = delegates.map(delegate => {
delegate.round = +delegate.round;
delegate.username = this.walletManager.findByPublicKey(delegate.publicKey).username;
return delegate;
});

Expand Down
4 changes: 2 additions & 2 deletions packages/core-database/src/wallet-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ export class WalletManager implements Database.IWalletManager {
* @param height
* @return {Array}
*/
public loadActiveDelegateList(maxDelegates: number, height?: number): any[] {
public loadActiveDelegateList(maxDelegates: number, height?: number): Database.IDelegateWallet[] {
if (height > 1 && height % maxDelegates !== 1) {
throw new Error("Trying to build delegates outside of round change");
}
Expand Down Expand Up @@ -252,7 +252,7 @@ export class WalletManager implements Database.IWalletManager {

this.logger.debug(`Loaded ${delegates.length} active ${pluralize("delegate", delegates.length)}`);

return delegates;
return delegates as Database.IDelegateWallet[];
}

/**
Expand Down
3 changes: 0 additions & 3 deletions packages/core-forger/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,12 @@
"@arkecosystem/core-p2p": "^2.3.0-next.3",
"@arkecosystem/core-utils": "^2.3.0-next.3",
"@arkecosystem/crypto": "^2.3.0-next.3",
"delay": "^4.1.0",
"lodash.isempty": "^4.4.0",
"lodash.sample": "^4.2.1",
"lodash.uniq": "^4.5.0",
"pluralize": "^7.0.0"
},
"devDependencies": {
"@types/lodash.isempty": "^4.4.6",
"@types/lodash.sample": "^4.2.6",
"@types/lodash.uniq": "^4.5.6",
"@types/pluralize": "^0.0.29"
},
Expand Down
Loading

0 comments on commit 327b98c

Please sign in to comment.