Update loader module in chain to use network module and fix the tests - Closes #2875 #3243

jondubois · 2019-04-02T10:43:40Z

What was the problem?

The loader was interacting with individual peers directly (e.g. for synching).

How did I fix it?

The loader module now interacts with the network via the new network module.
Fixed a few issues related to the network module.

How to test it?

Run the functional and integration tests.
Sync the node with testnet and check that the loader functionality still works.

Review checklist

The PR resolves Update loader module in chain to use network module and fix the tests #2875
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

jondubois · 2019-04-02T10:47:51Z

@ManuGowda I added you because it would be nice if QA team could test this branch on the network to verify that synching still works and that it doesn't introduce any new issues.

I was able to verify that synching still works by running on my local machine and synching with testnet (it was correctly adding blocks to the DB table). I did notice some disconnection errors (which is expected because my node was not exposed to the internet). It would be good to check that these errors do not show up when running the node on a remote instance as part of testnet.

ManuGowda

Overall the PR LGTM, I have made few comments related to function and variable naming and also log messages and log level usage. Also the network tests were removed, I hope the removed tests are covered somewhere?

ManuGowda · 2019-04-02T13:53:52Z

framework/src/modules/chain/chain.js

@@ -181,7 +181,9 @@ module.exports = class Chain {
 			scope.bus.message('bind', scope);

 			// Listen to websockets
-			await scope.webSocket.listen();
+			if (scope.config.peers.enabled) {


I think this has been fixed in the development branch, can you rebase `development branch?

ManuGowda · 2019-04-02T14:41:47Z

framework/src/modules/chain/chain.js

+					true,
+					global.constants.MAX_SHARED_TRANSACTIONS
+				);
+				const multisignatures = [];


we can rewrite this to

return transactions.map(transaction => { if (transaction.signatures && transaction.signatures.length) { return ({ transaction: transaction.id, signatures: transaction.signatures, }); } });

ManuGowda · 2019-04-02T14:45:04Z

framework/src/modules/chain/chain.js

+							limit: 34, // 1977100 bytes
+							lastId: query.lastBlockId,
+						},
+						(err, data) => {


to avoid variable shadowing, we can name err to specific error here. and same for data

ManuGowda · 2019-04-02T14:50:02Z

framework/src/modules/chain/submodules/blocks/process.js

+	async function getFromNetwork() {
+		// TODO: If there is an error, invoke the applyPenalty action on the Network module once it is implemented.
+		// TODO: Rename procedure to include target module name. E.g. chain:blocks
+		const result = await library.channel.invoke('network:request', {


use destructuring const { data } =

ManuGowda · 2019-04-02T14:50:33Z

framework/src/modules/chain/submodules/blocks/process.js

+			},
+		});
+
+		if (!result.data) {


update here and below after destructure changes

ManuGowda · 2019-04-02T15:18:57Z

framework/src/modules/chain/submodules/loader.js

+
+	try {
+		await validate(result, definitions.WSTransactionsResponse);
+	} catch (validateErrors) {


please do the same for signatures

ManuGowda · 2019-04-02T15:21:01Z

framework/src/modules/chain/submodules/loader.js

+		try {
+			library.logic.transaction.objectNormalize(transaction);
+		} catch (error) {
+			library.logger.debug('Transaction normalization failed', {


logger.debug? not logger.error?

ManuGowda · 2019-04-02T15:21:57Z

framework/src/modules/chain/submodules/loader.js

-	);
+			});
+		} catch (error) {
+			library.logger.debug(error);


same as above

ManuGowda · 2019-04-02T15:25:07Z

framework/src/modules/network/network.js

 		this.p2p.on(EVENT_FAILED_TO_FETCH_PEER_INFO, error => {
-			this.logger.debug(error.message);
+			this.logger.debug(error.message || error);


why logger.debug? why not logger.error? same for below.

ManuGowda · 2019-04-02T15:27:19Z

framework/src/modules/network/network.js

 			} catch (error) {
-				request.error(error); // Send an error back to the peer.
 				this.logger.debug(


same here as well.

framework/src/modules/chain/chain.js

framework/src/modules/chain/submodules/loader.js

shuse2 · 2019-04-03T06:21:13Z

framework/src/modules/chain/chain.js

+							lastId: query.lastBlockId,
+						},
+						(err, data) => {
+							(data || []).forEach(block => {


prefer early return if data` is undefined

framework/src/controller/bus.js

ishantiw

Most of the concerns are covered by comments from others, just a minor comment. Also, if you need the whole config to come to network module then maybe you can add below line,

app.overrideModuleOptions('network', { config });

to src/index.js. I have added it in my PR though.

framework/src/modules/network/network.js

framework/src/modules/chain/submodules/blocks.js

diego-G · 2019-04-03T13:50:16Z

framework/src/modules/network/network.js

@@ -12,8 +12,11 @@ const {
 	EVENT_REQUEST_RECEIVED,


I miss tests for this entire file.

diego-G · 2019-04-03T13:51:38Z

framework/test/mocha/unit/modules/chain/submodules/loader.js

@@ -455,286 +379,4 @@ describe('loader', () => {
 			});
 		});
 	});
-
-	describe('__private.loadBlocksFromNetwork', () => {


Why do we remove this instead of adapting it to the new times?

These unit tests and the underlying logic were poorly implemented. This refactoring improves this a bit but it's still not in a sufficiently clean/settled state to be unit tested.

So how do you assure it's working well?

diego-G · 2019-04-03T13:52:08Z

framework/src/modules/chain/submodules/loader.js

-						definitions.WSSignaturesResponse,
-						validateErr => setImmediate(waterCb, validateErr, res.signatures)
-					);
+__private.loadSignatures = async function() {


We should cover now this function with unit tests.

I think it's better to focus more on functional tests at this stage because the implementation details are still not in a settled state.

If the implementation details should be clear enough because what we want it's to replicate the exact behaviour that it's currently working. Anyways, I might agree if we add new functional test cases and we don't skip any network test.

diego-G · 2019-04-03T13:52:21Z

framework/src/modules/chain/submodules/loader.js

-								peer.string
-							);
-							modules.peers.remove(peer);
+__private.loadTransactions = async function() {


We should cover now this function with unit tests.

framework/src/modules/chain/submodules/loader.js

diego-G · 2019-04-03T13:55:16Z

framework/src/modules/chain/chain.js

+					const query = action.params[0] || {};
+					this.scope.modules.blocks.utils.loadBlocksData(
+						{
+							limit: 34, // 1977100 bytes


We should research and update this number. If I remember well it's not accurate. Block size varies depending on the transaction associated to it. We might be able to increase the number significantly.

We should raise a new issue.

For this current protocol, let's keep it this way because it's not really a protocol change, and we don't want to risk here.
However, please create an issue to address this, and we can tackle this when we change the p2p protocol

@jondubois would you mind to open the issue?

diego-G · 2019-04-03T13:57:38Z

framework/src/modules/chain/chain.js

@@ -272,6 +295,40 @@ module.exports = class Chain {
 					action.params[0],
 					action.params[1]
 				),
+			blocks: async action =>


I would suggest to pass the requested number of blocks as a variable, or returning a dynamic number depending on the sizes of the requested blocks.

Yes I think as a new issue.

@jondubois @diego-G same as above, we don't want to change the behavior now, so please create a new issue, and we will tackle later

…eracting with peers directly

jondubois · 2019-04-04T13:57:17Z

@shuse2 I raised an issue about allowing some modules to publish any event #3263

framework/src/modules/chain/submodules/blocks/process.js

framework/src/modules/chain/submodules/loader.js

…network_module

shuse2 · 2019-04-08T06:58:04Z

framework/src/modules/chain/submodules/blocks/process.js

-		(err, res) => {
-			// If comparison failed and current consensus is low - perform chain recovery
-			if (comparisonFailed && modules.transport.poorConsensus()) {
-				return modules.blocks.chain.recoverChain(cb);


this logic needs to stay

Ah yep, we need the recoverChain(...) feature but it should be possible to use it as part of the loadBlocksFromNetwork logic instead (we may be able to use that logic to detect a fork) - It would be nice to remove the commonBlocks logic.

diego-G · 2019-04-08T11:04:56Z

@jondubois @shuse2 in order to move on with PRs from P2P epic, what would you say about creating at least thoughtful test skeletons? That way we can tackle then later without forgetting the acknowledged context.

jondubois · 2019-04-08T11:43:06Z

@diego-G I think we need more (and better) functional tests but it should be a separate issue which we can work towards gradually.

I don't think that we should write unit tests (or the skeleton) until the logic has settled into a desirable working state first - Until we finish migrating to the new Network module, our internal logic is still likely to change and we should keep this flexibility.

But we can raise an issue for later.

jondubois requested review from shuse2 and ManuGowda April 2, 2019 10:43

shuse2 requested a review from ishantiw April 2, 2019 10:46

shuse2 added the 👁️ pending review label Apr 2, 2019

shuse2 assigned jondubois Apr 2, 2019

ManuGowda suggested changes Apr 2, 2019

View reviewed changes

shuse2 suggested changes Apr 3, 2019

View reviewed changes

ishantiw reviewed Apr 3, 2019

View reviewed changes

framework/src/modules/network/network.js Show resolved Hide resolved

framework/src/modules/network/network.js Outdated Show resolved Hide resolved

diego-G self-requested a review April 3, 2019 13:03

diego-G suggested changes Apr 3, 2019

View reviewed changes

jondubois added 14 commits April 4, 2019 10:39

Allow modules to emit any events on the bus under their own namespace

435429d

Expose synching methods on chain module

a62a26d

Migrate blocks submodule to use the new network module instead of int…

bb90658

…eracting with peers directly

Fix network module compatibility issues with v1 lisk-core network

1dd941c

Change loader to use the new network module instead of old peer system

b680818

Fix integration test related to blocks process

73bd7e4

Fix unit tests related to blocks process

d256395

Fix loader unit tests

7552813

Remove failing unit tests

e9528a0

Remove unused variable

7ad150e

Skip network tests until we have fixed the config format

b381a30

Resolve QA feedback

21df01d

Cleanup to account for changes to applicationState

c703035

Fix lint issue

4211761

jondubois force-pushed the 2875-loader_to_use_network_module branch from c06ed9d to 4211761 Compare April 4, 2019 13:14

jondubois added 3 commits April 4, 2019 16:21

Skip jest unit test case for which requirements are currently unclear

90f4eb9

Skip test cases for which the requirements are not yet known

f38f193

Skip network test until we have the new config format

b7d21a1

Fix linting

9540355

shuse2 suggested changes Apr 5, 2019

View reviewed changes

framework/src/modules/chain/submodules/blocks/process.js Show resolved Hide resolved

framework/src/modules/chain/submodules/loader.js Show resolved Hide resolved

framework/src/modules/chain/submodules/loader.js Show resolved Hide resolved

ishantiw mentioned this pull request Apr 5, 2019

Update api peer endpoints use network module - Closes#2879 #3180

Merged

shuse2 approved these changes Apr 5, 2019

View reviewed changes

Merge branch 'feature/create_network_module' into 2875-loader_to_use_…

275d969

…network_module

ManuGowda approved these changes Apr 5, 2019

View reviewed changes

shuse2 reviewed Apr 8, 2019

View reviewed changes

diego-G approved these changes Apr 8, 2019

View reviewed changes

shuse2 added ✅ ready and removed 👁️ pending review labels Apr 8, 2019

shuse2 merged commit 1a5078a into feature/create_network_module Apr 8, 2019

shuse2 deleted the 2875-loader_to_use_network_module branch April 8, 2019 11:51

Update loader module in chain to use network module and fix the tests - Closes #2875 #3243

Update loader module in chain to use network module and fix the tests - Closes #2875 #3243

Conversation

jondubois commented Apr 2, 2019

What was the problem?

How did I fix it?

How to test it?

Review checklist

jondubois commented Apr 2, 2019

ManuGowda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego-G Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego-G Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

diego-G Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondubois commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondubois Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

diego-G commented Apr 8, 2019 • edited Loading

jondubois commented Apr 8, 2019 • edited Loading

diego-G Apr 3, 2019 •

edited

Loading

diego-G Apr 5, 2019 •

edited

Loading

diego-G Apr 3, 2019 •

edited

Loading

jondubois Apr 8, 2019 •

edited

Loading

diego-G commented Apr 8, 2019 •

edited

Loading

jondubois commented Apr 8, 2019 •

edited

Loading