If an existing peer tries to connect to us, the inbound connection is kept even though they are not added to our peer list - Closes #4170 #4171
Conversation
…ectly disconnected
jondubois
requested review from
diego-G,
ishantiw,
mitsuaki-u,
shuse2 and
4miners
| export const FORBIDDEN_CONNECTION = 4403; | ||
| export const FORBIDDEN_CONNECTION_REASON = 'Peer is not allowed to connect'; | ||
|
|
||
| export const DUPLICATE_CONNECTION = 4404; |
There was a problem hiding this comment.
Errors look too similar. Can we use a more standalone name? https://github.com/LiskHQ/lisk-sdk/blob/decb81e6489730810a18f7968db424860ebf7c2d/elements/lisk-p2p/src/disconnect_status_codes.ts#L38
There was a problem hiding this comment.
The first/original connection can be outbound so the suggested name would create confusion because it makes it sound like there are two duplicate inbound connections which is usually not the case.
There was a problem hiding this comment.
Sorry, it wasn't a suggestion. I've just showed a similar variable that we are introducing in another PR #4169. Shall we change this variable's name or the other?
There was a problem hiding this comment.
Ah, I think it may be the same error. We should change it in the other PR so that they match because Ishan is working on it in parallel.
elements/lisk-p2p/src/peer_pool.ts
Outdated
|
|
||
| public constructor(peerPoolConfig: PeerPoolConfig) { | ||
| super(); | ||
| this._peerMap = new Map(); | ||
| this._peerPoolConfig = peerPoolConfig; | ||
| this._inboundPeerConfig = { |
There was a problem hiding this comment.
why does inbound have no config for banTime and wsMaxPayload but outbound does
There was a problem hiding this comment.
wsMaxPayload is not necessary for inbound because it is set on the SocketCluster server (not on the peer socket). It seems that banTime is not used in any of the peer classes. I guess we can remove it.
What was the problem?
In one of the
ifconditions where we refuse to add the inbound peer, we forget to disconnect their socket.How did I solve it?
How to manually test it?
A new integration test case was added.
Run
npm testReview checklist