-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(net): add rate limiting logic for P2P messages #6393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release_v4.8.1
Are you sure you want to change the base?
Changes from all commits
aa682c9
e50e11f
c2adb7d
3b3ddac
2454a92
81c6528
26d5fe4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,9 +178,11 @@ private void processMessage(PeerConnection peer, byte[] data) { | |
handshakeService.processHelloMessage(peer, (HelloMessage) msg); | ||
break; | ||
case P2P_DISCONNECT: | ||
peer.getChannel().close(); | ||
peer.getNodeStatistics() | ||
.nodeDisconnectedRemote(((DisconnectMessage)msg).getReason()); | ||
if (peer.getP2pRateLimiter().tryAcquire(type.asByte())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This rate limit is set per peer. This rate limiter does not affect the normal disconnection logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
peer.getChannel().close(); | ||
peer.getNodeStatistics() | ||
.nodeDisconnectedRemote(((DisconnectMessage)msg).getReason()); | ||
} | ||
break; | ||
case SYNC_BLOCK_CHAIN: | ||
syncBlockChainMsgHandler.processMessage(peer, msg); | ||
|
@@ -259,6 +261,7 @@ private void processException(PeerConnection peer, TronMessage msg, Exception ex | |
code = Protocol.ReasonCode.NO_SUCH_MESSAGE; | ||
break; | ||
case BAD_MESSAGE: | ||
case RATE_LIMIT_EXCEEDED: | ||
code = Protocol.ReasonCode.BAD_PROTOCOL; | ||
break; | ||
case SYNC_FAILED: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package org.tron.core.net; | ||
|
||
import com.google.common.cache.Cache; | ||
import com.google.common.cache.CacheBuilder; | ||
import com.google.common.util.concurrent.RateLimiter; | ||
|
||
public class P2pRateLimiter { | ||
private final Cache<Byte, RateLimiter> rateLimiters = CacheBuilder.newBuilder() | ||
.maximumSize(32).build(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The message types defined by the current protocol are very limited, and the limit of 32 is sufficient for the foreseeable future. |
||
|
||
public void register(Byte type, double rate) { | ||
RateLimiter rateLimiter = RateLimiter.create(Double.POSITIVE_INFINITY); | ||
rateLimiter.setRate(rate); | ||
rateLimiters.put(type, rateLimiter); | ||
} | ||
|
||
public void acquire(Byte type) { | ||
RateLimiter rateLimiter = rateLimiters.getIfPresent(type); | ||
if (rateLimiter == null) { | ||
return; | ||
} | ||
rateLimiter.acquire(); | ||
} | ||
|
||
public boolean tryAcquire(Byte type) { | ||
RateLimiter rateLimiter = rateLimiters.getIfPresent(type); | ||
if (rateLimiter == null) { | ||
return true; | ||
} | ||
return rateLimiter.tryAcquire(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rate limit is based on the premise of block synchronization. |
||
throw new P2pException(TypeEnum.RATE_LIMIT_EXCEEDED, fetchInvDataMsg.getType() | ||
+ " message exceeds the rate limit"); | ||
} | ||
if (fetchInvDataMsg.getHashList().size() > NetConstants.MAX_BLOCK_FETCH_PER_PEER) { | ||
throw new P2pException(TypeEnum.BAD_MESSAGE, "fetch too more blocks, size:" | ||
+ fetchInvDataMsg.getHashList().size()); | ||
} | ||
for (Sha256Hash hash : fetchInvDataMsg.getHashList()) { | ||
long blockNum = new BlockId(hash).getNum(); | ||
long minBlockNum = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package org.tron.core.net; | ||
|
||
import static org.tron.core.net.message.MessageTypes.FETCH_INV_DATA; | ||
import static org.tron.core.net.message.MessageTypes.SYNC_BLOCK_CHAIN; | ||
|
||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
public class P2pRateLimiterTest { | ||
@Test | ||
public void test() { | ||
P2pRateLimiter limiter = new P2pRateLimiter(); | ||
limiter.register(SYNC_BLOCK_CHAIN.asByte(), 2); | ||
limiter.acquire(SYNC_BLOCK_CHAIN.asByte()); | ||
boolean ret = limiter.tryAcquire(SYNC_BLOCK_CHAIN.asByte()); | ||
Assert.assertTrue(ret); | ||
limiter.tryAcquire(SYNC_BLOCK_CHAIN.asByte()); | ||
ret = limiter.tryAcquire(SYNC_BLOCK_CHAIN.asByte()); | ||
Assert.assertFalse(ret); | ||
ret = limiter.tryAcquire(FETCH_INV_DATA.asByte()); | ||
Assert.assertTrue(ret); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder should config file to add these new configurations with default values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok