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

Add get minimal peers list data for discovery process - Closes #3948 #4017

Merged

Conversation

diego-G
Copy link

@diego-G diego-G commented Jul 29, 2019

What was the problem?

Discovery process was handling a lot unneeded information which also could make us prone to attacks.

How did I solve it?

Create a new method only used in discovery process. Peers are able to send back peer lists and minimal peer list.

How to manually test it?

npm run test:integration

Review checklist

elements/lisk-p2p/src/peer/base.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
@diego-G diego-G requested a review from ishantiw July 30, 2019 09:56
pablitovicente
pablitovicente approved these changes Jul 30, 2019
@pablitovicente pablitovicente self-requested a review July 30, 2019 10:13
Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

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

Other than the variable name comment seems good but functional WS tets are failing.

Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

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

Functional WS tests failing for blocksCommon with error

TypeError: Cannot read property 'message' of undefined at Context.it.only (test/mocha/functional/ws/transport/transport.js:243:26) at process._tickCallback (internal/process/next_tick.js:68:7)

Before it was expecting err.response.message but now the same message could be found in err.message though before updating simply the access path to the value we should check if we don't affect other components with this change.

elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Just a few minor comments

elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer/base.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

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

Maybe it's the branch for P2P but functional:WS are still failing as the RPC is failing differently now (returns an error instead of an error wrapped in a response) so I would recommend to create an issue to fix those tests in a different PR but for this release. (btw this tests do pass in 2.2 for example)

@diego-G diego-G force-pushed the 3948-minimal_peers_list_data_on_discovery branch from a8fa5bf to 5e00024 Compare July 31, 2019 14:28
@jondubois jondubois merged commit 1e5467c into feature/implement-lip-p2p Jul 31, 2019
@shuse2 shuse2 deleted the 3948-minimal_peers_list_data_on_discovery branch August 2, 2019 10:06
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.

7 participants