feat(net): add rate limiting logic for P2P messages #6393
Conversation
fyyhtx
changed the title
| peer.getChannel().close(); | ||
| peer.getNodeStatistics() | ||
| .nodeDisconnectedRemote(((DisconnectMessage)msg).getReason()); | ||
| if (peer.getP2pRateLimiter().tryAcquire(type.asByte())) { |
There was a problem hiding this comment.
why disconnect need a rate limiter? As in your issue mentioned "After receiving the message, the connection will be disconnected and no response will be given."
There was a problem hiding this comment.
Another question: is this rate limit set per Peer or shared by all Peers? Will this rate limiter effect the normal disconnect logic?
There was a problem hiding this comment.
Another question: is this rate limit set per Peer or shared by all Peers? Will this rate limiter effect the normal disconnect logic?
This rate limit is set per peer. This rate limiter does not affect the normal disconnection logic.
There was a problem hiding this comment.
why disconnect need a rate limiter? As in your issue mentioned "After receiving the message, the connection will be disconnected and no response will be given."
To prevent the peer from sending a large number of repeated disconnect messages at once, it will not respond to the message, but will execute the disconnection logic.
|
|
||
| public class P2pRateLimiter { | ||
| private final Cache<Byte, RateLimiter> rateLimiters = CacheBuilder.newBuilder() | ||
| .maximumSize(32).build(); |
There was a problem hiding this comment.
here is a hidden size 32 limit, can make it a large one so it normally won't never hit the limit?
There was a problem hiding this comment.
The message types defined by the current protocol are very limited, and the limit of 32 is sufficient for the foreseeable future.
| @@ -164,6 +171,9 @@ public void setChannel(Channel channel) { | |||
| } | |||
| this.nodeStatistics = TronStatsManager.getNodeStatistics(channel.getInetAddress()); | |||
| lastInteractiveTime = System.currentTimeMillis(); | |||
| p2pRateLimiter.register(SYNC_BLOCK_CHAIN.asByte(), 2); | |||
| p2pRateLimiter.register(FETCH_INV_DATA.asByte(), 2); | |||
| p2pRateLimiter.register(P2P_DISCONNECT.asByte(), 1); | |||
There was a problem hiding this comment.
make this hardcode 2,2,1 number to use a static variable
There was a problem hiding this comment.
Will relay nodes trigger speed limits? What about busy sync nodes on the network?
There was a problem hiding this comment.
Will relay nodes trigger speed limits? What about busy sync nodes on the network?
For synchronization logic, relay nodes are no different from ordinary full nodes.
| @@ -156,6 +156,14 @@ private void check(PeerConnection peer, FetchInvDataMessage fetchInvDataMsg) thr | |||
| if (!peer.isNeedSyncFromUs()) { | |||
| throw new P2pException(TypeEnum.BAD_MESSAGE, "no need sync"); | |||
| } | |||
| if (!peer.getP2pRateLimiter().tryAcquire(fetchInvDataMsg.getType().asByte())) { | |||
There was a problem hiding this comment.
consumerInvToFetch task is scheduled every 30ms, but the limit FETCH_INV_DATA_RATE is 3 times/s, there will be conflict.
There was a problem hiding this comment.
This rate limit is based on the premise of block synchronization.
| @@ -318,6 +318,9 @@ public class Constant { | |||
|
|
|||
| public static final String RATE_LIMITER_HTTP = "rate.limiter.http"; | |||
| public static final String RATE_LIMITER_RPC = "rate.limiter.rpc"; | |||
| public static final String RATE_LIMITER_P2P_SYNC_BLOCK_CHAIN = "rate.limiter.p2p.syncBlockChain"; | |||
There was a problem hiding this comment.
I wonder should config file to add these new configurations with default values?
There was a problem hiding this comment.
I don't think it's necessary, they are not critical configuration items. The rate limit parameter configuration is a precautionary solution, these configuration items may never be used in practice.
What does this PR do?
Add rate limiting logic for P2P messages. Closes #6297
Why are these changes required?
This PR has been tested by:
Follow up
Extra details