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

refactor(core-p2p): Remove unnecessary ping() call #2123

Merged
merged 9 commits into from
Feb 20, 2019
Merged

refactor(core-p2p): Remove unnecessary ping() call #2123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0f36254
refactor(core-p2p): Remove unnecessary ping() call
vasild Feb 20, 2019
601e511
refactor(core-p2p): Use acceptNewPeer() from discoverPeers()
vasild Feb 20, 2019
40d9c16
Merge branch 'develop' into remove-ping
faustbrian Feb 20, 2019
21f0b10
fix(core-p2p): add missing await
vasild Feb 20, 2019
3b83991
test: fix getNetworkHeight
spkjp Feb 20, 2019
3123725
fix: use Promise.all
spkjp Feb 20, 2019
682ad73
refactor: remove milestoneHash
spkjp Feb 20, 2019
7252448
refactor: pass function to map directly
spkjp Feb 20, 2019
89e3246
revert: previous commit
spkjp Feb 20, 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
6 changes: 0 additions & 6 deletions packages/core-container/src/config/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,6 @@ class Config {
this.config.exceptions = crypto.get("exceptions");
this.config.milestones = crypto.get("milestones");
this.config.genesisBlock = crypto.get("genesisBlock");

// Calculate milestone hash
const milestonesBuffer = Buffer.from(JSON.stringify(this.config.milestones));
this.config.milestoneHash = HashAlgorithms.sha256(milestonesBuffer)
.slice(0, 8)
.toString("hex");
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/core-interfaces/src/core-p2p/peer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export interface IPeer {
ip: any;
port: number;
nethash: any;
milestoneHash: string;
version: any;
os: any;
status: any;
Expand Down
21 changes: 0 additions & 21 deletions packages/core-p2p/__tests__/court/guard.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,12 @@ import { defaults } from "../../src/defaults";
import { Peer } from "../../src/peer";
import { setUp, tearDown } from "../__support__/setup";

const CORE_ENV = process.env.CORE_ENV;
let guard;
let peerMock;

beforeAll(async () => {
await setUp();

app.getConfig().set("milestoneHash", "dummy-milestone");

guard = require("../../src/court/guard").guard;
guard.config.set("minimumVersions", [">=2.0.0"]);
});
Expand All @@ -28,25 +25,20 @@ beforeEach(async () => {

// this peer is here to be ready for future use in tests (not added to initial peers)
peerMock = new Peer("1.0.0.99", 4002);
peerMock.milestoneHash = "dummy-milestone";
Object.assign(peerMock, peerMock.headers);
});

describe("Guard", () => {
describe("isSuspended", () => {
it("should return true", async () => {
process.env.CORE_ENV = "false";
await guard.monitor.acceptNewPeer(peerMock);
process.env.CORE_ENV = CORE_ENV;

expect(guard.isSuspended(peerMock)).toBe(true);
});

it("should return false because passed", async () => {
process.env.CORE_ENV = "false";
await guard.monitor.acceptNewPeer(peerMock);
guard.suspensions[peerMock.ip].until = dayjs().subtract(1, "minute");
process.env.CORE_ENV = CORE_ENV;

expect(guard.isSuspended(peerMock)).toBe(false);
});
Expand Down Expand Up @@ -104,7 +96,6 @@ describe("Guard", () => {

const dummy = {
nethash: "d9acd04bde4234a81addb8482333b4ac906bed7be5a9970ce8ada428bd083192",
milestoneHash: "dummy-milestone",
version: "2.1.1",
status: 200,
state: {},
Expand All @@ -115,7 +106,6 @@ describe("Guard", () => {

const { until, reason } = guard.__determineOffence({
nethash: "d9acd04bde4234a81addb8482333b4ac906bed7be5a9970ce8ada428bd083192",
milestoneHash: "dummy-milestone",
ip: "dummy-ip-addr",
});

Expand All @@ -140,7 +130,6 @@ describe("Guard", () => {
it('should return a 5 minute suspension for "Invalid Version"', () => {
const { until, reason } = guard.__determineOffence({
nethash: "d9acd04bde4234a81addb8482333b4ac906bed7be5a9970ce8ada428bd083192",
milestoneHash: "dummy-milestone",
version: "1.0.0",
status: 200,
delay: 1000,
Expand All @@ -150,16 +139,6 @@ describe("Guard", () => {
expect(convertToMinutes(until)).toBe(5);
});

it('should return a 5 minute suspension for "Invalid Milestones"', () => {
const { until, reason } = guard.__determineOffence({
...dummy,
milestoneHash: "wrong-milestone",
});

expect(reason).toBe("Invalid Milestones");
expect(convertToMinutes(until)).toBe(5);
});

it('should return a 10 minutes suspension for "Node is not at height"', () => {
guard.monitor.getNetworkHeight = jest.fn(() => 154);

Expand Down
5 changes: 1 addition & 4 deletions packages/core-p2p/__tests__/monitor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,9 @@ describe("Monitor", () => {
peerMock.headers,
]);

process.env.CORE_ENV = "false";

await monitor.acceptNewPeer(peerMock);

expect(monitor.peers[peerMock.ip]).toBeObject();

process.env.CORE_ENV = "test";
});
});

Expand Down Expand Up @@ -155,6 +151,7 @@ describe("Monitor", () => {
]);
axiosMock.onGet(/.*\/peer\/list/).reply(() => [200, { peers: [] }, peerMock.headers]);
await monitor.discoverPeers();
await monitor.cleanPeers();

const height = await monitor.getNetworkHeight();
expect(height).toBe(2);
Expand Down
14 changes: 0 additions & 14 deletions packages/core-p2p/src/court/guard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,16 +198,6 @@ export class Guard {
return nethash === config.get("network.nethash");
}

/**
* Determine if the peer is has the same milestones.
* @param {Peer} peer
* @return {Boolean}
*/
public isValidMilestoneHash(peer) {
const milestoneHash = peer.milestoneHash || (peer.headers && peer.headers.milestoneHash);
return milestoneHash === config.get("milestoneHash");
}

/**
* Determine if the peer has a valid port.
* @param {Peer} peer
Expand Down Expand Up @@ -285,10 +275,6 @@ export class Guard {
return this.__determinePunishment(peer, offences.INVALID_VERSION);
}

if (!this.isValidMilestoneHash(peer)) {
return this.__determinePunishment(peer, offences.INVALID_MILESTONE_HASH);
}

// NOTE: Suspending this peer only means that we no longer
// will download blocks from him but he can still download blocks from us.
const heightDifference = Math.abs(this.monitor.getNetworkHeight() - peer.state.height);
Expand Down
91 changes: 14 additions & 77 deletions packages/core-p2p/src/monitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export class Monitor implements P2P.IMonitor {
const cachedPeers = restorePeers();
localConfig.set("peers", cachedPeers);

this.populateSeedPeers();
await this.populateSeedPeers();

if (this.config.skipDiscovery) {
logger.warn("Skipped peer discovery because the relay is in skip-discovery mode.");
Expand Down Expand Up @@ -121,7 +121,7 @@ export class Monitor implements P2P.IMonitor {
let nextRunDelaySeconds = 600;

if (!this.hasMinimumPeers()) {
this.populateSeedPeers();
await this.populateSeedPeers();
nextRunDelaySeconds = 5;
logger.info(`Couldn't find enough peers. Falling back to seed peers.`);
}
Expand All @@ -134,18 +134,13 @@ export class Monitor implements P2P.IMonitor {
* @param {Peer} peer
* @throws {Error} If invalid peer
*/
public async acceptNewPeer(peer) {
public async acceptNewPeer(peer, seed?: boolean) {
if (this.config.disableDiscovery && !this.pendingPeers[peer.ip]) {
logger.warn(`Rejected ${peer.ip} because the relay is in non-discovery mode.`);
return;
}

if (
!isValidPeer(peer) ||
this.guard.isSuspended(peer) ||
this.pendingPeers[peer.ip] ||
process.env.CORE_ENV === "test"
) {
if (!isValidPeer(peer) || this.guard.isSuspended(peer) || this.pendingPeers[peer.ip]) {
return;
}

Expand All @@ -172,7 +167,7 @@ export class Monitor implements P2P.IMonitor {
return this.guard.suspend(newPeer);
}

if (!this.guard.isValidNetwork(peer)) {
if (!this.guard.isValidNetwork(peer) && !seed) {
logger.debug(
`Rejected peer ${peer.ip} as it isn't on the same network. Expected: ${config.get(
"network.nethash",
Expand All @@ -182,24 +177,14 @@ export class Monitor implements P2P.IMonitor {
return this.guard.suspend(newPeer);
}

if (!this.guard.isValidMilestoneHash(newPeer)) {
logger.debug(
`Rejected peer ${peer.ip} as it has a different milestone hash. Expected: ${config.get(
"milestoneHash",
)} - Received: ${peer.milestoneHash}`,
);

return this.guard.suspend(newPeer);
}

if (this.getPeer(peer.ip)) {
return;
}

try {
this.pendingPeers[peer.ip] = true;

await newPeer.ping(1500);
await newPeer.ping(3000);

this.peers[peer.ip] = newPeer;

Expand Down Expand Up @@ -388,12 +373,7 @@ export class Monitor implements P2P.IMonitor {
for (const peer of shuffledPeers) {
try {
const hisPeers = await peer.getPeers();

for (const p of hisPeers) {
if (isValidPeer(p) && !this.getPeer(p.ip)) {
this.addPeer(p);
}
}
await Promise.all(hisPeers.map(p => this.acceptNewPeer(p)));
} catch (error) {
// Just try with the next peer from shuffledPeers.
}
Expand Down Expand Up @@ -769,35 +749,6 @@ export class Monitor implements P2P.IMonitor {
}
}

/**
* Add a new peer after it passes a few checks.
* @param {Peer} peer
* @return {void}
*/
private addPeer(peer) {
if (this.guard.isBlacklisted(peer)) {
return;
}

if (!this.guard.isValidVersion(peer)) {
return;
}

if (!this.guard.isValidNetwork(peer)) {
return;
}

if (!this.guard.isValidMilestoneHash(peer)) {
return;
}

if (!this.guard.isValidPort(peer)) {
return;
}

this.peers[peer.ip] = new Peer(peer.ip, peer.port);
}

/**
* Schedule the next update network status.
* @param {Number} nextUpdateInSeconds
Expand Down Expand Up @@ -835,7 +786,7 @@ export class Monitor implements P2P.IMonitor {
* Populate the initial seed list.
* @return {void}
*/
private populateSeedPeers() {
private async populateSeedPeers() {
const peerList = config.get("peers.list");

if (!peerList) {
Expand All @@ -851,26 +802,12 @@ export class Monitor implements P2P.IMonitor {
peers = { ...peers, ...localConfig.get("peers") };
}

const filteredPeers: any[] = Object.values(peers).filter((peer: any) => {
if (!isValidPeer(peer)) {
return false;
}

if (!this.guard.isValidPort(peer)) {
return false;
}

if (!this.guard.isValidVersion(peer)) {
return false;
}

return true;
});

for (const peer of filteredPeers) {
delete this.guard.suspensions[peer.ip];
this.peers[peer.ip] = new Peer(peer.ip, peer.port);
}
return Promise.all(
Object.values(peers).map((peer: any) => {
delete this.guard.suspensions[peer.ip];
return this.acceptNewPeer(peer, true);
}),
);
}
}

Expand Down
Loading