Skip to content
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

increase the default max message size for p2p messages #4120

Merged
merged 6 commits into from
Jul 20, 2022

Conversation

pinges
Copy link
Contributor

@pinges pinges commented Jul 18, 2022

The current limit means that we are disconnecting useful peers during syncing.

Signed-off-by: Stefan [email protected]

Addresses: #4119

Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this didn't impact any test code, that probably means we don't have this covered as well as we should. How did we end up at doubling the value?

@pinges
Copy link
Contributor Author

pinges commented Jul 20, 2022

Currently there is a 10MB limit for incoming messages in EthProtocolManager. When a message exceeds 10MB we are ignoring the message and disconnect the peer that sent the message. There is no hard limit for messages on the eth layer in the devp2p spec.
This PR removes this check as we found out that some useful peers are sending messages greater than 10MB, resulting in their disconnection.
The message size is still limited to 16.7 MB because that is the maximum message size allowed on the RLPx layer.
We have spun up 2 nodes on Goerli that successfully synced (fast sync) within 6.5 hours with that change in place.

@pinges pinges enabled auto-merge (squash) July 20, 2022 03:00
@macfarla
Copy link
Contributor

Currently there is a 10MB limit for incoming messages in EthProtocolManager. When a message exceeds 10MB we are ignoring the message and disconnect the peer that sent the message. There is no hard limit for messages on the eth layer in the devp2p spec. This PR removes this check as we found out that some useful peers are sending messages greater than 10MB, resulting in their disconnection. The message size is still limited to 16.7 MB because that is the maximum message size allowed on the RLPx layer. We have spun up 2 nodes on Goerli that successfully synced (fast sync) within 6.5 hours with that change in place.

do we still have a test for the 16.7Mb limit? is that at the Rlpx layer?

@@ -202,28 +202,9 @@ public void disconnectOnWrongChainId() {
}
}

@Test
public void disconnectOnVeryLargeMessage() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still have a test for the 16Mb limit? is that at the Rlpx layer?

@pinges pinges merged commit 86197c4 into hyperledger:main Jul 20, 2022
@pinges
Copy link
Contributor Author

pinges commented Jul 20, 2022

The RLPx message size is tested in test shouldThrowExceptionWhenFramingMessageTooLong in class FramerTest

@pinges pinges deleted the IncreaseDefaultMaxMessageSize branch July 20, 2022 05:02
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
)

* remove the limit for the incoming message size. The size is limited on the RLPx layer (16.7MB)

Signed-off-by: Stefan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants