-
Notifications
You must be signed in to change notification settings - Fork 1.5k
P2P message rate limit #6297
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
Comments
It seems necessary to limit the speed of P2P messages, and Ethereum has done the same thing. I would also like to ask, has the Tron network encountered similar problems so far? |
@jakamobiii As far as I know, there has been no such problem so far. Or it may have happened, but it did not cause obvious impact and therefore went undetected. But in any case, it is better to prevent it before it happens. |
@fyyhtx Will Ethereum's Goodbye message also disconnect? If so, referring to Ethereum's logic, can Tron's P2P_DISCONNECT message also be limited to 1 qps? |
@jwrct Yes, the Ethereum code also disconnects the peer after sending the Goodbye message. Your suggestion is very good, we can consider limiting the frequency of P2P_DISCONNECT messages to 1 qps. |
This is an excellent proposal. It would be beneficial to make it configurable, as some FullNodes may only wish to synchronize with the Mainnet or consider other proposals. |
@Sunny6889 This is a great suggestion, I think I will consider your suggestion carefully in the future implementation process. |
@fyyhtx I noticed that the latest java-tron code still has the processing logic for P2P_PING and P2P_PONG. Although the sending logic has been deleted, do you think it is necessary to add rate limit processing for them as well? |
@jwrct The removal of P2P_PING and P2P_PONG processing logic is indeed missed. It would be better to remove them in the version that releases P2P message rate limit or earlier, so that there is no need to consider rate limiting them. |
The preliminary implementation plan is as follows:
|
The first version of the implementation code has been preliminarily completed. Those who are interested can help review it. For specific codes, please refer to: fyyhtx@aa682c9 and fyyhtx@e50e11f |
Uh oh!
There was an error while loading. Please reload this page.
Background
Java-tron currently does not limit the rate of processing P2P messages, but the processing capacity of network nodes is limited by physical resources such as bandwidth, CPU, and memory. Processing a large number of P2P messages will cause excessive consumption of resources. Competition for resources among different applications may cause the following problems:
For example, SyncBlockChainMessage is sent to request synchronization of block lists, and FetchInvDataMessage is sent to request blocks or transactions. Java-tron currently does not limit the frequency of processing these request messages. If a large number of these request messages are received in a short period of time, when processing these messages, since the size of the reply message is much larger than the size of the request message, it is very likely that the node's bandwidth, CPU, memory and other system resources will be over-consumed, thereby affecting the stability of the node service.
By comparing with the code implementation of Ethereum, it is found that Ethereum has set corresponding qps limits for related messages. Before processing a message, it will first check whether the rate exceeds the limit. If it exceeds the limit, the message will no longer be processed and the corresponding error code will be responded.
Rationale
You can use the frequency of normal message sending as a benchmark to set appropriate frequency limits for different messages without affecting normal functions. The current P2P messages of java-tron are sorted out as follows:
When the limit is exceeded, the message is discarded directly and the number of times the peer exceeds the limit is recorded for use by the peer scoring logic.
Implementation
Do you have ideas regarding the implementation of this feature?
yes
Are you willing to implement this feature?
yes
The text was updated successfully, but these errors were encountered: