-
Notifications
You must be signed in to change notification settings - Fork 457
Rewrite peers module - #409 #441
Rewrite peers module - #409 #441
Conversation
@@ -40,6 +40,7 @@ | |||
"semver": "=5.3.0", | |||
"socket.io": "=1.4.8", | |||
"sodium": "LiskHQ/node-sodium#07ba174", | |||
"steed": "=1.1.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
scope.bus.message('bind', scope.modules); | ||
scope.logic.peers.bind(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expose modules and library to logic.peers
…/lisk into 409_rewrite-peers-module
var Router = require('../helpers/router.js'); | ||
var sandboxHelper = require('../helpers/sandbox.js'); | ||
var schema = require('../schema/peers.js'); | ||
var async = require('async'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted.
var updated = 0; | ||
library.logger.trace('Importing peers from database'); | ||
library.db.any(sql.getAll).then(function (rows) { | ||
library.logger.trace('Imported peers from database', {count: rows.length}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be info
level
return t.batch(queries); | ||
}) | ||
.then(function (data) { | ||
library.logger.debug('Peers exported to database'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be info
level
return peer.ip === pip && peer.port === port; | ||
}); | ||
if (frozenPeer) { | ||
//FIXME: Keeping peer frozen is bad idea at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure here
var consensus = Math.round(options.matched / peers.length * 100 * 1e2) / 1e2; | ||
consensus = isNaN(consensus) ? 0 : consensus; | ||
consensus = isNaN(consensus) ? 0 : consensus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intendation
|
||
req.sanitize(headers, schema.headers, function (err, report) { | ||
if (err) { return next(err); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will never have err
here, so no need to check for
@@ -245,6 +244,12 @@ __private.attachApi = function () { | |||
}); | |||
}); | |||
|
|||
router.get('/ping', function (req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New endpoint, experimental ;)
@@ -348,12 +353,12 @@ __private.banPeer = function (options) { | |||
library.logger.trace('Peer ban skipped', {options: options}); | |||
return false; | |||
} | |||
library.logger.warn([options.code, ['Ban', options.peer.string, (options.clock / 60), 'minutes'].join(' '), options.req.method, options.req.url].join(' ')); | |||
modules.peers.state(options.peer.ip, options.peer.port, 0, options.clock); | |||
library.logger.debug([options.code, ['Ban', options.peer.string, (options.clock / 60), 'minutes'].join(' '), options.req.method, options.req.url].join(' ')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changeg to debug
level because it's not so important
|
||
// Public methods | ||
Peers.prototype.bind = function (scope) { | ||
modules = scope.modules; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also modules are not used anywhere here as far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I left support for that for future usage. Maybe we should move some other functions from modules/peers.js
to logic.
logic/peers.js
Outdated
}; | ||
|
||
Peers.prototype.ban = function (ip, port, seconds) { | ||
return library.logic.peers.upsert({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this
return true; | ||
}; | ||
|
||
Peers.prototype.ban = function (ip, port, seconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are passing whole peer object / instance to every other function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, logic should be changed here.
0e7631a
to
a8142ba
Compare
}; | ||
|
||
peer = self.create(peer); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check here if peer is valid one (have string
property).
var passed = true; | ||
_.each(filter, function (value, key) { | ||
// Special case for dapp peers | ||
if (key === 'dappid' && (peer[key] === null || (Array.isArray(peer[key]) && !_.includes(peer[key], String(value))))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something wrong with that, need more tests.
{name: 'height', def: 1}, | ||
{name: 'os', def: null}, | ||
{name: 'version', def: null}, | ||
{name: 'broadhash', def: null, init: function (col) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need more tests.
var where = []; | ||
var params = {}; | ||
__private.dbSave = function (cb) { | ||
var peers = library.logic.peers.list(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call normalized version library.logic.peers.list(true);
, so later not need to set default values for SQL query - will be more readable.
Closing and reopening with branch conflicts resolved against latest development. |
No description provided.