Rate limit WebSocket messages - Closes #3975 #3981
Conversation
…n peer is removed
There was a problem hiding this comment.
If we are going to disconnect from a peer after it reaches the threshold of the number of messages it sends per second, in theory, it should also be moved from tried to new peer or follow the eviction rules on disconnection, do you think it will have any impact on the LIP?
Because this is partly handled in banning,
getTransactions: More than 3 requests in a 10 s time window : Honest peers should send a getTransactions message only after receiving a new postTransactionsAnnouncement message and in particular only request transactions every 5 seconds
Similar ban scores for postTransactionsAnnouncement, postSignatures, etc. My concern is, this is working in parallel to Banning and a peer is only punished when it reaches a ban score and disconnected for 24 hours. Otherwise, a peer can get evicted very quickly from newPeers list because of double punishment from two mechanisms (rateLimit and Banning).
Apart from this, it looks good.
elements/lisk-p2p/src/peer/base.ts
Outdated
| @@ -151,16 +155,23 @@ export class Peer extends EventEmitter { | |||
| responseRate: number; | |||
| lastResponded: number; | |||
| }; | |||
| private _callCounter: Map<string, number>; | |||
| private _rpcCounter: Map<string, number>; | |||
| private _rpcRates: Map<any, number>; | |||
There was a problem hiding this comment.
Why we have any type as key?
elements/lisk-p2p/src/peer/base.ts
Outdated
| [...this._rpcCounter.entries()].map(entry => { | ||
| const rate = entry[1] / this._rateInterval; | ||
|
|
||
| return [entry[0], rate] as any; |
There was a problem hiding this comment.
isn't entry[0] is string type and we can use it as key for rpcRates type as Map<string, number>?
There was a problem hiding this comment.
we don't need to typecast as any, it doesn't give me warning
There was a problem hiding this comment.
It gives me warnings in vscode if I don't put any.
elements/lisk-p2p/src/peer/base.ts
Outdated
| } | ||
|
|
||
| this._rpcRates = new Map( | ||
| [...this._rpcCounter.entries()].map(entry => { |
There was a problem hiding this comment.
maybe for clarity we can use .map([key, rate]) => or .map([key, value]) =>?
elements/lisk-p2p/src/peer/base.ts
Outdated
| this._rpcCounter = new Map(); | ||
|
|
||
| this._messageRates = new Map( | ||
| [...this._messageCounter.entries()].map(entry => { |
elements/lisk-p2p/src/peer/base.ts
Outdated
| [...this._rpcCounter.entries()].map(entry => { | ||
| const rate = entry[1] / this._rateInterval; | ||
|
|
||
| return [entry[0], rate] as any; |
There was a problem hiding this comment.
we don't need to typecast as any, it doesn't give me warning
What was the problem?
There was no rate limiting at the WebSocket level to block malicious peers.
How did I fix it?
wsMaxMessageRatemessages per second, they will be disconnected with a forbidden message.wsMaxMessageRateand to increase precision.How to test it?
New integration test cases were added.
npm run test:integrationReview checklist