Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Received post block broadcast request in unexpected format - Closes #2453 #2552

Merged
merged 2 commits into from
Nov 19, 2018

Conversation

lsilvs
Copy link
Contributor

@lsilvs lsilvs commented Nov 14, 2018

What was the problem?

Node was receiving postBlock call from a peer not listed in the connectionsTable.

How did I fix it?

As the issue is not reproducible I've improved the code by not undoing the connectionsTable operation in case slaveToMasterSender.send fails.
The reason is because when PeersUpdateRules.prototype.{insert|remove} is invoked it means the socket has already connected/disconnected hence the connectionsTable should not be reverted even when slaveToMasterSender.send fails.

How to test it?

Run network tests

Review checklist

When PeersUpdateRules.prototype.{insert|remove} is invoked it means the socket has already connected/disconnected hence the connectionsTable should not be reverted in the case slaveToMasterSender.send fails
@@ -67,7 +67,6 @@ PeersUpdateRules.prototype.insert = function(peer, connectionId, cb) {
peer,
err => {
if (err) {
connectionsTable.remove(peer.nonce);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we move also the connectionsTable.add, after the operation succeeds on the master process, to line https://github.com/LiskHQ/lisk/blob/6feb9a42e13884453b4de3fc64760acdf3af7ee5/api/ws/workers/peers_update_rules.js#L80?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What the master process will do is to perform the outbound connection but the inbound is already done. We should not wait for the outbound connection in order to populate connectionsTable because it is used to add the nonce to new requests and it makes no difference to have the outbound already or not.

@@ -67,7 +67,6 @@ PeersUpdateRules.prototype.insert = function(peer, connectionId, cb) {
peer,
err => {
if (err) {
connectionsTable.remove(peer.nonce);
if (!err.code) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we move also the connectionsTable.remove, after the operation succeeds on the master process, to line https://github.com/LiskHQ/lisk/blob/6feb9a42e13884453b4de3fc64760acdf3af7ee5/api/ws/workers/peers_update_rules.js#L99 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same explanation as above

@MaciejBaj MaciejBaj merged commit 704472c into development Nov 19, 2018
@MaciejBaj MaciejBaj deleted the 2453-block-broadcast-request-unexpected-format branch November 19, 2018 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Received post block broadcast request in unexpected format
2 participants